Further fixes to focus delegation #7361

domenic · 2021-11-23T20:26:54Z

This ensures that focus delegation, used by both delegatesFocus and <dialog>, does not traverse into non-delegatesFocus shadow trees. Otherwise you would get unintuitive results in situations such as the following:

<div id="outer">
<#shadowroot delegatesFocus=true>
  <div id="inner>
  <#shadowroot delegatesFocus=false>
    <input>
  </#shadowroot>
  </div>
</#shadowroot>
</div>

In such a scenario, with the previous specification, inner.focus() would not focus anything, since inner's shadow root does not delegate focus so we never hit the "focus delegate" algorithm. But outer.focus() would focus the <input> element, since with it having delegatesFocus=true, we use the "focus delegate" algorithm, which would look at all shadow-including descendants. After this change, both inner.focus() and outer.focus() would not focus anything, since the <input> is hidden inside a delegatesFocus=false shadow tree.

Discovered in https://matrixlogs.bakkot.com/WHATWG/2021-11-23#L36-L62 by @emilio and @sefeng211. Review from them appreciated.

At least two implementers are interested (and none opposed):
- Chromium
- Firefox
Tests are written and can be reviewed and commented upon at:
- Expand and change dialog focus delegation tests web-platform-tests/wpt#31724
Implementation bugs are filed:
- I will update all the bugs from Handle shadow DOM and <dialog> focusing #7285 and Fixes to delegatesFocus #7079.

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

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

This ensures that focus delegation, used by both delegatesFocus and <dialog>, does not traverse into non-delegatesFocus shadow trees. Otherwise you would get unintuitive results in situations such as the following: <div id="outer"> <#shadowroot delegatesFocus=true> <div id="inner> <#shadowroot delegatesFocus=false> <input> </#shadowroot> </div> </#shadowroot> </div> In such a scenario, with the previous specification, inner.focus() would not focus anything, since inner's shadow root does not delegate focus so we never hit the "focus delegate" algorithm. But outer.focus() would focus the <input> element, since with it having delegatesFocus=true, we use the "focus delegate" algorithm, which would look at all shadow-including descendants. After this change, both inner.focus() and outer.focus() would not focus anything, since the <input> is hidden inside a delegatesFocus=false shadow tree. Discovered in https://matrixlogs.bakkot.com/WHATWG/2021-11-23#L36-L62.

Follows whatwg/html#7361.

mfreed7 · 2021-12-02T17:01:07Z

Completely drive-by, unrelated comment: your example could be written thusly, and it would work (at least in Chromium):

<div id="outer">
  <template shadowroot=open shadowrootdelegatesfocus=true>
    <div id="inner">
      <template shadowroot=open shadowrootdelegatesfocus=false>
        <input>
      </template>
    </div>
  </template>
</div>

domenic · 2022-01-05T23:02:52Z

Ping @emilio and/or @sefeng211 and/or @annevk for review :). Happy new year!

emilio

The effect of this looks good, though I find it would probably be simpler if this algorithm was written in terms of a shadow-including DOM traversal.

annevk

@emilio it's not clear to me the shadow-including terminology from DOM would help here. Potentially you could replace

Set root to root's host's root.

with

Set root to the first shadow-including ancestor of root that is a shadow root or document.

but that does not seem better? Or am I missing something?

annevk · 2022-01-07T12:10:58Z

And, happy new year to you as well @domenic!

emilio · 2022-01-07T15:16:10Z

You want to basically do a regular Dom traversal and dig only into "delegatesfocus" shadow roots, right? Would that not be equivalent?

domenic · 2022-01-07T18:00:22Z

I see what you mean. The current approach is trying to maximally reuse definitions, notably "shadow-including descendants" and "shadow-including tree-order". But doing so means we then have to check the ancestors.

Instead we could reimplement those definitions (i.e. do our own shadow-including depth-first preorder traversal), and then tweak the result to not traverse into non-delegatesfocus descendants. That might be cleaner indeed.

domenic · 2022-01-07T18:37:51Z

I rewrote this to do such a tree traversal. I think the result is actually a lot better, mainly because we can reuse the "get the focusable area" algorithm, which naturally does the recursion we need for tree traversal. It's also more precise than the previous wording of "focusable areas whose DOM anchors are shadow-including descendants", which seems like it could be ambiguous, or mismatch "get the focusable area", in some cases.

Unfortunately since it's basically a re-write this does need complete re-review. But I'm optimistic.

annevk · 2022-01-12T16:02:26Z

I think I lost the plot a bit, but this new algorithm first goes through descendants and then the shadow host's shadow tree, right? That means that in

div
- host
  - #shadow delegatesFocus
    - x autofocus
- y autofocus

y gets focus.

domenic · 2022-01-12T18:06:03Z

Yeah. autofocus is supposed to be "hidden" by shadow boundaries; you'd need to put autofocus on host in that case to make x get focus.

emilio

Looks good. It was a bit harder to review because apparently the PR diff links don't get updated?

emilio · 2022-01-18T16:34:12Z

D'oh, there are two sets of links (-:

Follows whatwg/html#7361.

…n tests, a=testonly Automatic update from web-platform-tests Expand and change dialog focus delegation tests Follows whatwg/html#7361. -- wpt-commits: 4b4b82fbede9d597d92aee1b934e33f4ac1dc971 wpt-pr: 31724

Follows whatwg/html#7361.

…n tests, a=testonly Automatic update from web-platform-tests Expand and change dialog focus delegation tests Follows whatwg/html#7361. -- wpt-commits: 4b4b82fbede9d597d92aee1b934e33f4ac1dc971 wpt-pr: 31724

This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Bug: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d

Fixes #8156. Fixes #8157. See also previous work in #7361, #7284, and #7079.

This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Bug: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d

This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Fixed: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d

This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 The gist of the changes are: 1. Use the DOM tree instead of the flat tree to search for an element to give initial focus. 2. Don't traverse into shadow roots when looking for elements to give initial focus unless the shadow root has delegatesFocus. This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Fixed: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d

This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 The gist of the changes are: 1. Use the DOM tree instead of the flat tree to search for an element to give initial focus. 2. Don't traverse into shadow roots when looking for elements to give initial focus unless the shadow root has delegatesFocus. This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Fixed: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067796}

This reverts commit afeccd1f5b2dd72595abe55bc9ef093f11b495f1. Reason for revert: dialog-focus-shadow.html is failing on a couple of mac builders: https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.13%20Tests/57992/overview https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.14%20Tests/28597/overview Original change's description: > Implement new dialog shadow focus behavior > > This implements the new dialog initial focus behavior specified in these > changes: > whatwg/html#7079 > whatwg/html#7284 > whatwg/html#7361 > whatwg/html#8174 > > The gist of the changes are: > 1. Use the DOM tree instead of the flat tree to search for an element to > give initial focus. > 2. Don't traverse into shadow roots when looking for elements to give > initial focus unless the shadow root has delegatesFocus. > > This will stay experimental until I have also made the other changes for > dialog initial focus: whatwg/html#8199 > After those changes have been made as well, I will carefully enable the > flag by default. > > Fixed: 383230, 670130, 1292852 > Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 > Reviewed-by: Mason Freed <masonf@chromium.org> > Commit-Queue: Joey Arhar <jarhar@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1067796} Change-Id: I010be9a77fd8c289edb8c028355182d0fe7dec39

This reverts commit afeccd1. Reason for revert: dialog-focus-shadow.html is failing on a couple of mac builders: https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.13%20Tests/57992/overview https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.14%20Tests/28597/overview Original change's description: > Implement new dialog shadow focus behavior > > This implements the new dialog initial focus behavior specified in these > changes: > whatwg/html#7079 > whatwg/html#7284 > whatwg/html#7361 > whatwg/html#8174 > > The gist of the changes are: > 1. Use the DOM tree instead of the flat tree to search for an element to > give initial focus. > 2. Don't traverse into shadow roots when looking for elements to give > initial focus unless the shadow root has delegatesFocus. > > This will stay experimental until I have also made the other changes for > dialog initial focus: whatwg/html#8199 > After those changes have been made as well, I will carefully enable the > flag by default. > > Fixed: 383230, 670130, 1292852 > Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 > Reviewed-by: Mason Freed <masonf@chromium.org> > Commit-Queue: Joey Arhar <jarhar@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1067796} Change-Id: I010be9a77fd8c289edb8c028355182d0fe7dec39 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4005547 Commit-Queue: Tim Sergeant <tsergeant@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Tim Sergeant <tsergeant@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Tim Sergeant <tsergeant@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067950}

This reverts commit afeccd1f5b2dd72595abe55bc9ef093f11b495f1. Reason for revert: dialog-focus-shadow.html is failing on a couple of mac builders: https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.13%20Tests/57992/overview https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.14%20Tests/28597/overview Original change's description: > Implement new dialog shadow focus behavior > > This implements the new dialog initial focus behavior specified in these > changes: > whatwg/html#7079 > whatwg/html#7284 > whatwg/html#7361 > whatwg/html#8174 > > The gist of the changes are: > 1. Use the DOM tree instead of the flat tree to search for an element to > give initial focus. > 2. Don't traverse into shadow roots when looking for elements to give > initial focus unless the shadow root has delegatesFocus. > > This will stay experimental until I have also made the other changes for > dialog initial focus: whatwg/html#8199 > After those changes have been made as well, I will carefully enable the > flag by default. > > Fixed: 383230, 670130, 1292852 > Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 > Reviewed-by: Mason Freed <masonf@chromium.org> > Commit-Queue: Joey Arhar <jarhar@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1067796} Change-Id: I010be9a77fd8c289edb8c028355182d0fe7dec39 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4005547 Commit-Queue: Tim Sergeant <tsergeant@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Tim Sergeant <tsergeant@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Tim Sergeant <tsergeant@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067950}

This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 The gist of the changes are: 1. Use the DOM tree instead of the flat tree to search for an element to give initial focus. 2. Don't traverse into shadow roots when looking for elements to give initial focus unless the shadow root has delegatesFocus. This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Fixed: 383230, 670130, 1292852 Change-Id: If024ab0f22716c55b439e44b2298e39897ee844e

…or, a=testonly Automatic update from web-platform-tests Implement new dialog shadow focus behavior This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 The gist of the changes are: 1. Use the DOM tree instead of the flat tree to search for an element to give initial focus. 2. Don't traverse into shadow roots when looking for elements to give initial focus unless the shadow root has delegatesFocus. This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Fixed: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067796} -- wpt-commits: e1d2b674fbcc1ca01eccb624fdd758c38f3407a7 wpt-pr: 36498

…s behavior", a=testonly Automatic update from web-platform-tests Revert "Implement new dialog shadow focus behavior" This reverts commit afeccd1f5b2dd72595abe55bc9ef093f11b495f1. Reason for revert: dialog-focus-shadow.html is failing on a couple of mac builders: https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.13%20Tests/57992/overview https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.14%20Tests/28597/overview Original change's description: > Implement new dialog shadow focus behavior > > This implements the new dialog initial focus behavior specified in these > changes: > whatwg/html#7079 > whatwg/html#7284 > whatwg/html#7361 > whatwg/html#8174 > > The gist of the changes are: > 1. Use the DOM tree instead of the flat tree to search for an element to > give initial focus. > 2. Don't traverse into shadow roots when looking for elements to give > initial focus unless the shadow root has delegatesFocus. > > This will stay experimental until I have also made the other changes for > dialog initial focus: whatwg/html#8199 > After those changes have been made as well, I will carefully enable the > flag by default. > > Fixed: 383230, 670130, 1292852 > Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 > Reviewed-by: Mason Freed <masonf@chromium.org> > Commit-Queue: Joey Arhar <jarhar@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1067796} Change-Id: I010be9a77fd8c289edb8c028355182d0fe7dec39 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4005547 Commit-Queue: Tim Sergeant <tsergeant@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Tim Sergeant <tsergeant@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Tim Sergeant <tsergeant@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067950} -- wpt-commits: 011077556f5634fe685c613564c3325e77387462 wpt-pr: 36838

…or, a=testonly Automatic update from web-platform-tests Implement new dialog shadow focus behavior This implements the new dialog initial focus behavior specified in these changes: whatwg/html#7079 whatwg/html#7284 whatwg/html#7361 whatwg/html#8174 The gist of the changes are: 1. Use the DOM tree instead of the flat tree to search for an element to give initial focus. 2. Don't traverse into shadow roots when looking for elements to give initial focus unless the shadow root has delegatesFocus. This will stay experimental until I have also made the other changes for dialog initial focus: whatwg/html#8199 After those changes have been made as well, I will carefully enable the flag by default. Fixed: 383230, 670130, 1292852 Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067796} -- wpt-commits: e1d2b674fbcc1ca01eccb624fdd758c38f3407a7 wpt-pr: 36498

…s behavior", a=testonly Automatic update from web-platform-tests Revert "Implement new dialog shadow focus behavior" This reverts commit afeccd1f5b2dd72595abe55bc9ef093f11b495f1. Reason for revert: dialog-focus-shadow.html is failing on a couple of mac builders: https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.13%20Tests/57992/overview https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.14%20Tests/28597/overview Original change's description: > Implement new dialog shadow focus behavior > > This implements the new dialog initial focus behavior specified in these > changes: > whatwg/html#7079 > whatwg/html#7284 > whatwg/html#7361 > whatwg/html#8174 > > The gist of the changes are: > 1. Use the DOM tree instead of the flat tree to search for an element to > give initial focus. > 2. Don't traverse into shadow roots when looking for elements to give > initial focus unless the shadow root has delegatesFocus. > > This will stay experimental until I have also made the other changes for > dialog initial focus: whatwg/html#8199 > After those changes have been made as well, I will carefully enable the > flag by default. > > Fixed: 383230, 670130, 1292852 > Change-Id: I13995197f1942aa356cab0f3b41a0e226d1d170d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961528 > Reviewed-by: Mason Freed <masonf@chromium.org> > Commit-Queue: Joey Arhar <jarhar@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1067796} Change-Id: I010be9a77fd8c289edb8c028355182d0fe7dec39 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4005547 Commit-Queue: Tim Sergeant <tsergeant@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Tim Sergeant <tsergeant@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Tim Sergeant <tsergeant@chromium.org> Cr-Commit-Position: refs/heads/main@{#1067950} -- wpt-commits: 011077556f5634fe685c613564c3325e77387462 wpt-pr: 36838

domenic added topic: shadow Relates to shadow trees (as defined in DOM) topic: focus topic: dialog The <dialog> element. labels Nov 23, 2021

domenic added a commit to web-platform-tests/wpt that referenced this pull request Nov 23, 2021

Expand and change dialog focus delegation tests

8fc838b

Follows whatwg/html#7361.

domenic mentioned this pull request Nov 23, 2021

Expand and change dialog focus delegation tests web-platform-tests/wpt#31724

Merged

emilio approved these changes Jan 6, 2022

View reviewed changes

domenic requested a review from annevk January 6, 2022 18:16

annevk approved these changes Jan 7, 2022

View reviewed changes

Rewrite as tree traversal

b324375

domenic requested review from emilio and annevk January 7, 2022 18:37

emilio approved these changes Jan 18, 2022

View reviewed changes

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2022

Expand and change dialog focus delegation tests

4b4b82f

Follows whatwg/html#7361.

domenic merged commit d1fca22 into main Jan 31, 2022

domenic deleted the more-delegatesfocus-fixes branch January 31, 2022 16:15

mattwoodrow pushed a commit to mattwoodrow/wpt that referenced this pull request Feb 15, 2022

Expand and change dialog focus delegation tests

32c606c

Follows whatwg/html#7361.

DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this pull request Feb 28, 2022

Expand and change dialog focus delegation tests

7f64dda

Follows whatwg/html#7361.

domenic mentioned this pull request Aug 9, 2022

Yet more fixes to focus delegation #8174

Merged

3 tasks

chromium-wpt-export-bot mentioned this pull request Oct 17, 2022

Implement new dialog shadow focus behavior web-platform-tests/wpt#36498

Merged

domenic added a commit that referenced this pull request Oct 22, 2022

Yet more fixes to focus delegation

4d6609f

Fixes #8156. Fixes #8157. See also previous work in #7361, #7284, and #7079.

chromium-wpt-export-bot mentioned this pull request Nov 7, 2022

Revert "Implement new dialog shadow focus behavior" web-platform-tests/wpt#36838

Merged

chromium-wpt-export-bot mentioned this pull request Nov 7, 2022

Reland "Implement new dialog shadow focus behavior" web-platform-tests/wpt#36851

Merged

josepharhar mentioned this pull request Aug 26, 2023

Dialog initial focus when dialog contains a slot element #9640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further fixes to focus delegation #7361

Further fixes to focus delegation #7361

domenic commented Nov 23, 2021 •

edited by pr-preview bot

Loading

mfreed7 commented Dec 2, 2021

domenic commented Jan 5, 2022

emilio left a comment

annevk left a comment

annevk commented Jan 7, 2022

emilio commented Jan 7, 2022

domenic commented Jan 7, 2022

domenic commented Jan 7, 2022

annevk commented Jan 12, 2022

domenic commented Jan 12, 2022

emilio left a comment

emilio commented Jan 18, 2022

Further fixes to focus delegation #7361

Further fixes to focus delegation #7361

Conversation

domenic commented Nov 23, 2021 • edited by pr-preview bot Loading

mfreed7 commented Dec 2, 2021

domenic commented Jan 5, 2022

emilio left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

annevk commented Jan 7, 2022

emilio commented Jan 7, 2022

domenic commented Jan 7, 2022

domenic commented Jan 7, 2022

annevk commented Jan 12, 2022

domenic commented Jan 12, 2022

emilio left a comment

Choose a reason for hiding this comment

emilio commented Jan 18, 2022

domenic commented Nov 23, 2021 •

edited by pr-preview bot

Loading