Fix FocusTrap escape due to strange tabindex values #2093
Merged
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
It will still keep the same DOM order if tabIndex matches, thanks to stable sorts!
All the arguments resulted in usage like `focusIn(container, Focus.First, true, null)`, and to make things worse, we need to add something else to this list in the future. Instead, let's keep the `container` and the type of `Focus` as known params, all the other things can sit in an options object.
This code will now ensure that we can't escape the FocusTrap if you use `<tab>` and you happen to tab to an element outside of the FocusTrap because the next item in line happens to be outside of the FocusTrap and we never hit any of the focus guard elements. How it works is as follows: 1. The `onBlur` is implemented on the `FocusTrap` itself, this will give us some information in the event itself. - `e.target` is the element that is being blurred (think of it as `from`) - `e.currentTarget` is the element with the event listener (the dialog) - `e.relatedTarget` is the element we are going to (think of it as `to`) 2. If the blur happened due to a `<tab>` or `<shift>+<tab>`, then we will move focus back inside the FocusTrap, and go from the `e.target` to the next or previous value. 3. If the blur happened programmatically (so no tab keys are involved, aka no direction is known), then the focus is restored to the `e.target` value. Fixes: #1656
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
bruh what a fix 😅 |
ahuth
added a commit
to chanzuckerberg/edu-design-system
that referenced
this pull request
Jan 5, 2023
Resolves e2e test failures in Traject (see FB-PLP/traject#13040). Issue starts in 1.7.6, possibly from tailwindlabs/headlessui#2093.
ahuth
added a commit
to chanzuckerberg/edu-design-system
that referenced
this pull request
Jan 5, 2023
Resolves e2e test failures in Traject (see FB-PLP/traject#13040). Issue starts in 1.7.6, possibly from tailwindlabs/headlessui#2093.
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.
This PR will now ensure that we can't escape the FocusTrap if you use
<tab>and you happen to tab to an element outside of the FocusTrap because the next item in line happens to be outside of the FocusTrap and we never hit any of the focus guard elements.How it works is as follows:
onBluris implemented on theFocusTrapitself, this will give us some information in the event itself.e.targetis the element that is being blurred (think of it asfrom)e.currentTargetis the element with the event listener (the dialog)e.relatedTargetis the element we are going to (think of it asto)<tab>or<shift>+<tab>, then we will move focus back inside the FocusTrap, and go from thee.targetto the next or previous value.e.targetvalue.Fixes: #1656