Fix treegrid navigation in dataview list #57905
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
talldan
added
[Type] Bug
|
Size Change: +13 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 17, 2024
| @@ -33,6 +33,7 @@ function buildSelector( sequential ) { | |||
| sequential ? '[tabindex]:not([tabindex^="-"])' : '[tabindex]', | |||
| 'a[href]', | |||
| 'button:not([disabled])', | |||
| '[role="button"][tabindex]', | |||
There was a problem hiding this comment.
Why is this needed, isn't this element caught by the first rule anyway?
There was a problem hiding this comment.
That was my first assumption, but unfortunately not. It would work fine when you first enter the list as <div role="button" tabindex="0"> matches the first selector.
However, as you navigate around the list, unfocused elements are given tabIndex="-1", so that results in a <div role="button" tabindex="-1"> element that doesn't match any of these selectors, meaning you can no longer navigate to them.
I figure adding this selector to the list will make it work like buttons which would match the selector on line 35 when they have a tabIndex="-1".
(though I still think it would be preferable to use a button element)
There was a problem hiding this comment.
Why unfocused elements are given tabIndex -1 in the first place?
There was a problem hiding this comment.
That's how roving tab index usually works. The last focused element has tabIndex of 0, and all other focusable elements -1, that way when tabbing back in, focus lands on the last focused element.
There was a problem hiding this comment.
Mmm, right but the question I have is:
"focusable" is a reusable utility and technically a div with tabIndex -1 is not "focusable" so the "right" thing to do seem to be, to not include these in the results.
There was a problem hiding this comment.
I don't plan to merge the base PR, so this is a bit of "what if" discussion. However, it seems like elements with button role should be picked up by the focusable package. Note that buttons (or any/most of the rules above) would also have tabIndex=-1 at some point when using the roving tabindex technique.
There was a problem hiding this comment.
Note that buttons (or any/most of the rules above) would also have tabIndex=-1 at some point when using the roving tabindex technique.
This actually seem like a bug to me. A button with tabIndex=-1 is not focusable AFAIK
There was a problem hiding this comment.
There's some semantics, but I think this is the reason why there are distinct tabbable and focusable utilities.
From the spec (https://html.spec.whatwg.org/multipage/interaction.html#the-tabindex-attribute):
−1 (or other negative integer values)
Causes the element to be focusable, and indicates that the author would prefer the element to be click focusable but not sequentially focusable. The user agent might ignore this preference for click and sequential focusability, e.g., for specific element types according to platform conventions, or for keyboard-only users.
There was a problem hiding this comment.
Ah, also found this info from the same spec which is relevant:
Being focusable is a statement about whether an element can be focused programmatically, e.g. via the focus() method or autofocus attribute. In contrast, sequentially focusable and click focusable govern how the user agent responds to user interaction: respectively, to sequential focus navigation and as activation behavior.
There was a problem hiding this comment.
Ah makes sense. Thanks for correcting me.
oandregal
merged commit 28bf788
into
update/dataviews-list-layout-keyboard-navigation
60 checks passed
|
Nice, thanks! I'll keep the conversation in the base PR. |
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.
Note, this PR merges into the branch associated with #57895, and not trunk.
What?
Attempt to solve the problem mentioned here - #57895 (comment)
How?
Ensures any role=button elements with a tabindex can be selected by the focusable utility.
Also adds a tabindex to the element so that it can be selected when focus enters the tree grid for the first time. After this happens though it defers to treegrid for tabindex management.