Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Menu] Each Menu adds its own document event listeners for detecting keyboard state #1239

Closed
AlabasterAxe opened this issue Mar 12, 2022 · 10 comments · May be fixed by #3172
Closed

[Menu] Each Menu adds its own document event listeners for detecting keyboard state #1239

AlabasterAxe opened this issue Mar 12, 2022 · 10 comments · May be fixed by #3172

Comments

@AlabasterAxe
Copy link

AlabasterAxe commented Mar 12, 2022

Bug report

This is basically a reopening of #1139 since we're still seeing considerable slow down even after updating past the fix.

Current Behavior

Radix Dropdown Menus and Context Menus each add their own document event listeners for keydown and pointerdown which can cause a slowdown in the handling of the corresponding events

Expected behavior

Handling of keydown and pointerdown don't slowdown as a result of additional menu components on the page.

Reproducible example

Reproduction is to add n menus to a react app and observe that there are n document event listeners for either keydown or pointerdown

My Profile demonstrating consistent slowness with handling mousedown events:
Profile-20220312T073820.json.zip

And here's a screenshot from the above profile demonstrating a couple of examples of keydown events that take over a second to handle:

Screen Shot 2022-03-12 at 8 35 19 AM

Suggested solution

Provide a global keyboard state context which registers a single document event listener and all menus can share.

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-context-menu, @radix-ui/react-dropdown-menu 0.1.5
React n/a 16.13
Browser Chrome 99
Assistive tech n/a n/a
Node n/a v16.14.0
npm/yarn yarn 1.22
Operating System MacOS 12.2.1
@AlabasterAxe
Copy link
Author

@benoitgrelard @jjenzz Just pinging this. This is causing serious overhead in our app. It would be great if we could have a solution in Radix that didn't incur a performance penalty for each additional menu. Thanks!

@hipstersmoothie
Copy link

Looking at the code it seems any component using the following components could add N (N = number of components) listeners to the app causing it to be slow once the app gets large enough:

  • DismissableLayer: will add N pointerDown, focusin events
  • FocusScope: will add N focusin, focusout events
  • DismissableLayer: will add N pointerDown, pointer move, keydown events
  • RadioGroup: will add N keydown, keyup events
  • useEscapeKeydown: will add N keydown events

@jjenzz
Copy link
Contributor

jjenzz commented Mar 16, 2022

@hipstersmoothie many of the components listed would only add events when the content parts are open so unlikely to be an issue unless you have 1000s of dialogs open at the same time (for example).

the issue with Menu is specifically because it is adding events in the Root part which will exist for all instances on the page (not just open content). the only one from your list that i can see is likely to suffer the same issues is RadioGroup.

@jjenzz
Copy link
Contributor

jjenzz commented Mar 16, 2022

@AlabasterAxe how many menus do you have rendered on your page? i'm not working at modulz anymore so i cannot help with a timeline for a solution to this i'm afraid but i'm surprised it is causing such a significant slow down.

you can see up to 500 menus (that also open dialogs) here performing fine: https://codesandbox.io/s/radix-performance-demo-forked-o6q977?file=/src/App.js

@AlabasterAxe
Copy link
Author

Nice thanks for the pointer! taking a look. I agree that example performs very well. Not sure why performance would be so different.

In the cases I've been looking at it was ~700.

@AlabasterAxe
Copy link
Author

Your page does reproduce the same issue:
Screen Shot 2022-03-16 at 7 01 14 PM

The top-level task took 42 ms overall and 40ms of which is just handling the keydown event listeners. It's possible that if there's more going on on the page that these event handlers are slower to process? 🤷

@hipstersmoothie
Copy link

Yeah this is easily reproducible with the given example. Here I upped the menus to 1000 and each keydown takes 140ms

CleanShot 2022-03-16 at 16 23 56

Even downing it to 700 (matching our usage) see 80ms delays

CleanShot 2022-03-16 at 16 25 02

This PR I opened does fix the issue though. 🙏ing we can get a merge

@jjenzz
Copy link
Contributor

jjenzz commented Mar 18, 2022

Here I upped the menus to 1000 and each keydown takes 140ms

while i agree it might be nice to reduce these events (to prevent the overhead of these reports if anything), if you're experiencing a performance issue from them i would think you are rendering far too many menus at once.

personally i would avoid rendering 500 menus let alone 1000 since it is unlikely to be possible for a human to interact with that many menus in a single viewport, so react will be redundantly managing those trees.

using something like IntersectionObserver/virtual scroll to render only the menus in the user's viewport will help your performance all round.

@AlabasterAxe
Copy link
Author

@jjenzz 100% agree, there's no reason we need to render many of those components since most of them are off-screen. I'm planning on looking into this soon.

At the same time, I think it's reasonable for these components to defensive about potential misuse, especially since in this case it seems like there are reasonable ways to avoid registering one listener per menu.

@benoitgrelard benoitgrelard changed the title Each Menu adds its own document event listeners for detecting keyboard state [DropdownMenu] Each Menu adds its own document event listeners for detecting keyboard state Mar 31, 2022
@benoitgrelard benoitgrelard changed the title [DropdownMenu] Each Menu adds its own document event listeners for detecting keyboard state [Menu] Each Menu adds its own document event listeners for detecting keyboard state Mar 31, 2022
@benoitgrelard
Copy link
Collaborator

Hi @AlabasterAxe,

At the same time, I think it's reasonable for these components to defensive about potential misuse, especially since in this case it seems like there are reasonable ways to avoid registering one listener per menu.

Whilst this might be nice, it is clear that a lot more can be done user-side/app-side to ensure better performance.
Additionally, we have already reduced the numbers of listeners for each menu instance from 3 to 1 (#1140).

We have to prioritise where we focus our time so I will close this issue for now as it is unlikely we will do more in this case.

✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants