fix useFocusRing perf (#9456)

* fix useFocusRing perf

* Add test for changes

* fix tests for react 17

---------

Co-authored-by: Robert Snow <[email protected]>

Author: sfc-gh-tkojima

packages/@react-aria/focus/src/useFocusRing.ts

@@ -51,14 +51,15 @@ export function useFocusRing(props: AriaFocusRingProps = {}): FocusRingAria {
 
   let onFocusChange = useCallback(isFocused => {
     state.current.isFocused = isFocused;
+    state.current.isFocusVisible = isFocusVisible();
     setFocused(isFocused);
     updateState();
   }, [updateState]);
 
   useFocusVisibleListener((isFocusVisible) => {
     state.current.isFocusVisible = isFocusVisible;
     updateState();
-  }, [], {isTextInput});
+  }, [isTextInput, isFocused], {enabled: isFocused, isTextInput});
 
   let {focusProps} = useFocus({
     isDisabled: within,

packages/@react-aria/interactions/src/useFocusVisible.ts

@@ -39,7 +39,7 @@ export interface FocusVisibleResult {
 
 let currentModality: null | Modality = null;
 let currentPointerType: PointerType = 'keyboard';
-let changeHandlers = new Set<Handler>();
+export const changeHandlers = new Set<Handler>();
 interface GlobalListenerData {
   focus: () => void
 }
@@ -333,10 +333,13 @@ export function useFocusVisible(props: FocusVisibleProps = {}): FocusVisibleResu
 /**
  * Listens for trigger change and reports if focus is visible (i.e., modality is not pointer).
  */
-export function useFocusVisibleListener(fn: FocusVisibleHandler, deps: ReadonlyArray<any>, opts?: {isTextInput?: boolean}): void {
+export function useFocusVisibleListener(fn: FocusVisibleHandler, deps: ReadonlyArray<any>, opts?: {enabled?: boolean, isTextInput?: boolean}): void {
   setupGlobalFocusEvents();
 
   useEffect(() => {
+    if (opts?.enabled === false) {
+      return;
+    }
     let handler = (modality: Modality, e: HandlerEvent) => {
       // We want to early return for any keyboard events that occur inside text inputs EXCEPT for Tab and Escape
       if (!isKeyboardFocusEvent(!!(opts?.isTextInput), modality, e)) {
@@ -351,3 +354,4 @@ export function useFocusVisibleListener(fn: FocusVisibleHandler, deps: ReadonlyA
   // eslint-disable-next-line react-hooks/exhaustive-deps
   }, deps);
 }
+

packages/@react-aria/interactions/test/useFocusVisible.test.js

@@ -11,7 +11,7 @@
  */
 import {act, fireEvent, pointerMap, render, renderHook, screen, waitFor} from '@react-spectrum/test-utils-internal';
 import {addWindowFocusTracking, useFocusVisible, useFocusVisibleListener} from '../';
-import {hasSetupGlobalListeners} from '../src/useFocusVisible';
+import {changeHandlers, hasSetupGlobalListeners} from '../src/useFocusVisible';
 import {mergeProps} from '@react-aria/utils';
 import React from 'react';
 import {useButton} from '@react-aria/button';
@@ -65,7 +65,7 @@ describe('useFocusVisible', function () {
   beforeAll(() => {
     user = userEvent.setup({delay: null, pointerMap});
   });
-  
+
   beforeEach(() => {
     fireEvent.focus(document.body);
   });
@@ -84,7 +84,7 @@ describe('useFocusVisible', function () {
     render(<Example />);
     await user.tab();
     let el = screen.getByText('example-focusVisible');
-    
+
     await user.click(el);
     toggleBrowserTabs();
 
@@ -165,7 +165,7 @@ describe('useFocusVisible', function () {
 
       await user.click(el);
       expect(el.textContent).toBe('example');
-      
+
       // Focus events after beforeunload no longer work
       fireEvent(iframe.contentWindow, new Event('beforeunload'));
       await user.keyboard('{Enter}');
@@ -355,4 +355,102 @@ describe('useFocusVisibleListener', function () {
     expect(fnMock).toHaveBeenCalledTimes(3);
     expect(fnMock.mock.calls).toEqual([[true], [true], [false]]);
   });
+
+  describe('subscription model', function () {
+    let user;
+    beforeAll(() => {
+      user = userEvent.setup({delay: null, pointerMap});
+    });
+
+    function Example(props) {
+      return (
+        <div>
+          <ButtonExample data-testid="button1" />
+          <ButtonExample data-testid="button2" />
+        </div>
+      );
+    }
+
+    function ButtonExample(props) {
+      const ref = React.useRef(null);
+      const {buttonProps} = useButton({}, ref);
+      const {focusProps, isFocusVisible} = useFocusRing();
+
+      return <button {...mergeProps(props, buttonProps, focusProps)} data-focus-visible={isFocusVisible || undefined} ref={ref}>example</button>;
+    }
+    it('does not call changeHandlers when unneeded', async function () {
+      // Save original methods
+      const originalAdd = changeHandlers.add.bind(changeHandlers);
+      const originalDelete = changeHandlers.delete.bind(changeHandlers);
+      // Map so we can also track references to the original handlers to remove them later
+      const handlerSpies = new Map();
+
+      // Intercept handler registration and wrap with spy
+      changeHandlers.add = function (handler) {
+        const spy = jest.fn(handler);
+        handlerSpies.set(handler, spy);
+        return originalAdd.call(this, spy);
+      };
+
+      changeHandlers.delete = function (handler) {
+        const spy = handlerSpies.get(handler);
+        if (spy) {
+          handlerSpies.delete(handler);
+          return originalDelete.call(this, spy);
+        }
+        return originalDelete.call(this, handler);
+      };
+
+      // Possibly a little extra cautious with the unmount, but better safe than sorry with cleanup.
+      let {getByTestId, unmount} = render(<Example />);
+
+      let button1 = getByTestId('button1');
+      let button2 = getByTestId('button2');
+      expect(button1).not.toHaveAttribute('data-focus-visible');
+      expect(button2).not.toHaveAttribute('data-focus-visible');
+      // No handlers registered yet because nothing is focused
+      expect(handlerSpies.size).toBe(0);
+
+      // Tab to first button, this should add its handler
+      await user.tab();
+      expect(document.activeElement).toBe(button1);
+      expect(button1).toHaveAttribute('data-focus-visible');
+      expect(button2).not.toHaveAttribute('data-focus-visible');
+
+      expect(handlerSpies.size).toBe(1);
+      let [button1Spy] = [...handlerSpies.values()];
+
+      button1Spy.mockClear();
+
+      // Tab to second button - first handler should be removed, second added
+      await user.tab();
+      expect(button1).not.toHaveAttribute('data-focus-visible');
+      expect(button2).toHaveAttribute('data-focus-visible');
+
+      // Still only 1 handler registered (swapped from button1 to button2)
+      expect(handlerSpies.size).toBe(1);
+      let [button2Spy] = [...handlerSpies.values()];
+      expect(button2Spy).not.toBe(button1Spy); // Should be a different spy
+
+      // button1's handler was called during tab (keydown/keyup before removal)
+      // the handler is removed later in an effect
+      expect(button1Spy.mock.calls.length).toBeGreaterThan(0);
+
+      button1Spy.mockClear();
+      button2Spy.mockClear();
+
+      // After button1's handler is removed, it should NOT be called
+      // for subsequent modality changes - only button2's handler should be called
+      await user.click(button2);
+      expect(button1).not.toHaveAttribute('data-focus-visible');
+      expect(button2).not.toHaveAttribute('data-focus-visible');
+      expect(button1Spy).toHaveBeenCalledTimes(0); // button1's handler should NOT be called
+      expect(button2Spy).toHaveBeenCalledTimes(1); // Only button2's handler called to change modality to pointer
+
+      // Cleanup
+      unmount();
+      changeHandlers.add = originalAdd;
+      changeHandlers.delete = originalDelete;
+    });
+  });
 });