Update useFocusEvents disabled behavior

jamesplease · Jun 17, 2021 · 3085b02 · 3085b02
1 parent f783ae2
commit 3085b02
Show file tree

Hide file tree

Showing 2 changed files with 178 additions and 56 deletions.
diff --git a/src/hooks/use-focus-events.test.js b/src/hooks/use-focus-events.test.js
@@ -69,65 +69,153 @@ describe('useFocusEvents', () => {
     });
   });
 
-  it('disabled/enabled (enabled by default)', () => {
-    const nodeAOnDisabled = jest.fn();
-    const nodeAOnEnabled = jest.fn();
-    const nodeBOnDisabled = jest.fn();
-    const nodeBOnEnabled = jest.fn();
-    let focusStore;
-    let updateDisableA;
-
-    function TestComponent() {
-      const [disableA, setDisableA] = useState(false);
-      focusStore = useFocusStoreDangerously();
-
-      updateDisableA = setDisableA;
-
-      useFocusEvents('nodeA', {
-        disabled: nodeAOnDisabled,
-        enabled: nodeAOnEnabled,
-      });
+  describe('disabled/enabled', () => {
+    it('works when enabled on mount', () => {
+      const nodeAOnDisabled = jest.fn();
+      const nodeAOnEnabled = jest.fn();
+      const nodeBOnDisabled = jest.fn();
+      const nodeBOnEnabled = jest.fn();
+      let focusStore;
+      let updateDisableA;
 
-      useFocusEvents('nodeB', {
-        disabled: nodeBOnDisabled,
-        enabled: nodeBOnEnabled,
-      });
+      function TestComponent() {
+        const [disableA, setDisableA] = useState(false);
+        focusStore = useFocusStoreDangerously();
+
+        updateDisableA = setDisableA;
+
+        useFocusEvents('nodeA', {
+          disabled: nodeAOnDisabled,
+          enabled: nodeAOnEnabled,
+        });
+
+        useFocusEvents('nodeB', {
+          disabled: nodeBOnDisabled,
+          enabled: nodeBOnEnabled,
+        });
+
+        return (
+          <>
+            <FocusNode
+              focusId="nodeA"
+              data-testid="nodeA"
+              disabled={disableA}
+            />
+            <FocusNode focusId="nodeB" data-testid="nodeB" />
+          </>
+        );
+      }
+
+      render(
+        <FocusRoot>
+          <TestComponent />
+        </FocusRoot>
+      );
+
+      let focusState = focusStore.getState();
+      expect(focusState.focusedNodeId).toEqual('nodeA');
+      expect(focusState.focusHierarchy).toEqual(['root', 'nodeA']);
+
+      expect(nodeAOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeAOnEnabled.mock.calls.length).toBe(0);
+      expect(nodeBOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+
+      act(() => updateDisableA(true));
+
+      focusState = focusStore.getState();
+      expect(focusState.focusedNodeId).toEqual('nodeB');
+      expect(focusState.focusHierarchy).toEqual(['root', 'nodeB']);
+
+      expect(nodeAOnDisabled.mock.calls.length).toBe(1);
+      expect(nodeAOnEnabled.mock.calls.length).toBe(0);
+      expect(nodeBOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+
+      act(() => updateDisableA(false));
+
+      focusState = focusStore.getState();
+      expect(focusState.focusedNodeId).toEqual('nodeB');
+      expect(focusState.focusHierarchy).toEqual(['root', 'nodeB']);
+
+      expect(nodeAOnDisabled.mock.calls.length).toBe(1);
+      expect(nodeAOnEnabled.mock.calls.length).toBe(1);
+      expect(nodeBOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+    });
+
+    it('works when disabled on mount', () => {
+      const nodeAOnDisabled = jest.fn();
+      const nodeAOnEnabled = jest.fn();
+      const nodeBOnDisabled = jest.fn();
+      const nodeBOnEnabled = jest.fn();
+      let focusStore;
+      let updateDisableA;
+
+      function TestComponent() {
+        const [disableA, setDisableA] = useState(true);
+        focusStore = useFocusStoreDangerously();
+
+        updateDisableA = setDisableA;
+
+        useFocusEvents('nodeCCC', {
+          disabled: nodeAOnDisabled,
+          enabled: nodeAOnEnabled,
+        });
+
+        useFocusEvents('nodeB', {
+          disabled: nodeBOnDisabled,
+          enabled: nodeBOnEnabled,
+        });
+
+        return (
+          <>
+            <FocusNode
+              focusId="nodeCCC"
+              data-testid="nodeA"
+              disabled={disableA}
+            />
+            <FocusNode focusId="nodeB" data-testid="nodeB" />
+          </>
+        );
+      }
 
-      return (
-        <>
-          <FocusNode focusId="nodeA" data-testid="nodeA" disabled={disableA} />
-          <FocusNode focusId="nodeB" data-testid="nodeB" />
-        </>
+      render(
+        <FocusRoot>
+          <TestComponent />
+        </FocusRoot>
       );
-    }
 
-    render(
-      <FocusRoot>
-        <TestComponent />
-      </FocusRoot>
-    );
+      let focusState = focusStore.getState();
+      expect(focusState.focusedNodeId).toEqual('nodeB');
+      expect(focusState.focusHierarchy).toEqual(['root', 'nodeB']);
 
-    let focusState = focusStore.getState();
-    expect(focusState.focusedNodeId).toEqual('nodeA');
-    expect(focusState.focusHierarchy).toEqual(['root', 'nodeA']);
+      expect(nodeAOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeAOnEnabled.mock.calls.length).toBe(0);
+      expect(nodeBOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeBOnEnabled.mock.calls.length).toBe(0);
 
-    expect(nodeAOnDisabled.mock.calls.length).toBe(0);
-    expect(nodeAOnEnabled.mock.calls.length).toBe(0);
-    expect(nodeBOnDisabled.mock.calls.length).toBe(0);
-    expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+      act(() => updateDisableA(false));
 
-    act(() => updateDisableA(true));
+      focusState = focusStore.getState();
+      expect(focusState.focusedNodeId).toEqual('nodeB');
+      expect(focusState.focusHierarchy).toEqual(['root', 'nodeB']);
 
-    expect(nodeAOnDisabled.mock.calls.length).toBe(1);
-    expect(nodeAOnEnabled.mock.calls.length).toBe(0);
-    expect(nodeBOnDisabled.mock.calls.length).toBe(0);
-    expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+      expect(nodeAOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeAOnEnabled.mock.calls.length).toBe(1);
+      expect(nodeBOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeBOnEnabled.mock.calls.length).toBe(0);
 
-    act(() => updateDisableA(false));
+      act(() => updateDisableA(true));
 
-    expect(nodeAOnDisabled.mock.calls.length).toBe(1);
-    expect(nodeAOnEnabled.mock.calls.length).toBe(1);
-    expect(nodeBOnDisabled.mock.calls.length).toBe(0);
-    expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+      focusState = focusStore.getState();
+      expect(focusState.focusedNodeId).toEqual('nodeB');
+      expect(focusState.focusHierarchy).toEqual(['root', 'nodeB']);
+
+      expect(nodeAOnDisabled.mock.calls.length).toBe(1);
+      expect(nodeAOnEnabled.mock.calls.length).toBe(1);
+      expect(nodeBOnDisabled.mock.calls.length).toBe(0);
+      expect(nodeBOnEnabled.mock.calls.length).toBe(0);
+    });
   });
 });
diff --git a/src/hooks/use-focus-events.ts b/src/hooks/use-focus-events.ts
@@ -1,6 +1,7 @@
-import { useRef } from 'react';
+import { useRef, useState } from 'react';
 import useOnChange from './internal/use-on-change';
 import useFocusNode from './use-focus-node';
+import warning from '../utils/warning';
 import { Id, Node } from '../types';
 
 type EventCallback = (node: Node) => void;
@@ -17,18 +18,47 @@ interface Events {
 }
 
 export default function useFocusEvents(nodeId: Id, events: Events = {}): void {
-  const node = useFocusNode(nodeId);
+  const [constantNodeId] = useState(nodeId);
+
+  const node = useFocusNode(constantNodeId);
   const nodeRef = useRef(node);
   nodeRef.current = node;
 
   const eventsRef = useRef(events);
   eventsRef.current = events;
 
+  // This pattern allows the `focus` hook to be called even on mount.
+  // When the node doesn't exist, we set this to false. Then, when the initial
+  // mounting is done, `node.isFocused` is true, and the callbacks fire.
   const isFocused = Boolean(node && node.isFocused);
-  const isDisabled = Boolean(node && node.disabled);
+
+  // This ensures that the enabled/disabled hooks are *not* called on mount.
+  // This way, `disabled/enabled` are only called when that state actually changes
+  let isDisabled;
+  if (!node) {
+    isDisabled = null;
+  } else {
+    isDisabled = node.disabled;
+  }
+
+  // For active, we also wouldn't want it to be called on mount, but it's not possible
+  // for a node to be mounted as active, so this simpler logic gives us the desired behavior.
   const isActive = Boolean(node && node.active);
 
-  useOnChange(isFocused, currentIsFocused => {
+  useOnChange(nodeId, (currentId, prevId) => {
+    if (typeof prevId !== 'string') {
+      return;
+    }
+
+    if (currentId !== prevId) {
+      warning(
+        `The nodeId passed into useFocusEvents changed. This change has been ignored: the nodeId cannot be changed.`,
+        'FOCUS_EVENTS_NODE_ID_CHANGED'
+      );
+    }
+  });
+
+  useOnChange(isFocused, (currentIsFocused) => {
     if (nodeRef.current) {
       if (currentIsFocused && typeof eventsRef.current.focus === 'function') {
         eventsRef.current.focus(nodeRef.current);
@@ -41,7 +71,11 @@ export default function useFocusEvents(nodeId: Id, events: Events = {}): void {
     }
   });
 
-  useOnChange(isDisabled, currentIsDisabled => {
+  useOnChange(isDisabled, (currentIsDisabled, prevIsDisabled) => {
+    if (prevIsDisabled === undefined || prevIsDisabled == null) {
+      return;
+    }
+
     if (nodeRef.current) {
       if (
         currentIsDisabled &&
@@ -57,7 +91,7 @@ export default function useFocusEvents(nodeId: Id, events: Events = {}): void {
     }
   });
 
-  useOnChange(isActive, currentIsActive => {
+  useOnChange(isActive, (currentIsActive) => {
     if (nodeRef.current) {
       if (currentIsActive && typeof eventsRef.current.active === 'function') {
         eventsRef.current.active(nodeRef.current);