Don't delegate focus when the shadow host is an ancestor of the currently focused area #5039
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
rebase with upstream
…ntly focused area
domenic
approved these changes
Oct 24, 2019
| <ol> | ||
| <li>If <var>new focus target</var> is a <span>shadow-including inclusive ancestor</span> of | ||
| the <span>currently focused area of a top-level browsing context</span>'s <span>DOM | ||
| anchor</span>, then set <var>new focus target</var> to null and stop these steps.</li> |
There was a problem hiding this comment.
It's a bit ambiguous how to interpret "stop these steps" here. I think the easiest thing to do will be to just nest the other three steps under an "Otherwise:" step 2.
I'll make that change since it's easy enough.
domenic
added
topic: focus
topic: shadow
|
I'll merge this, as well as the accompany tests, Monday or Tuesday if there haven't been any comments. Feel free to file bugs pointing to the spec and tests PR; there's no need to wait on landing before doing so. |
rniwa
reviewed
Oct 28, 2019
| area">focusable areas</span> whose <span>DOM anchor</span> is a descendant of <var>new focus | ||
| target</var> in the <span>flat tree</span>.</p> | ||
| <ol> | ||
| <li>If <var>new focus target</var> is a <span>shadow-including inclusive ancestor</span> of |
There was a problem hiding this comment.
I don't think this is quite right. You can have two elements that are descendants of each other without shadow trees, in those cases, focus() should be able to move the focus to the outer / ancestor node which is a simple ancestor of the currently focused element.
e.g.
const div = document.body.appendChild(document.createElement('div'));
div.tabIndex = 0;
const span = div.appendChild(document.createElement('span'));
span.textContent = 'hello'
div.tabIndex = 0;
span.tabIndex = 0;
span.focus();
div.focus(); // This should move the focus to div.There was a problem hiding this comment.
I think it does move the focus to the div, because div is not a shadow host that delegates focus. These steps are within the "If new focus target is a shadow host whose shadow root 's delegates focus is true" clause.
|
Also see #5052 |
8 tasks
josepharhar
added a commit
to josepharhar/html
that referenced
this pull request
Oct 29, 2022
I was implementing the new dialog shadowdom initial focus goodness when I noticed this in chromium: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc;l=4714-4718;drc=82b10d45463bbc3d019f5ef981dc8afeb8900a6d When getting the focusable area of a target, in the case that the target is a shadow-incuding inclusive ancestor of the currently focused element, the spec says to return null, but chromium's implementation returns the currently focused element instead and cites this issue: WICG/webcomponents#840 I'm not certain exactly what the consensus was, but I do see that this spec PR and test was made in response: whatwg#5039 web-platform-tests/wpt#19867 The added test doesn't fail if I return null like the spec says to do, but dialog-focus-shadow-double-nested.html does fail if I return null with this error message: ``` FAIL showModal() assert_equals: document.activeElement expected Element node <div></div> but got Element node <button tabindex="-1">Focusable</button> ``` Based on this test failure and reading the WICG issue, I believe that we should be returning the currently focused element, which this patch does.
4 tasks
domenic
pushed a commit
that referenced
this pull request
Nov 1, 2022
Doing otherwise causes https://github.com/web-platform-tests/wpt/blob/4c24c9acd809eb8630e795e3f8e3a787d9d6d039/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-double-nested.html to fail on the document.activeElement check. This matches some previous discussions in WICG/webcomponents#840 and #5039.
pmeenan
pushed a commit
to pmeenan/html
that referenced
this pull request
Dec 2, 2022
Doing otherwise causes https://github.com/web-platform-tests/wpt/blob/4c24c9acd809eb8630e795e3f8e3a787d9d6d039/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-double-nested.html to fail on the document.activeElement check. This matches some previous discussions in WICG/webcomponents#840 and whatwg#5039.
pmeenan
pushed a commit
to pmeenan/html
that referenced
this pull request
Dec 2, 2022
Doing otherwise causes https://github.com/web-platform-tests/wpt/blob/4c24c9acd809eb8630e795e3f8e3a787d9d6d039/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-double-nested.html to fail on the document.activeElement check. This matches some previous discussions in WICG/webcomponents#840 and whatwg#5039.
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.
As discussed in WICG/webcomponents#840
(See WHATWG Working Mode: Changes for more details.)
cc @domenic @rniwa @smaug----
/interaction.html ( diff )