feat: Prioritize accessible names check higher than inaccessibility check in byRole queries

[kevin940726]

What:

Related to #698 and #820.

*ByRole queries are filtering inaccessibility (hidden: false) earlier than accessible names. This PR changes that by prioritizing accessible names over inaccessibility. Hopefully to achieve better performance in most cases.

Why:

As stated in #820 (comment), one of the escape hatches to improve the performance of the *ByRole queries is to pass hidden: true to the options. The reason is that the visibility check is more expensive in most cases. People are already using this technique to improve their queries' performance by setting the default options to true globally. The downside of this technique is obviously that we'd lose the visibility check for better and predictable queries.

What if instead of removing the inaccessibility check altogether, we just have to move the order lower? Since the query is really just a set of filter functions, if we can run the faster filter first and short-circuiting most of the candidates, then hopefully we don't have to run the slower inaccessibility check on so many elements.

For a naive test with 1000 buttons all with different names in jsdom environment, selecting the last element takes around 2 seconds. After applying this changes, it only takes around 1 second in average on my machine. It's not a lot but seems like a free win to me.

const {getByRole} = render(
  Array.from({length: 1000})
    .map((_, index) => `<button>${index}</button>`)
    .join(''),
)
expect(getByRole('button', {name: '999'})).not.toBeNull()

Obviously it doesn't reflect the real world usage, but I'd expect the usage of accessible names check are more frequent than the inaccessibility check. It'd only be slower if there are multiple elements with the same name but only a part of them are accessible/visible. I can't think of a case where this would be a common scenario in testing. Additionally, people are already opting-out of the inaccessibility check globally, this should have little impact for them.

Preferably, it'd be nice to have it tested in some real world repos to see if it actually improves the performance though.

How:

Move the inaccessibility check lower to the last.

Checklist:

• Documentation added to the
  docs site (I don't think this needs to be added to the documentation site. A record in changelog should be fine?)
• Tests (N/A)
• TypeScript definitions updated (N/A)
• Ready to be merged
  

[codecov]

Codecov Report

Merging #1068 (f7ffeac) into main (b6b9b5b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1068   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          952       952           
  Branches       311       311           
=========================================
  Hits           952       952           

Flag	Coverage Δ
node-12	100.00% <100.00%> (ø)
node-14	100.00% <100.00%> (ø)
node-16.9.1	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/queries/role.js	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 b6b9b5b...f7ffeac. Read the comment docs.

[codesandbox-ci]

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 f7ffeac:

Sandbox	Source
react-testing-library-examples	Configuration

[eps1lon]

Thanks for this work. Taking this change for a spin in mui/material-ui#29368

The reason is that the visibility check is more expensive in most cases.

Could you elaborate a bit more on this? Did you assume this from usage in some repository? Did you notice this during profiling? In which cases is it more expensive?

It's not at all clear to me that visibility checks are more expensive since the accessible name can potentially compute styles for more elements. So this really needs a data-driven approach to determine the best heuristic.

[kevin940726]

Could you elaborate a bit more on this? Did you assume this from usage in some repository? Did you notice this during profiling? In which cases is it more expensive?

It's not at all clear to me that visibility checks are more expensive since the accessible name can potentially compute styles for more elements. So this really needs a data-driven approach to determine the best heuristic.

Fair point, I don't have the data though, I kinda hope to have some folks testing this out and gather some real life performance data in some of the popular repos 😅 (material-ui for instance). Hence this line in the PR description above:

Preferably, it'd be nice to have it tested in some real world repos to see if it actually improves the performance though.

However, the established workaround of the original issue could more or less back this up though. We're already suggesting users to try disabling the visibility check altogether to speed up the performance, that could only mean that the check itself is less important and costly, no? I understand that with the manual opt-out, the users got to choose whether to disable it or not though.

In addition, I'd assume that most of the *ByRole query usages out there are simply just role + name. For instance, for querying a button with "Click me" text:

getByRole('button', { name: 'Click me' });

The two major inputs from the user here are, obviously, role and name. While we do perform visibility check by default, it's more implicit than these two explicit checks. With that in mind, I'd assume that these two checks will filter out most of elements that don't match, leaving us with a list of elements very close to the final results. Finally, we can perform the more implicit visibility check to further narrow down the results.

To quote your study here:

To touch on another potential optimization point: In Material-UI we set the default value to true (fast option) in local development and to false (slow) in CI. The reason being that you rarely accidentally query for inaccessible elements. If you do, it should be obvious in the test which we'll check during code review (since an explicit hidden option stands out). In 1015 ByRole calls we only ever need to explicitly specify the hidden state in 25 cases. So in the vast majority of cases we just check the hidden state for confidence not because the test is actually about the hidden state.

If I understand this correctly, it means that the hidden state rarely makes a difference to the final results, which means that the filter function isn't that useful when it's placed early especially if it is expensive. On the other hand, even though the "name computation" check is also expensive, it's also essential to the query. While the fewer times we need to run it the better, we shouldn't run a potentially more expensive check earlier to do that.

But again, it's all just my guess. I'd love to have some real world data to support the theory 🙇‍♂️ .

[eps1lon]

Thanks for the explanation. This makes more sense to me now.

Did some analysis in mui/material-ui#29368. Looks safe to land.

[kevin940726]

Any plans to merge this? Or should we get more approvals from maintainers?

[eps1lon]

Just wanted to let this sit so that everybody has a chance to review

[timdeschryver]

This LGTM.
Although I agree with @markerikson that it could be made faster (even if it's just a little bit) if all filter statements are merged into 1 statement.

[github-actions]

🎉 This PR is included in version 8.11.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[markerikson]

Just as an FYI, I tried specifically adding a dep on @testing-library/dom to bump us to 8.11, and ran our longest UI test file, an "integration"-style test using next-page-tester.

Before:

 PASS  client/features/programDetails/characteristicsTab/tests/Characteristics.tests.tsx (107.118s)
  ProgramCharacteristicsContent
    √ Renders and allows editing Program Characteristics (98432ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        109.2s, estimated 124s

After:

 PASS  client/features/programDetails/characteristicsTab/tests/Characteristics.tests.tsx (90.958s)
  ProgramCharacteristicsContent
    √ Renders and allows editing Program Characteristics (82187ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        93.06s, estimated 106s

No other code changes.

So, this distinctly made some improvement.

(I still don't think the test file should be taking anywhere this long to begin with, but I haven't had time to really drill down and see what else is taking it so long.)