Optimizing `displayCheck: "full"` #604

DaviDevMod · 2022-03-02T22:00:51Z

Optimized displayCheck: "full" by replacing this DOM traversal (executed on every potential focusable element) with the following statement:

return !node.getClientRects().length;

which is a more general check for whether an elements has display boxes. display: "none" is only a special case of !node.getClientRects().length.

Another case of elements with no display boxes would be elements with display: "contents". And (since this case wasn't previosly handled) tests have been added to cover this possibility.

Fixes #592

PR Checklist

Please leave this checklist in your PR.

Source changes maintain stated browser compatibility.
Issue being fixed is referenced.
Unit test coverage added/updated.
E2E test coverage added/updated.
Typings added/updated.
Changes do not break SSR:
- Careful to test typeof document/window !== 'undefined' before using it in code that gets executed on load.
README updated (API changes, instructions, etc.).
Changes to dependencies explained.
Changeset added (run yarn changeset locally to add one, and follow the prompts).
- EXCEPTION: A Changeset is not required if the change does not affect any of the source files that produce the package bundle. For example, demo changes, tooling changes, test updates, or a new dev-only dependency to run tests more efficiently should not have a Changeset since it will not affect package consumers.

changeset-bot · 2022-03-02T22:00:54Z

🦋 Changeset detected

Latest commit: fe27341

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
tabbable	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2022-03-02T22:04:40Z

Codecov Report

Merging #604 (7594e8f) into master (38b2f51) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 7594e8f differs from pull request most recent head fe27341. Consider uploading reports for the commit fe27341 to get more accurate results

@@            Coverage Diff             @@
##           master     #604      +/-   ##
==========================================
- Coverage   97.22%   97.16%   -0.06%     
==========================================
  Files           1        1              
  Lines         144      141       -3     
  Branches       60       59       -1     
==========================================
- Hits          140      137       -3     
  Misses          4        4

Impacted Files	Coverage Δ
src/index.js	`97.16% <100.00%> (-0.06%)`	⬇️

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 38b2f51...fe27341. Read the comment docs.

DaviDevMod · 2022-03-02T22:17:18Z

Not sure why the coverage has decreased. It may be that there are cases other than the display: "full" and display: "contents" ones, in which an element has no display boxes. And we have no tests for these additional cases.

stefcameron · 2022-03-04T19:33:00Z

Not sure why the coverage has decreased. It may be that there are cases other than the display: "full" and display: "contents" ones, in which an element has no display boxes. And we have no tests for these additional cases.

I think the reason it has decreased is because technically, there were 4 lines that were covered before, and now there is only one:

But this is a blind comparison between before (say 300 lines total) and now (say 296 lines total), so technically, coverage has decreased by a fraction, but that's not because there are lines that were covered before and aren't now. It's because there are just less lines covered.

Hopefully that makes sense. Not something to worry about in this case.

stefcameron

This looks good, thank you!

DaviDevMod · 2022-03-04T20:31:13Z

I think the reason it has decreased is because technically, there were 4 lines that were covered before, and now there is only one

Oh so that's how the coverage is calculated.
I don't know what kind of smarter calculation I was expecting. But yes, recognising which lines are covered by tests, is already pretty clever.

Thank you for the clarification!

stefcameron · 2022-03-05T20:03:13Z

@all-contributors add @DaviDevMod for code, test

allcontributors · 2022-03-05T20:03:21Z

@stefcameron

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

TODO: - fix the one failing test (see bunch of DEBUG comments) - optimize/clarify getCandidate... loop (see DEBUG comment) - clarify docs about getShadowRoot() === true that's meant for non-zero test

TODO: Add support for `getShadowRoot: true` instead of function.

* chore(dev): declarative shadow root test fixtures - new development shadow-root-utils - refactor setupFixture to render using new utils - refactor current shadow dom tests to use declarative root - debug page renders shadow root fixtures correctly * feat: separate radio light/shadow dom groups * feat: detect display across shadow boundaries * feat: scan through shadow boundary - iterate down dom instead of query when getShadowRoot is provided - new candidate list/tree format with scoped lists * chore(types): added getShadowDom to types * fix: type of getShadowRoot option * test: add test to locate tabbable host * feat: slot elements are not focusable/tabbable * chore: added some jsdocs * refactor: modernize syntax - as requested in PR * Prepare for 5.3.0-beta.0 * 5.3.0-beta.0 * Adjusting code after #604 and comments * Disable shadow DOM for isFocusable/isTabbable if getShadowRoot not given This goes along with disabling it for `tabbable()` and `focusable()` when the option isn't given. * Clarify getShadowRoot must be set to enable shadow DOM support * Add support for `getShadowRoot: true` Note this is the equivalent of `getShadowRoot: () => false` which simply enables shadow DOM support for all open shadows. * Prepare for v5.3.0-beta.1 * 5.3.0-beta.1 * Add prepublishOnly script for manual publishing * fix(index.js) The tabIndex of audio, video and details was left to the default if set to some NaN (#610) * fix(index.js) The tabIndex of contentEditable elements was assumed to be zero in any case, not only in the case it was not specifically set. * Simplified and optimized 'getTabIndex'. * Made better use of short-circuit evaluation in 'isNodeMatchingSelectorTabbable', reducing the chances to call the computationally expensive 'isNodeMatchingSelectorFocusable'. * (Re)Added 'isScope' parameter to 'getTabIndex'. This parameter wasn't present in the master branch, so I lost it in the rebase process. * Added tests for a `contenteditable` with negative tab index. * Fixed bug, now the getTabIndex can return 0 not only when the tabindex is not explicitly set, but also when is set to a value that gives NaN when parsed as integer (which would have been resulted in the default browser tabIndex, as if the tabindex wasn't set at all). Also added test for the case an element has a tab index that can't be turned into an integer. * Added changeset, added entry in CHANGELOG.md and wrote more tests. * Be consistent with asterisks * Sync package.json/yarn.lock with beta-530 base branch Co-authored-by: Stefan Cameron <stefan@stefcameron.com> * [#632] Add test for radios in a form (#638) Can't repro the issue, but might as well keep the test since we seem to like fieldsets but not forms for some reason. * Add changeset for shadow root support Co-authored-by: Ido Rosenthal <idoros@gmail.com> Co-authored-by: DaviDevMod <98312056+DaviDevMod@users.noreply.github.com>

craigkovatch · 2022-05-05T19:22:57Z

This broke ~all of our unit tests which touch code that uses tabbable, opening an issue...

DaviDevMod added 3 commits March 2, 2022 21:11

perf(index.js): Optimized displayCheck: "full".

7772764

Added tests for display:"contents".

49cf379

Added changeset.

fe27341

DaviDevMod changed the title ~~Perf/display check full~~ Optimizing displayCheck: "full" Mar 2, 2022

DaviDevMod force-pushed the perf/displayCheck-full branch from 46e6d3f to fe27341 Compare March 3, 2022 10:25

stefcameron approved these changes Mar 4, 2022

View reviewed changes

stefcameron merged commit dd6d0ec into focus-trap:master Mar 5, 2022

allcontributors bot mentioned this pull request Mar 5, 2022

docs: add DaviDevMod as a contributor for code, test #605

Merged

stefcameron mentioned this pull request Mar 5, 2022

Add displayCheck option to pass down to tabbable focus-trap/focus-trap#362

Closed

stefcameron added a commit that referenced this pull request Mar 6, 2022

WIP: adjusting code after #604 and comments

305b81e

stefcameron added a commit that referenced this pull request Mar 11, 2022

Adjusting code after #604 and comments

61d1a67

TODO: Add support for `getShadowRoot: true` instead of function.

stefcameron added a commit that referenced this pull request Mar 11, 2022

Adjusting code after #604 and comments

5290547

TODO: Add support for `getShadowRoot: true` instead of function.

stefcameron added a commit that referenced this pull request Mar 11, 2022

Adjusting code after #604 and comments

de1637e

This was referenced Mar 11, 2022

Shadow DOM support (v5.3.0) #570

Merged

fix(index.js) The tabIndex of audio, video and details was left to the default if set to some NaN #610

Merged

stefcameron added a commit that referenced this pull request Mar 17, 2022

Adjusting code after #604 and comments

898984b

stefcameron added a commit that referenced this pull request Apr 16, 2022

Adjusting code after #604 and comments

d606bcd

stefcameron added a commit that referenced this pull request Apr 20, 2022

Adjusting code after #604 and comments

91a2975

craigkovatch mentioned this pull request May 5, 2022

v5.3.0 breaks literally all of our usage of tabbable 😅 when called on a node not attached to the document #664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimizing `displayCheck: "full"` #604

Optimizing `displayCheck: "full"` #604

DaviDevMod commented Mar 2, 2022 •

edited by stefcameron

Loading

changeset-bot bot commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

DaviDevMod commented Mar 2, 2022 •

edited

Loading

stefcameron commented Mar 4, 2022

stefcameron left a comment

DaviDevMod commented Mar 4, 2022

stefcameron commented Mar 5, 2022

allcontributors bot commented Mar 5, 2022

craigkovatch commented May 5, 2022

Optimizing displayCheck: "full" #604

Optimizing displayCheck: "full" #604

Conversation

DaviDevMod commented Mar 2, 2022 • edited by stefcameron Loading

changeset-bot bot commented Mar 2, 2022 • edited Loading

🦋 Changeset detected

codecov bot commented Mar 2, 2022 • edited Loading

Codecov Report

DaviDevMod commented Mar 2, 2022 • edited Loading

stefcameron commented Mar 4, 2022

stefcameron left a comment

Choose a reason for hiding this comment

DaviDevMod commented Mar 4, 2022

stefcameron commented Mar 5, 2022

allcontributors bot commented Mar 5, 2022

craigkovatch commented May 5, 2022

Optimizing `displayCheck: "full"` #604

Optimizing `displayCheck: "full"` #604

DaviDevMod commented Mar 2, 2022 •

edited by stefcameron

Loading

changeset-bot bot commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

DaviDevMod commented Mar 2, 2022 •

edited

Loading