script: Add basic support for tab navigation#42952
Merged
Merged
Conversation
servo-highfive
added
the
S-awaiting-review
|
🔨 Triggering try run (#22574078780) for Linux (WPT) |
|
Test results for linux-wpt from try job (#22574078780): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (19)
Stable unexpected results (8)
|
|
|
simonwuelker
reviewed
Mar 2, 2026
| winning_node_and_tab_index = Some((element, element_tab_index)); | ||
| continue; | ||
| }; | ||
| // If the candidate is a less than the current winner, then it replaces it as the winner. |
Member
There was a problem hiding this comment.
did you mean to write "is a lesser tab index"?
Member
Author
There was a problem hiding this comment.
Sure. It was a bit implied from the text, but I agree that your wording is clearer. I'll clean up all the comments.
Member
Author
There was a problem hiding this comment.
I've updated most of the comments to be a bit clearer and also have renamed some variables to make it clear what the candidate is.
Comment on lines
+1755
to
+1757
| for node in root_node | ||
| .upcast::<Node>() | ||
| .traverse_preorder(ShadowIncluding::Yes) |
Member
There was a problem hiding this comment.
I wonder if its worth placing a (ordered) list of elements with a non-negative tabindex attribute on the document, to avoid this traversal. But we can worry about that later.
Member
Author
There was a problem hiding this comment.
We did consider this actually! It's a bit tricky because you still need to track the relative order of elements with equal tab indices in the DOM is. In the end we looked at what Gecko, WebKit, and Blink do. They seem to walk the DOM in this case. Even though it's expensive, I guess it's cheap enough as pressing tab is not a high frequency part of the code. Our plan was to do it this way and then optimize if we found an issue later.
Comment on lines
+1782
to
+1785
| let Some((_, winning_tab_index)) = winning_node_and_tab_index else { | ||
| winning_node_and_tab_index = Some((element, element_tab_index)); | ||
| continue; | ||
| }; |
Member
There was a problem hiding this comment.
If i understand tab navigation correctly then we will never find a better element if winning_tab_index is 1 and the focus direction is "forward", so can we return early here if that happens?
The same applies to find_first_tab_focusable_element.
Member
Author
There was a problem hiding this comment.
Hrm. In fact, I think we can generalize this. If we find any element with a tab index equal to current + 1 we can exit early. I'll make that change.
mrobinson
force-pushed
the
tab-navigation-1
branch
from
0b080bf to
c0bd3bc
Compare
Member
Author
|
@simonwuelker Thanks for the review. I've updated a new version of the change. PTAL when you have a moment. |
simonwuelker
requested changes
Mar 2, 2026
Comment on lines
+1755
to
+1757
| for node in root_node | ||
| .upcast::<Node>() | ||
| .traverse_preorder(ShadowIncluding::Yes) |
Comment on lines
+1857
to
+1861
| FocusDirection::Forward => match winning_node_and_tab_index { | ||
| Some((_, winning_tab_index)) | ||
| if compare_tab_indices(element_tab_index, winning_tab_index) != | ||
| Ordering::Less => {}, | ||
| _ => { |
Member
There was a problem hiding this comment.
This match statement is pretty convoluted, consider using something like
if winning_node_and_tab_index.is_none_or(|(_, winning_tab_index)| {
compare_tab_indices(element_tab_index, winning_tab_index) == Ordering::Less
}) {
// ...
}(same for the other match arm below)
Member
Author
There was a problem hiding this comment.
I've done this. It's indeed a bit clearer.
Comment on lines
+1862
to
+1864
| // Only promote a candidate to the current winner if it has a lesser tab | ||
| // index than the current winner or there is currently no winer. | ||
| winning_node_and_tab_index = Some((candidate_element, element_tab_index)); |
Member
There was a problem hiding this comment.
if element_tab_index is 1 at this point then we can immediately return the candidate element, because we're not going to find another element with higher precedence (= lesser tab index)
Member
Author
There was a problem hiding this comment.
Okay. This is done now.
servo-highfive
added
S-needs-code-changes
simonwuelker
reviewed
Mar 2, 2026
Comment on lines
+1730
to
+1731
| // Step 8. If result is true and target is a focusable area | ||
| // that is click focusable, then Run the focusing steps at target. |
Member
There was a problem hiding this comment.
Its unclear what spec algorithm this implements, please put a link either here or at the top of the function.
Member
Author
There was a problem hiding this comment.
Sorry. I'll remove these comments. They are from the event handling code that handles focus changes. I believe this part of tab navigation is not really specified.
mrobinson
force-pushed
the
tab-navigation-1
branch
from
c0bd3bc to
aeb8ac0
Compare
servo-highfive
added
S-awaiting-review
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
servo-highfive
added
S-tests-failed
This change adds very basic support for tab navigation, but without support for focus scopes. Followup changes will refine the behavior of this implementation to follow the specification around focus scopes, shadow DOM, and slots. In particular `delegatesFocus` is not supported here yet. Signed-off-by: Martin Robinson <[email protected]> Co-authored-by: Oriol Brufau <[email protected]>
mrobinson
force-pushed
the
tab-navigation-1
branch
from
aeb8ac0 to
9dd199b
Compare
servo-highfive
added
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
simonwuelker
pushed a commit
to simonwuelker/servo
that referenced
this pull request
Mar 3, 2026
This change adds very basic support for tab navigation, but without support for focus scopes. Followup changes will refine the behavior of this implementation to follow the specification around focus scopes, shadow DOM, and slots. In particular `delegatesFocus` is not supported here yet. Testing: This causes quite a few WPT tests to start passing. Signed-off-by: Martin Robinson <[email protected]> Co-authored-by: Oriol Brufau <[email protected]>
offline-ant
pushed a commit
to offline-ant/havi
that referenced
this pull request
Jun 4, 2026
This change adds very basic support for tab navigation, but without support for focus scopes. Followup changes will refine the behavior of this implementation to follow the specification around focus scopes, shadow DOM, and slots. In particular `delegatesFocus` is not supported here yet. Testing: This causes quite a few WPT tests to start passing. Signed-off-by: Martin Robinson <[email protected]> Co-authored-by: Oriol Brufau <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 change adds very basic support for tab navigation, but without
support for focus scopes. Followup changes will refine the behavior of
this implementation to follow the specification around focus scopes,
shadow DOM, and slots. In particular
delegatesFocusis not supportedhere yet.
Testing: This causes quite a few WPT tests to start passing.