Handle edge cases in with-constrained-tabbing. #10174
Merged
Conversation
This file contains hidden or 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
aduth
approved these changes
Sep 27, 2018
Member
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
This component could use some tests.
| * When pressing Tab and none of the tabbables has focus, the keydown | ||
| * event happens on the wrapper div: move focus on the first tabbable. | ||
| */ | ||
| } else if ( ! tabbables.includes( event.target ) ) { |
Member
There was a problem hiding this comment.
Would it be enough to test event.target === this.focusContainRef.current ? More optimized, more explicit to what we are testing.
Or are there other cases where the target of the tab press would itself not be tabbable? I guess might account for elements with tabIndex=-1 within the content? If so, should be update the comment to reflect this?
Contributor
Author
There was a problem hiding this comment.
Yep it would be nicer and simpler. Unfortunately, it doesn't work on IE11 where the event.target seems to be the clicked children element in the content. See below, in order:
- event.target
- event.currentTarget
- this.focusContainRef.current
Checks edge cases in withConstrainedTabbing.
mcsf
force-pushed
the
update/with-constrained-tabbing-improvements
branch
from
42b8656 to
2ee25cc
Compare
Contributor
|
Added tests co-authored with @talldan. Should be ready to merge. |
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
Handle edge cases to keep tabbing constrained in
withConstrainedTabbing, for example when clicking in the content and then pressing Tab. See conversation on #9973To test:
Previously, focus went to the document root.
By adding
tabIndex="-1"to thewithConstrainedTabbingit is possible to detect when theevent.targetis not the firstTabbable orlastTabbable. This happens, for example when the content is clicked and then Tab is pressed again. In this edge case, focus is moved to the first tabbable.Fixes #10165