Constrained tabbing: simplify #34836
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
7 tasks
|
Size Change: +40 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Aljullu
reviewed
Sep 15, 2021
There was a problem hiding this comment.
Thanks for picking this up and working on this alternative @ellatrix. I like how elegant this solution is and that it doesn't require e.preventDefault() on the native event. During my testing, it worked great, but I found one edge case where I think it's not working as expected.
It's possible to reproduce it modifying the Modal story as follows (L31-L35):
<Modal { ...props } onRequestClose={ closeModal }>
- <Button variant="secondary" onClick={ closeModal }>
- Close Modal
- </Button>
+ <div className="wrapper-div" tabIndex={ -1 }>
+ <Button
+ onClick={ () => {
+ const wrapper = document.querySelector(
+ '.wrapper-div'
+ );
+ wrapper.focus();
+ } }
+ >
+ Focus wrapper
+ </Button>
+ <Button variant="secondary" onClick={ closeModal }>
+ Close Modal
+ </Button>
+ </div>
</Modal>What it does is that when you click on the Focus wrapper button, it will focus a wrapper div around both buttons. Pressing Tab again, I would expect Focus wrapper to be focused, but instead the close button is.
This video shows the issue (I added *:focus { border: 2px solid red } so it's easier to see where the focus is).
focus-not-in-wrapper.mp4
I took a look at @wordpress/dom and I think the issue comes from this line:
gutenberg/packages/dom/src/tabbable.js
Line 187 in 79be738
It's filtering out tabbable elements inside the currently focused element, but I don't think that's needed? What do you think?
Removed it now, but I expect some failures. We should probably add another function |
|
Actually it works fine in other instances too. It must have been a mistake. Omitting |
|
Instead of listening to the Tab key, can't we just render two focus trap elements around the modal element with I made this video a while ago using that technique to solve a more complex problem, but the idea is similar: https://www.youtube.com/watch?v=ELauJdPoDaQ Update: this is also how the focus is trapped within dialogs in the WAI-ARIA docs (w3c/aria-practices#545). |
|
@diegohaz I'm not sure it's that simple. When we detect focus on a capture element, we can more focus to the other side, but then focus will remain there in the capture element. We still need to find the next tabbable element and focus that instead of the focus capture element. So in the end, the complexity seems roughly the same as this. |
ellatrix
force-pushed
the
simplify/constrained-tabbing
branch
from
c7575dc to
e1e1db1
Compare
|
Tests are passing and it fixes the problem, so I'm going to merge this. |
This was referenced Nov 18, 2022
This was referenced Nov 22, 2022
This was referenced Jan 25, 2023
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.
Description
Fixes #34681.
Alternative to #34684.
Idea: on tab, check if the previous/next tubbable is outside the constrained tabbing area, and if so, move focus to the wrapper. Since we're not preventing default behaviour, the browser will automatically move focus in the right direction!
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).