Should host with delegatesFocus true keep the previous focused element focused if it's a shadow-including descendant of the host?
#840
Comments
rakina
changed the title
delegatesFocus true keep the previous focused element focused if it's a shadow-including inclusive ancestor of the host?delegatesFocus true keep the previous focused element focused if it's a shadow-including descendant of the host?
|
Hm... that might be annoying behavior for users though. If page's script kept focusing a component, it would always reset the focus in the component. I guess the current processing model returns early if the current focus is already at where |
|
You mean we would keep the current focused element in all case where shadow host is already a shadow-including ancestor of the focused element, regardless of |
|
I mean that focusing step should bail out as it does in step 4. |
|
Yeah, I agree focus shouldn't change. |
|
Hmm, interesting. Though I don't really understand cases where the page would try to focus on a host multiple times, if you both think this behavior is actually justified I guess that's fine (+ no need to worry about the risk of changing Blink behavior, yay). So we'll check "If new focus target is a shadow host whose shadow root's delegates focus is true, and new focus target is a shadow-including ancestor of the currently focused area of a top-level browsing context, then return."? |
|
Yeah, something like that. |
|
Has the PRs been updated to reflect this? |
|
The PR which got merged for this issue seems to have a bug as I pointed out in PR. The current spec also seems to take care of the nested delegation: whatwg/html#5052 |
In particular, don't delegate focus when the shadow host is an ancestor of the currently focused area. Fixes WICG/webcomponents#840. Tests: web-platform-tests/wpt#19867.
… hosts which delegate focus https://bugs.webkit.org/show_bug.cgi?id=203869 Reviewed by Antti Koivisto. LayoutTests/imported/w3c: Imported the latest test from web-platform-tests/wpt#20079. * web-platform-tests/shadow-dom/focus/focus-method-delegatesFocus-expected.txt: * web-platform-tests/shadow-dom/focus/focus-method-delegatesFocus.html: Source/WebCore: Fixed the bug that WebKit doesn't skip a shadow host with delegatesFocus set when looking for the first programatically focusable element. Test: imported/w3c/web-platform-tests/shadow-dom/focus/focus-method-delegatesFocus.html * dom/Element.cpp: (WebCore::shadowRootWithDelegatesFocus): Added. (WebCore::isProgramaticallyFocusable): Updated to return false on a shadow host with delegatesFocus. (WebCore::Element::focus): Don't move the focus if the shadow host only contains a focused element per WICG/webcomponents#840. Canonical link: https://commits.webkit.org/217572@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252537 268f45cc-cd09-0410-ab3c-d52691b4dbfc
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.
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.
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.
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.
Just noticed this behavior in Blink when re-reading the focus code one last time for
delegatesFocusspecs whatwg/html#4796.So the case is like this:
The currently focused element is
#bar, and then we callhost.focus(). In Blink right now we would keep the focus on#barand not focus on anything else. In fact we would keep the focused element whenever we try to focus on#hostand any ofhost's shadow-including descendant is the currently focused element.This seems weird and we aren't doing the "keep the current focused element" in any other case (CMIIW), so I propose we don't spec this behavior, and instead do the focus delegation in
hostno matter what (with the delegation methods from #830 (comment))cc @domenic @rniwa @smaug---- @annevk @ekashida
The text was updated successfully, but these errors were encountered: