checkbox: fix node/state sync bug for mixed state

reach · Jan 31, 2020 · c8f5c99 · c8f5c99
1 parent 7702c46
commit c8f5c99
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 25 deletions.
diff --git a/packages/checkbox/examples/basic-mixed.example.js b/packages/checkbox/examples/basic-mixed.example.js
@@ -8,21 +8,29 @@ function Example() {
   const [checked, setChecked] = useState(true);
   return (
     <div>
-      <MixedCheckbox
-        id="whatever-input"
-        value="whatever"
-        checked={checked}
-        onChange={event => {
-          setChecked(event.target.checked);
-        }}
-      />
-      <label htmlFor="whatever-input">
-        You must control the state WHATTTTTT
-      </label>
-      <button onClick={() => setChecked(!checked)}>
-        Toggle that checkbox baby
-      </button>
-      <button onClick={() => setChecked("mixed")}>Mix it up</button>
+      <div>
+        <label>
+          <MixedCheckbox checked="mixed" />
+          Perma-mixed
+        </label>
+      </div>
+      <div>
+        <MixedCheckbox
+          id="whatever-input"
+          value="whatever"
+          checked={checked}
+          onChange={event => {
+            setChecked(event.target.checked);
+          }}
+        />
+        <label htmlFor="whatever-input">
+          You must control the state WHATTTTTT
+        </label>
+        <button onClick={() => setChecked(!checked)}>
+          Toggle that checkbox baby
+        </button>
+        <button onClick={() => setChecked("mixed")}>Mix it up</button>
+      </div>
     </div>
   );
 }

diff --git a/packages/checkbox/src/mixed.tsx b/packages/checkbox/src/mixed.tsx
@@ -84,7 +84,7 @@ export enum MixedCheckboxEvents {
  * @param context
  */
 function checkToggleAllowed(data: MixedCheckboxData) {
-  return data ? !data.isControlled || !data.disabled : false;
+  return !!(data && !data.isControlled && !data.disabled);
 }
 
 /**
@@ -268,12 +268,13 @@ type MixedCheckboxArgs = {
   defaultChecked?: boolean;
   disabled?: boolean;
   onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void;
+  onClick?: (event: React.MouseEvent<HTMLInputElement>) => void;
 };
 
 export type UseMixedCheckboxProps = Required<
   Pick<
     React.InputHTMLAttributes<HTMLInputElement>,
-    "checked" | "disabled" | "onChange" | "type"
+    "checked" | "disabled" | "onChange" | "onClick" | "type"
   >
 > & { "aria-checked": MixedOrBool };
 
@@ -293,8 +294,13 @@ export function useMixedCheckbox(
   args?: MixedCheckboxArgs,
   functionOrComponentName: string = "useMixedCheckbox"
 ): [UseMixedCheckboxProps, { checked: MixedOrBool }] {
-  let { checked: controlledChecked, defaultChecked, disabled, onChange } =
-    args || {};
+  let {
+    checked: controlledChecked,
+    defaultChecked,
+    disabled,
+    onChange,
+    onClick
+  } = args || {};
 
   let isControlled = controlledChecked != null;
 
@@ -319,6 +325,7 @@ export function useMixedCheckbox(
     checked: stateValueToChecked(current.value),
     disabled: !!disabled,
     onChange: wrapEvent(onChange, handleChange),
+    onClick: wrapEvent(onClick, handleClick),
     type: "checkbox"
   };
 
@@ -338,6 +345,25 @@ export function useMixedCheckbox(
     }
   }
 
+  function handleClick() {
+    /*
+     * A controlled checkbox will have a checked="mixed" prop, but the
+     * underlying input element will receive checked="false" and
+     * aria-checked="mixed". A user click will change the underlying
+     * element's indeterminate property back to false even if the state
+     * doesn't change. This does not trigger a re-render, so our check won't
+     * work in a normal effect. We'll check again on every user click and
+     * update the node imperatively if the state doesn't change.
+     */
+    syncDomNodeWithState();
+  }
+
+  function syncDomNodeWithState() {
+    if (ref.current) {
+      ref.current.indeterminate = current.value === MixedCheckboxStates.Mixed;
+    }
+  }
+
   useEffect(() => {
     if (__DEV__ && !ref.current) {
       throw new Error(
@@ -355,12 +381,8 @@ export function useMixedCheckbox(
     }
   }, [isControlled, controlledChecked, send]);
 
-  // Prevent flashing before mixed marker is displayed
-  useIsomorphicLayoutEffect(() => {
-    if (ref.current) {
-      ref.current.indeterminate = current.value === MixedCheckboxStates.Mixed;
-    }
-  }, [current.value]);
+  // Prevent flashing before mixed marker is displayed and check on every render
+  useIsomorphicLayoutEffect(syncDomNodeWithState);
 
   useEffect(() => {
     send({