fix useFocusRing perf

[sfc-gh-tkojima]

Closes

While investigating a customer reported perf issue with CPU profile attached in our Workspaces product (a web based IDE), we noticed that useFocusVisibleListener was eating up a lot of cpu cycles while users were editing code.

This is due to the fact that we add a function to changeHandlers for each focusable component and that fn runs 2 times for every key press (once on keydown and again on keyup), in extreme scenarios we can have 200+ handlers attached.

This PR updates useFocusVisibleListener to take an enabled opt, so useFocusRing only adds the handler if the component is currently focused.

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

[snowystinger]

This is due to the fact that we install a listener for each focusable component and that listener runs 2 times for every key press (once on keydown and again on keyup), in extreme scenarios we can have 200+ listeners attached.

I assume this means our keyboard and focus listeners? We only attach one of each listener and use a subscription to update anything which calls useFocusVisible. So there shouldn't be 200+ listeners attached.

If it's a listener that you are adding, could you follow the same model as us and use a subscription model?

Otherwise, maybe an example of how this is all interacting would be useful, in a codesandbox or stackblitz. Or some tests or stories which show that many listeners are being added.

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

Line 337 in 245a703

setupGlobalFocusEvents();

This calls a singleton manager function which attaches one of each listener

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

Line 347 in 245a703

changeHandlers.add(handler);

This adds a subscription to the manager

[sfc-gh-tkojima]

Handler === listener here, I updated my PR description, we are adding 200 items to changeHandlers.

The important thing is we are executing 200 functions in a very hot code path, 2x for every keystroke and each one of those invocations runs 4 identical instanceof checks, it's very inefficient.

[sfc-gh-tkojima]

I can setup a repro, but you can tell from the code pretty easily that it will add a handler unconditionally for each component instance that calls useFocusRing().

The changes in this PR are pretty reasonable I feel and all the existing tests pass (useFocusRing code paths have good test coverage). Is there concern with the change?

[sfc-gh-tkojima]

Here is the repro:

https://codesandbox.io/p/sandbox/vy3rxw?file=%2Fsrc%2FApp.js%3A15%2C1

If you add a breakpoint, you can see that changeHandlers has 1001 items in it (one for each focusable component instance).

It should be noted that we were not ourselves able to repro the same perf issues the customer was reporting but this is objectively a lot of unnecessary overhead that can be optimized regardless.

[snowystinger]

I see now, thanks, got a little hung up on the adding listeners, but it was adding subscriptions.

It looks like we got close to this a while back #1516

I'm trying to determine why we didn't do this previously when it was partially suggested. Could be as simple though as not having a use case that extreme at the time.

[sfc-gh-tkojima]

@snowystinger anything else we need to do to get this merged?

[snowystinger]

Yep, a second review, thanks for your patience