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

Don't delegate focus when the shadow host is an ancestor of the currently focused area #5039

Merged
merged 4 commits into from
Oct 28, 2019
Merged

Don't delegate focus when the shadow host is an ancestor of the currently focused area #5039

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc82525
Merge pull request #1 from whatwg/master
rakina Oct 24, 2019
11e8717
Don't delegate focus when the shadow host is an ancestor of the curre…
rakina Oct 24, 2019
a96a60e
Nest a bit more
domenic Oct 24, 2019
f76a1f5
Also found a nearby let/set confusion, why not fix it while we're here
domenic Oct 24, 2019
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
34 changes: 23 additions & 11 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74003,7 +74003,7 @@ END:VCARD</pre>

<dd>

<p>Let <var>new focus target</var> be the <code>Document</code>'s <span>viewport</span>.</p>
<p>Set <var>new focus target</var> to the <code>Document</code>'s <span>viewport</span>.</p>

</dd>

Expand Down Expand Up @@ -74034,18 +74034,30 @@ END:VCARD</pre>

<dd>

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

Choose a reason for hiding this comment

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

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.

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

Copy link

Choose a reason for hiding this comment

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

Oh, you're right.

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.</li>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!


<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>new focus target</var> in the <span>flat tree</span>.</p>
<li>
<p>Otherwise:</p>

<p>Set <var>new focus target</var> to 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.</p>
<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>new 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>new focus target</var> in the <span>flat tree</span>.</li>

<li>Set <var>new focus target</var> to 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>
</ol>

<p class="note">For <span data-x="sequentially focusable">sequential focusability</span>, the
handling of <span data-x="shadow host">shadow hosts</span> and <span>delegates focus</span> is
Expand Down