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

Improve performance of tabbing #967

Merged

Conversation

camchenry
Copy link
Contributor

@camchenry camchenry commented Jun 7, 2022

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 calling getComputedStyle repeatedly. Even though getComputedStyle only takes a few milliseconds to complete, this time can add up for tests that have many calls to userEvent.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 to tab. 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:
Screen Shot 2022-06-07 at 3 58 20 PM
After:
Screen Shot 2022-06-07 at 3 59 01 PM

This results in a roughly ~10x (-90%) improvement in the peformance of userEvent.tab.

Benchmark Code

How:

  • Added a new (internal) utility function to check the focusable state of an element, with an option to skip checking visibility
  • Changed getTabDestination to use new focusable utility function and check focusability in a loop until we find the next focusable element in the tab order
  • Fixes elements with a tabindex of less than -1 by checking for negative tabindex, rather than just -1

Checklist:

  • [N/A] Documentation
  • Tests
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 7, 2022

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:

Sandbox Source
userEvent-dom Configuration
userEvent-react Configuration

@ph-fritsche
Copy link
Member

Thanks for contributing to this project. ❤️

Avoiding unnecessary calls to getComputedStyle is a great performance improvement.
It doesn't matter much in the browser where this call is fast, but it is notoriously slow in DOM implementation written in JS (e.g. Jsdom).

I've opened a PR simplifying the implementation: camchenry#1

@camchenry camchenry changed the title Improve performance of tabbing by ~10x Improve performance of tabbing Jun 8, 2022
@camchenry
Copy link
Contributor Author

Thanks for contributing to this project. ❤️

Avoiding unnecessary calls to getComputedStyle is a great performance improvement. It doesn't matter much in the browser where this call is fast, but it is notoriously slow in DOM implementation written in JS (e.g. Jsdom).

I've opened a PR simplifying the implementation: camchenry#1

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
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #967 (e732ee9) into main (9dd3985) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #967   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           85        85           
  Lines         1820      1825    +5     
  Branches       667       669    +2     
=========================================
+ Hits          1820      1825    +5     
Impacted Files Coverage Δ
src/utils/focus/getTabDestination.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dd3985...e732ee9. Read the comment docs.

@camchenry
Copy link
Contributor Author

👋 @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.

@ph-fritsche
Copy link
Member

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):
If a checked radio is hidden, the group is ignored.

// For radio groups keep only the active radio
// If there is no active radio, keep only the checked radio
// If there is no checked radio, treat like everything else

@ph-fritsche ph-fritsche merged commit d2d8a39 into testing-library:main Jun 17, 2022
@ph-fritsche
Copy link
Member

@all-contributors add @camchenry code

@allcontributors
Copy link
Contributor

@ph-fritsche

I've put up a pull request to add @camchenry! 🎉

@github-actions
Copy link

🎉 This PR is included in version 14.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@camchenry camchenry deleted the pr/improve-tab-performance branch June 17, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants