Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should delegatesFocus use the flat tree? #7207

Closed
domenic opened this issue Oct 12, 2021 · 11 comments · Fixed by #7079
Closed

Should delegatesFocus use the flat tree? #7207

domenic opened this issue Oct 12, 2021 · 11 comments · Fixed by #7079
Labels
agenda+ To be discussed at a triage meeting topic: focus topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@domenic
Copy link
Member

domenic commented Oct 12, 2021

See discussion in #7079. Including the effects of that PR, focusing and unfocusing (e.g. via focus() and blur()) a delegatesFocus shadow host uses the flat tree: given

<!doctype html>
<div id="host">
  <div tabindex=0>In light tree</div>
</div>
<script>
  let host = document.getElementById("host");
  host.attachShadow({ mode: "open", delegatesFocus: true }).innerHTML = `
    <slot></slot>
    <div tabindex=0>In shadow tree</div>
  `;
  host.focus();
</script>

the <div tabindex=0>In light tree</div> will be focused, and if you insert host.blur() afterward, the div will be blurred.

@emilio states

I find delegating focus to stuff outside the shadow tree a bit weird, because you call focus(), the focus changes, but activeElement is not the host, and the host doesn't match :focus either.

I'm not sure why delegatesFocus was specified to look for focusable elements using the flat tree rather than the shadow-including tree of the shadow root, seems really weird.

which I tend to agree with. Recall that the main use cases for delegatesFocus are things like <input type="date"> where you want to focus things in the shadow DOM, not the light DOM.

The alternate proposal is that delegatesFocus should use the shadow tree. In this case, in the above example, the <div tabindex=0>In shadow tree</div> would get focus delegated to it, and document.activeElement would be the shadow host, and :focus would match the host.

@domenic
Copy link
Member Author

domenic commented Oct 15, 2021

@mfreed7, do you find the arguments here convincing? I think we should probably use the shadow tree instead of the flat tree, for both blur() and focus(); we could try to find some web developers to ask, but based on the known use cases for delegatesFocus I think shadow tree is correct.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 19, 2021

Yeah, I do find them a bit convincing. But as I mentioned, I'd feel better getting some input from developers that use delegatesFocus. Have we heard of any use cases that involve focusing slotted light dom content? I would hate to resolve this in favor of using the shadow root's shadow-including subtree, change browser behavior, and then find out what the critical use case was for instead using the flat tree.

Absent any such developer input, I suppose I could be convinced by the above arguments. I do think it sounds a bit funny to focus light dom slotted content. So if we're in a hurry to resolve this one way or the other, I'm ok changing Chromium's focus() to not use the flat tree.

@emilio
Copy link
Contributor

emilio commented Oct 19, 2021

I'm not on a massive rush other than "the longer we wait to change this, the easier it is for people to depend on it"

@domenic
Copy link
Member Author

domenic commented Oct 20, 2021

Trying to get developer feedback seems fair. I think you're more connected to the web components developer-using community than I am these days, so could you take point on that?

Edit: I also tweeted, to do my part :) https://twitter.com/domenic/status/1450919182850211849

I don't think we're in a big hurry, but it does stand out as a wart. And I got the impression it was hurting Firefox's progress on shipping some stuff?

@nemzes
Copy link

nemzes commented Oct 20, 2021

I'm my limited experience using WCs, this change seems to make sense and is in line with other isolation models. If I want to focus/blur some slotted content I should be able to do that regardless of the shadow DOM by keeping a reference to the slotted light DOM. I see delegatesFocus as part of the API into the WC, not the slotted content.

@WickyNilliams
Copy link

WickyNilliams commented Oct 21, 2021

This changes makes sense to me. I've only dabbled with delegatesFocus, due to lack of broad browser support, but i would have been surprised to discover that it can delegate to slotted content. I specifically want this to delegate to my shadow elements, that's the benefit of the feature imo

@bathos
Copy link

bathos commented Oct 21, 2021

I'd feel better getting some input from developers that use delegatesFocus

Just one data point but I have used it a fair amount and was under the impression that the proposed behavior was already how it worked. I either got lucky and just never had slots prior to the first in-shadow focusable or possibly just learned I have some bugs I don't know about!

@emilio
Copy link
Contributor

emilio commented Oct 21, 2021

Seems there's no great compelling argument for the current behavior then... I'm happy to update the WPTs and Gecko. @domenic can / do you want to fix the spec? It'd probably take me a bit more than you, but if you're busy I can try :-)

@emilio
Copy link
Contributor

emilio commented Oct 21, 2021

https://bugzilla.mozilla.org/show_bug.cgi?id=1737020 has a patch + tests

@domenic
Copy link
Member Author

domenic commented Oct 21, 2021

Sure, I'm happy to fix the spec. I will do it by expanding the scope of #7079.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 21, 2021

Thanks for tweeting, @domenic. Based on seemingly universal support for not including slotted content, I also agree. Let's land it.

Thanks, I appreciate you gathering some additional input!

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 21, 2021
This behavior is much more reasonable and aligns with developers
expectations better, see whatwg/html#7207.

Differential Revision: https://phabricator.services.mozilla.com/D129146

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1737020
gecko-commit: e41c536602c68eb6ad42e53e0d2a7f7a4209931e
gecko-reviewers: smaug, sefeng
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 22, 2021
…n the flat tree. r=smaug,sefeng

This behavior is much more reasonable and aligns with developers
expectations better, see whatwg/html#7207.

Differential Revision: https://phabricator.services.mozilla.com/D129146
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2021
This behavior is much more reasonable and aligns with developers
expectations better, see whatwg/html#7207.

Differential Revision: https://phabricator.services.mozilla.com/D129146

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1737020
gecko-commit: e41c536602c68eb6ad42e53e0d2a7f7a4209931e
gecko-reviewers: smaug, sefeng
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 25, 2021
…n the flat tree. r=smaug,sefeng

This behavior is much more reasonable and aligns with developers
expectations better, see whatwg/html#7207.

Differential Revision: https://phabricator.services.mozilla.com/D129146
domenic added a commit that referenced this issue Oct 28, 2021
* Make it use the shadow tree instead of the flat tree. Closes #7207.
* Make it work with the unfocusing steps symmetrically to the focusing steps. Closes #7070.
domenic added a commit that referenced this issue Nov 1, 2021
* Make it use the shadow tree instead of the flat tree. Closes #7207.
* Make it work with the unfocusing steps symmetrically to the focusing steps. Closes #7070.
domenic added a commit that referenced this issue Nov 1, 2021
9ba7124 updated delegatesFocus to use the shadow tree, as part of #7207. However we forgot to update the autofocus="" processing in the same way.
domenic added a commit that referenced this issue Nov 2, 2021
9ba7124 updated delegatesFocus to use the shadow tree, as part of #7207. However we forgot to update the autofocus="" processing in the same way.
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this issue Nov 18, 2021
This behavior is much more reasonable and aligns with developers
expectations better, see whatwg/html#7207.

Differential Revision: https://phabricator.services.mozilla.com/D129146

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1737020
gecko-commit: e41c536602c68eb6ad42e53e0d2a7f7a4209931e
gecko-reviewers: smaug, sefeng
dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
* Make it use the shadow tree instead of the flat tree. Closes whatwg#7207.
* Make it work with the unfocusing steps symmetrically to the focusing steps. Closes whatwg#7070.
dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
9ba7124 updated delegatesFocus to use the shadow tree, as part of whatwg#7207. However we forgot to update the autofocus="" processing in the same way.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
* Make it use the shadow tree instead of the flat tree. Closes whatwg#7207.
* Make it work with the unfocusing steps symmetrically to the focusing steps. Closes whatwg#7070.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
9ba7124 updated delegatesFocus to use the shadow tree, as part of whatwg#7207. However we forgot to update the autofocus="" processing in the same way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ To be discussed at a triage meeting topic: focus topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging a pull request may close this issue.

7 participants