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

[#292] Add support for inert attribute #924

Merged
merged 7 commits into from
Feb 15, 2023
Merged

[#292] Add support for inert attribute #924

merged 7 commits into from
Feb 15, 2023

Conversation

stefcameron
Copy link
Member

Fixes #292

Inert elements, as well as elements inside inert ancestors, will no longer be treated as tabbable/focusable.

Considering this a minor update since inert elements in browsers supporting the inert attribute (which has wide enough support now) would already be non-interactive. So this is tabbable catching to reality rather than tabbable altering behavior.

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.

Fixes #292

Inert elements, as well as elements inside inert ancestors, will no longer
be treated as tabbable/focusable.

Considering this a minor update since inert elements in browsers supporting
the inert attribute (which has wide enough support now) would already be
non-interactive. So this is tabbable catching to reality rather than
tabbable altering behavior.
@changeset-bot
Copy link

changeset-bot bot commented Feb 9, 2023

🦋 Changeset detected

Latest commit: a89c74c

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

This PR includes changesets to release 1 package
Name Type
tabbable Minor

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

codecov bot commented Feb 9, 2023

Codecov Report

Base: 97.87% // Head: 97.94% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (a89c74c) compared to base (b8c7550).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
+ Coverage   97.87%   97.94%   +0.07%     
==========================================
  Files           1        1              
  Lines         235      243       +8     
  Branches      113      119       +6     
==========================================
+ Hits          230      238       +8     
  Misses          5        5              
Impacted Files Coverage Δ
src/index.js 97.94% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stefcameron
Copy link
Member Author

Unfortunately, FF still does not support inert, therefore, henceforth, all FF PR builds will fail as it stands because we don't have a suite of FF-specific tests that don't include use of inert...

So I have disabled the requirement for the FF tests to pass.

@idoros
Copy link
Contributor

idoros commented Feb 9, 2023

Not sure if this works, but this post suggests Cypress supports conditional test configuration: https://filiphric.com/skip-test-conditionally-with-cypress#test-configuration

* False if `node` is falsy.
*/
const isInert = function (node, lookUp = true) {
return !!(node?.inert || (lookUp && node && isInert(node.parentNode))); // recursive
Copy link
Contributor

@idoros idoros Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to lookup with JavaScript instead of matching with CSS match:

With parent check:
is not inert: node.matches(':not([inert]), :not([inert] *)')
is inert: node.matches('[inert], :is([inert] *)')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but unfortunately we can't call matches on what I think are the shadow roots. Lots of shadow-related tests fail because of matches is not a function errors when I use your suggestion. What I have right now works in all cases.

src/index.js Outdated
'[contenteditable]:not([contenteditable="false"])',
'details>summary:first-of-type',
'details',
'input:not([inert] *)',
Copy link
Contributor

@idoros idoros Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add :not([inert]) to all of these selectors to save the extra JavaScript check on the element itself

This can also be done with multiple selectors inside the :not(), with pretty good browser support, but probably less then what is currently supported: :not([inert], [inert] *)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. We can eliminate more candidates right off the bat with the addition of :not([inert]), which I have done. I didn't use the multi-selector syntax since the exhaustive syntax has broader support (just in case...).

@stefcameron
Copy link
Member Author

@idoros

Not sure if this works, but this post suggests Cypress supports conditional test configuration: https://filiphric.com/skip-test-conditionally-with-cypress#test-configuration

Yes! I had no idea. It was buried in cypress-io/cypress#5346 and https://github.com/cypress-io/cypress-documentation/pull/3209/files but here's the official doc entry: https://docs.cypress.io/guides/references/configuration#Single-test-configuration

Thank you!

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

Successfully merging this pull request may close these issues.

support inert attribute
2 participants