Improve autofocus and delegatesFocus interaction #6990

domenic · 2021-08-25T17:16:28Z

Now, when focusing a shadow host with delegates focus set, we try to focus the first focusable area with the autofocus attribute set, instead of just the first focusable area.

Closes #833. Note that this implements (2) from #833 (comment) , not (1).

At least two implementers are interested (and none opposed):
- We had rough consensus between all implementers in TPAC 2018, but we should get more explicit signoff for this particular variation.
- Per below review comments Gecko and Chromium have signed off on this specific proposal.
Tests are written and can be reviewed and commented upon at:
- https://phabricator.services.mozilla.com/D128301
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1259645
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1728864
- Safari: https://bugs.webkit.org/show_bug.cgi?id=231678

(See WHATWG Working Mode: Changes for more details.)

/infrastructure.html ( diff )
/interaction.html ( diff )

Now, when focusing a shadow host with delegates focus set, we try to focus the first focusable area with the autofocus attribute set, instead of just the first focusable area. Closes #833.

source

smaug---- · 2021-09-02T16:44:38Z

@sefeng211

sefeng211 · 2021-09-02T18:38:07Z

Looks good! I've filed the bug for Firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1728864

domenic · 2021-09-14T20:53:56Z

Ping @mfreed7 for review and maybe tests if he's up for writing them :)

sefeng211 · 2021-09-15T14:15:40Z

I'll start working on the Firefox implementation for this shortly, and I can write the tests along the way.

sefeng211 · 2021-09-17T16:16:53Z

source

+
+      <ol>
+       <li><p>Set <var>potential autofocus delegates</var> to the list of all <span>click
+       focusable</span> <span data-x="focusable area">focusable areas</span> whose <span>DOM


Shadow host with delegateFocus = true is not a focusable area right, so we won't try to delegating the focus to the inner shadow DOM for hosts that have autofocus + delegatesFocus = true?

Hmm, you are right. And that is consistent with the existing behavior where you can't delegateFocus to a delegatesFocus shadow root. And you cannot delegatesFocus to an area element's shapes, either, I guess. Because we strictly look for focusable areas when assembling possible focus delegates / possible autofocus delegates.

But that feels a bit weird. Maybe we should fix that, both for the existing case and for this new case? WDYT?

Right now, we look for focusable area in the flat tree, so we are able to delegate the focus to an inner shadow DOM with delegatesFocus.

So something like this https://jsfiddle.net/8t297ekp/ works. Do I misunderstand something?

I am not sure; I think I might be confused too. In your example, innerHost is not a focusable area, right? Only the <input> is. So it doesn't matter where innerHost is delegatesFocus or not, right? When you try to focus host, It just finds the <input> as a flat tree child and focuses that instead, I think...

That sounds right. How about an example like this https://jsfiddle.net/41e0zhqn/1/?

I think we'd want the innerInput to be focus, however with the current PR, it won't because nothing will be added to the possible autofocus delegates, so we go through possible focus delegates, thus the outerInput is focused?

Yes, I agree. I can imagine roughly the following paths to solving this:

Make potential autofocus delegates only care about the autofocus attribute, and not care about focusable area-ness.

Make potential autofocus delegates include delegatesHost shadow hosts via a one-off addition. I.e. set it to what it currently is plus any descendant delegatesFocus shadow hosts.

Make potential autofocus delegates computation smarter, by having it look at all descendants, call "get the focusable area", and if the result is non-null store it in potential autofocus delegates.

Try to use the flat tree somehow instead of the DOM tree? That was my original approach but @rniwa pointed out in Improve autofocus and delegatesFocus interaction #6990 (comment) that it didn't work that well. I wonder if there's something smarter we can do that lets us stay more symmetric with the non-autofocus case, but I can't think of what that would be.

I think (3) is probably best. WDYT?

So I think rniwa's concern was if we have a DOM like this

<div #host> (delegatesFocus = true) #shadowRoot <div #innertHost> #shadowRoot <input autofocus #innerInput> <input autofocus #outerInput>

We'd want #outerInput to be focused, so we don't want to use a flat tree?

(3) also sounds the best to me, however, I think we still need to make sure the above example works?

OK, I think I pushed a version of (3) and I believe it works for that case as well. PTAL!

sefeng211 · 2021-09-17T23:07:37Z

source

+    <ol>
+     <li><p>If <var>descendant</var> does not have an <code
+     data-x="attr-fe-autofocus">autofocus</code> content attribute, then
+     <span>continue</span>.</p></li>


Hmm....So we don't consider inner elements if the ancestor doesn't have autofocus. For a case like this

<div #host> (delegatesFocus = true) <input #outerInput> #shadowRoot <div #innertHost> #shadowRoot <input autofocus #innerInput>

outerInput is going to be focused, is this expected...?

I think that is the desired behavior, although it'd be good to have @rniwa confirm. The way I think about it is that at a per-shadow-tree level, by default you focus the first focusable area, but you can put autofocus on something specific to override it. Since there's no autofocus in the first level (i.e. in the shadow-including children of #host), we focus #outerInput.

Sorry for the long delay reviewing this. After reading and re-reading, I think the last comment (and the current state of the spec text) feels right. The delegatesfocus attribute applies to each shadow host individually, and does not "cascade" into contained shadow roots. I like the recursive use of "get the focusable area". So LGTM.

Got it, I agree, that makes sense to me.

mfreed7 · 2021-09-25T20:56:21Z

source

+    <ol>
+     <li><p>If <var>descendant</var> does not have an <code
+     data-x="attr-fe-autofocus">autofocus</code> content attribute, then
+     <span>continue</span>.</p></li>


Sorry for the long delay reviewing this. After reading and re-reading, I think the last comment (and the current state of the spec text) feels right. The delegatesfocus attribute applies to each shadow host individually, and does not "cascade" into contained shadow roots. I like the recursive use of "get the focusable area". So LGTM.

annevk

Looks good editorially, apart from one sentence that should be addressed before landing. But approving to make that easier.

annevk · 2021-10-11T10:41:51Z

source

+     <li><p>Return the first <span>focusable area</span> in <span>tree order</span> of their <span
+     data-x="DOM anchor">DOM anchors</span> in <var>possible focus delegates</var>, or null if


"of who their" and "are in"?

domenic · 2021-10-12T19:57:26Z

Great, this is ready to land except for tests then. @sefeng211 any progress on those?

sefeng211 · 2021-10-13T01:04:10Z

I just finished the patch for Firefox (which includes the tests). It's under review, hopefully, it'll be landed soon.

Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1728864 gecko-commit: d3034ec3376b62837afd4ee95acb18373d0143df gecko-reviewers: smaug

…ates focus r=smaug Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301

Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1728864 gecko-commit: d3034ec3376b62837afd4ee95acb18373d0143df gecko-reviewers: smaug

…ates focus r=smaug Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301

Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1728864 gecko-commit: d3034ec3376b62837afd4ee95acb18373d0143df gecko-reviewers: smaug

Improve autofocus and delegatesFocus interaction

7db993d

Now, when focusing a shadow host with delegates focus set, we try to focus the first focusable area with the autofocus attribute set, instead of just the first focusable area. Closes #833.

domenic added addition/proposal New features or enhancements topic: focus agenda+ To be discussed at a triage meeting labels Aug 25, 2021

rniwa reviewed Aug 29, 2021

View reviewed changes

source Show resolved Hide resolved

Normal "descendant" for autofocus delegates

10ccc0a

past removed the agenda+ To be discussed at a triage meeting label Sep 2, 2021

past mentioned this pull request Sep 2, 2021

Upcoming HTML standard issue triage meeting on 9/2/2021 #6936

Closed

domenic requested a review from mfreed7 September 3, 2021 17:34

domenic requested a review from annevk September 15, 2021 16:06

sefeng211 reviewed Sep 17, 2021

View reviewed changes

New approach

3de77cb

sefeng211 reviewed Sep 17, 2021

View reviewed changes

mfreed7 approved these changes Sep 25, 2021

View reviewed changes

sefeng211 approved these changes Oct 1, 2021

View reviewed changes

annevk approved these changes Oct 11, 2021

View reviewed changes

past mentioned this pull request Oct 11, 2021

Upcoming HTML standard issue triage meeting on 10/7/2021 #7014

Closed

Editorial fix

af8c139

domenic merged commit 383e1a9 into main Oct 13, 2021

domenic deleted the autofocus-delegatesfocus-2 branch October 13, 2021 15:53

moz-wptsync-bot mentioned this pull request Oct 25, 2021

[Gecko Bug 1728864] Focus the first focusable area with autofocus for delegates focus web-platform-tests/wpt#31376

Merged

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 26, 2021

Bug 1728864 - Focus the first focusable area with autofocus for deleg…

27331d5

…ates focus r=smaug Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 27, 2021

Bug 1728864 - Focus the first focusable area with autofocus for deleg…

7723377

…ates focus r=smaug Spec: whatwg/html#6990 Differential Revision: https://phabricator.services.mozilla.com/D128301

past mentioned this pull request Nov 5, 2021

Upcoming HTML standard issue triage meeting on 11/4/2021 #7201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve autofocus and delegatesFocus interaction #6990

Improve autofocus and delegatesFocus interaction #6990

domenic commented Aug 25, 2021 •

edited

Loading

smaug---- commented Sep 2, 2021

sefeng211 commented Sep 2, 2021

domenic commented Sep 14, 2021

sefeng211 commented Sep 15, 2021

sefeng211 Sep 17, 2021

domenic Sep 17, 2021

sefeng211 Sep 17, 2021

domenic Sep 17, 2021

sefeng211 Sep 17, 2021

domenic Sep 17, 2021

sefeng211 Sep 17, 2021 •

edited

Loading

domenic Sep 17, 2021

sefeng211 Sep 17, 2021

domenic Sep 20, 2021

mfreed7 Sep 25, 2021

sefeng211 Oct 1, 2021

mfreed7 Sep 25, 2021

annevk left a comment

annevk Oct 11, 2021

domenic commented Oct 12, 2021

sefeng211 commented Oct 13, 2021

		<li><p>Return the first <span>focusable area</span> in <span>tree order</span> of their <span
		data-x="DOM anchor">DOM anchors</span> in <var>possible focus delegates</var>, or null if

Improve autofocus and delegatesFocus interaction #6990

Improve autofocus and delegatesFocus interaction #6990

Conversation

domenic commented Aug 25, 2021 • edited Loading

smaug---- commented Sep 2, 2021

sefeng211 commented Sep 2, 2021

domenic commented Sep 14, 2021

sefeng211 commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sefeng211 Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Oct 12, 2021

sefeng211 commented Oct 13, 2021

domenic commented Aug 25, 2021 •

edited

Loading

sefeng211 Sep 17, 2021 •

edited

Loading