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
Improve performance of tabbing #967
Improve performance of tabbing #967
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e732ee9:
|
Thanks for contributing to this project. ❤️ Avoiding unnecessary calls to I've opened a PR simplifying the implementation: camchenry#1 |
PR testing-library#967 code suggestions
Thank you for the suggestions! I have gone ahead and merged those in this branch. I noticed that CI was failing previously due to a typecheck issue, which I'm not sure what that was about. |
Codecov Report
@@ Coverage Diff @@
## main #967 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 85 85
Lines 1820 1825 +5
Branches 667 669 +2
=========================================
+ Hits 1820 1825 +5
Continue to review full report at Codecov.
|
👋 @ph-fritsche Is there anything else that I can do to help get this merged? I'm more than happy to help contribute any additional changes or tests as needed. |
Sorry about the delay. Wanted to dispel one minor concern: Feeding our logic for radio groups with hidden elements does in fact match what is observed in the browser (Chrome): user-event/src/utils/focus/getTabDestination.ts Lines 42 to 44 in e732ee9
|
@all-contributors add @camchenry code |
I've put up a pull request to add @camchenry! 🎉 |
🎉 This PR is included in version 14.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
This PR changes the implementation of
getTabDestination
to only calculate the visibility of an element "just in time," rather than calculating the visibility of all elements ahead of time.(As an additional change, this also fixes elements with a
tabindex
less than-1
from being focusable.)Why:
Currently,
getTabDestination
computes the visibility of all elements ahead of time, which can be a costly operation due to callinggetComputedStyle
repeatedly. Even thoughgetComputedStyle
only takes a few milliseconds to complete, this time can add up for tests that have many calls touserEvent.tab()
or lots of focusable DOM elements.Computing the visibility only for adjacent elements in the tab order saves many calls to
getComputedStyle
which results in dramatically improved performance for calls totab
. Assuming there few hidden elements in the tab order, then this makes the visibility run in constant time rather than linear time in the number of focusable elements.Here is a rough benchmark with just tabbing between a few elements with 100 focusable buttons in the DOM.
Before:
After:
This results in a roughly ~10x (-90%) improvement in the peformance of
userEvent.tab
.Benchmark Code
How:
getTabDestination
to use new focusable utility function and check focusability in a loop until we find the next focusable element in the tab ordertabindex
of less than-1
by checking for negativetabindex
, rather than just-1
Checklist: