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.

Sign up for GitHub

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

Jump to bottom

Improve autofocus and delegatesFocus interaction #6990

Merged
merged 4 commits into from
Oct 13, 2021
Merged

Improve autofocus and delegatesFocus interaction #6990

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7db993d
Improve autofocus and delegatesFocus interaction
domenic Aug 25, 2021
10ccc0a
Normal "descendant" for autofocus delegates
domenic Aug 31, 2021
3de77cb
New approach
domenic Sep 17, 2021
af8c139
Editorial fix
domenic Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 52 additions & 21 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2974,6 +2974,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-child" data-x="concept-tree-child">child</dfn> concept</li>
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-root">root</dfn> and <dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-root">shadow-including root</dfn> concepts</li>
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-inclusive-ancestor">inclusive ancestor</dfn>,
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-descendant">descendant</dfn>,
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-descendant">shadow-including descendant</dfn>,
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-descendant">shadow-including inclusive descendant</dfn>, and
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-ancestor">shadow-including inclusive ancestor</dfn> concepts</li>
Expand Down Expand Up @@ -74956,8 +74957,8 @@ END:VCARD</pre>

<p>To <dfn>get the focusable area</dfn> for a <var>focus target</var> that is either an element
that is not a <span>focusable area</span>, or is a <span>browsing context</span>, given an
optional string <var>focus trigger</var>, run the first matching set of steps from the following
list:</p>
optional string <var>focus trigger</var> (default "<code data-x="">other</code>"), run the first
matching set of steps from the following list:</p>

<dl class="switch">
<dt>If <var>focus target</var> is an <code>area</code> element with one or more shapes that are
Expand Down Expand Up @@ -74993,28 +74994,28 @@ END:VCARD</pre>

<dd>
<ol>
<li>If <var>focus target</var> is a <span>shadow-including inclusive ancestor</span> of the
<li><p>If <var>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 return null.</li>
then return null.</p></li>

<li>
<p>Otherwise:</p>
<li><p>Let <var>autofocus delegate</var> be the <span>autofocus delegate</span> for <var>focus
target</var> given <var>focus trigger</var>.</p></li>

domenic marked this conversation as resolved.
Show resolved Hide resolved
<ol>
<li>If <var>focus trigger</var> is "<code data-x="">click</code>", then let <var>possible
focus delegates</var> be the list of all <span>click focusable</span> <span
data-x="focusable area">focusable areas</span> whose <span>DOM anchor</span> is a descendant
of <var>focus target</var> in the <span>flat tree</span>.</li>

<li>Otherwise, let <var>possible focus delegates</var> be the list of all <span
data-x="focusable area">focusable areas</span> whose <span>DOM anchor</span> is a descendant
of <var>focus target</var> in the <span>flat tree</span>.</li>

<li>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
<var>possible focus delegates</var> is empty.</li>
</ol>
</li>
<li><p>If <var>autofocus delegate</var> is not null, then return <var>autofocus
delegate</var>.</p></li>

<li><p>If <var>focus trigger</var> is "<code data-x="">click</code>", then let <var>possible
focus delegates</var> be the list of all <span>click focusable</span> <span
data-x="focusable area">focusable areas</span> whose <span>DOM anchor</span> is a descendant
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

  2. 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.

  3. 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.

  4. 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?

Copy link
Contributor

@sefeng211 sefeng211 Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

of <var>focus target</var> in the <span>flat tree</span>.</p></li>

<li><p>Otherwise, let <var>possible focus delegates</var> be the list of all <span
data-x="focusable area">focusable areas</span> whose <span>DOM anchor</span> is a descendant
of <var>focus target</var> in the <span>flat tree</span>.</p></li>

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

<p class="note">For <span data-x="sequentially focusable">sequential focusability</span>, the
Expand All @@ -75029,6 +75030,36 @@ END:VCARD</pre>
<dd><p>Return null.</p></dd>
</dl>

<p>The <dfn>autofocus delegate</dfn> for a <var>focus target</var> given a <var>focus
trigger</var> is given by the following steps:</p>

<ol>
<li>
<p>For each <span>descendant</span> <var>descendant</var> of <var>focus target</var>, in
<span>tree order</span>:</p>

<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


<li><p>Let <var>focusable area</var> be <var>descendant</var>, if <var>descendant</var> is a
<span>focusable area</span>; otherwise let <var>focusable area</var> be the result of <span
data-x="get the focusable area">getting the focusable area</span> for <var>descendant</var>
given <var>focus trigger</var>.</p></li>

<li><p>If <var>focusable area</var> is null, then <span>continue</span>.</p></li>

<li><p>If <var>focusable area</var> is not <span>click focusable</span> and <var>focus
trigger</var> is "<code data-x="">click</code>", then <span>continue</span>.</p></li>

<li><p>Return <var>focusable area</var>.</p></li>
</ol>
</li>

<li><p>Return null.</p></li>
</ol>

<p>The <dfn export>focusing steps</dfn> for an object <var>new focus target</var> that is either a
<span>focusable area</span>, or an element that is not a <span>focusable area</span>, or a
<span>browsing context</span>, are as follows. They can optionally be run with a <var>fallback
Expand Down