Support elements with positive tabindex attributes in single-container traps #974
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: e26c3aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
3 tasks
Fixes #375 ALL existing cypress tests are passing. TODO: - [ ] get rid of DEBUG comments - [ ] will need to share logic from tabbable about determining tabindex somehow - [ ] check manually tested examples - [ ] see if a cypress test can be added for the new positive-tabindex example - [ ] add changeset
stefcameron
added a commit
to focus-trap/tabbable
that referenced
this pull request
Jun 26, 2023
This is needed for focus-trap/focus-trap#974 which will add support for positive tabindexes in focus-trap.
stefcameron
added a commit
to focus-trap/tabbable
that referenced
this pull request
Jun 26, 2023
This is needed for focus-trap/focus-trap#974 which will add support for positive tabindexes in focus-trap. Also updates the docs/typings to make them more consistent and fix some broken links.
stefcameron
added a commit
to focus-trap/tabbable
that referenced
this pull request
Jun 26, 2023
This is needed for focus-trap/focus-trap#974 which will add support for positive tabindexes in focus-trap. Also updates the docs/typings to make them more consistent and fix some broken links.
stefcameron
force-pushed
the
positive-tabindex
branch
from
8aae81d
to
9ed40da
Compare
stefcameron
changed the title
stefcameron
force-pushed
the
positive-tabindex
branch
from
9ed40da
to
6844c90
Compare
- focus-trap limited to a single container if at least one positive tabindex node is found in any of the containers given to it; an exception is thrown - handled negative tabindex edge case, setting focus to the next tabbable node in DOM order (should be document position, but that would require extensive work not worth the effort for this feature) - Removed all DEBUG comments - Using new tababble `getTabIndex()` API from tabbable v6.2.0 - Added Cypress test - Added Changeset - Manually checked "manual-check" examples
stefcameron
force-pushed
the
positive-tabindex
branch
from
6844c90
to
f22e361
Compare
|
@stefcameron It looks like I can't review arbitrary lines, eg, I can't review line 516 of index.js to make this suggestion. So either I open another PR against |
|
@DaviDevMod I think it's because using Unless it's just that one suggestion you're wanting to make, in which case I can easily add that myself, but just wondering what's going on there. |
Merged
* Fix `destinationNode` in case the `target` of the keyboard event has a negative tab index
* Refactor `findNextNavNode`
Simplified the logic a bit and introduced the following condition:
```js
containerGroup.focusableNodes.indexOf(target) >=
containerGroup.focusableNodes.indexOf(
containerGroup.lastTabbableNode
)
```
checking that `target` is either the `lastTabbableNode` in the container or a focusable (and not tabbable) node preceding it in document order.
* Simplify `nextTabbableNode`
* Fix `nextTabbableNode` logic for case in which `node` is not tabbable
* Cache index of `node` within the `focusableNodes` array, in the body of `nextTabbleNode`
* Further simplify `nextTabbableNode`
The logic for the case in which `node` has a negative tab index would work also for non-negative tab indexes.
Distinguishing between the two cases may be more performant in principle, but not enough to justify any added complexity (at least in my opinion).
* Distinguish (again) between `node` with negative and non-negative tab index, in `nextTabbableNode`
This reverts commits 075b58c and f198c2b
* Revert "Refactor `findNextNavNode`"
This reverts commit 91da5ca.
|
@all-contributors add @DaviDevMod for code, bug |
|
I've put up a pull request to add @DaviDevMod! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #375
ALL existing cypress tests are passing.
The goal is to take the least invasive approach when it comes to enforcing tab order. Let the browser do as much of the work as possible, and only interject when it doesn't do what we expect it to (which is the general theme of focus-trap).
Here's the working demo, including negative tabindex edge case support:
DONE:
updateTabbableNodes()if >1 container && >0 positive tabindex nodes found, then throw Error("trap not supported")PR Checklist
Please leave this checklist in your PR.
npm run demo-bundlein your branch and include the/docs/demo-bundle.jsfile that gets generated in your PR).// TEST MANUALLYcomments here) that can't be fully tested in Cypress have been manually verified.typeof document/window !== 'undefined'before using it in code that gets executed on load.npm run changesetlocally to add one, and follow the prompts).