Yet more fixes to focus delegation #8174

domenic · 2022-08-09T08:51:19Z

Fixes #8156. Fixes #8157.

This is the latest in a series of PRs as we slowly iterate toward correct algorithms for these. For previous work in the series, see #7361, #7284, #7079.

/cc @emilio, @nt1m, @rniwa, @smaug----, @josepharhar as people who seem to be working on these areas in implementations. @emilio can you review in particular?

At least two implementers are interested (and none opposed):
- Chrome is interested
- @emilio filed these bugs and I think I can assume he's interested
- In general these just seem like oversights and I suspect everyone is on board with fixing them.
Tests are written and can be reviewed and commented upon at:
- [Gecko Bug 1741936] Implement focus delegate algorithm web-platform-tests/wpt#36449
- Implement new dialog shadow focus behavior web-platform-tests/wpt#36498
Implementation bugs are filed:
- See bugs filed in Fixes to delegatesFocus #7079.

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

/interaction.html ( diff )

Fixes #8156. Fixes #8157.

emilio

Looks correct to me now. I'm not convinced tests cover nested delegateFocus and so on, but I haven't looked super-closely.

nt1m · 2022-08-10T16:10:18Z

Regarding the tests, we implemented proper focus delegation for dialogs recently in WebKit, and we found some failures, where we think the test might be incorrect:

https://searchfox.org/wubkat/rev/3728a98461ce1ad0e74b7a416e823bd17ae6872e/LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-expected.txt

For the after sibling test, @rniwa and I don't understand why we expect the outside <button> to be focused, and not one of the elements in shadow root.

The implementation was done: WebKit/WebKit#2828

josepharhar · 2022-09-01T23:05:35Z

I think some of the test cases in dialog-focus-shadow.html seem contradictory now that I've finally gotten around to implementing all this dialog shadowdom focus stuff:

<dialog data-description="No autofocus, yes delegatesFocus, sibling after">
  <template class="turn-into-shadow-tree delegates-focus">
    <button disabled>Non-focusable</button>
    <button tabindex="-1" class="focus-me">Focusable</button>
    <button disabled>Non-focusable</button>
  </template>
  <button tabindex="-1">Focusable</button>
</dialog>

<dialog data-description="Autofocus inside shadow tree, yes delegatesFocus, sibling after">
  <template class="turn-into-shadow-tree delegates-focus">
    <button tabindex="-1">Focusable</button>
    <button tabindex="-1" autofocus>Focusable</button>
    <button disabled>Non-focusable</button>
  </template>
  <button tabindex="-1" class="focus-me">Focusable</button>
</dialog>

If I'm reading this correctly, the only difference between the tests is that the second one has the autofocus attribute on the element which should be focused according to the first test, but that causes a different element outside the shadowroot to be focused instead? Shouldn't the element with autofocus be focused in the second test? I made a patch for this here: https://chromium-review.googlesource.com/c/chromium/src/+/3866548/1/third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow.html

For the after sibling test, @rniwa and I don't understand why we expect the outside to be focused, and not one of the elements in shadow root.

@nt1m I'm not sure if this is the same issue, can you confirm?

josepharhar · 2022-10-10T22:54:32Z

I think we also need tests for potentially focusable elements being slotted into shadowroots in the dialog. I can propose some behavior and provide test cases like I did in my last comment soon.

sefeng211 · 2022-10-13T14:28:43Z

I am working on the Firefox implementation, and I agree with #8174 (comment) from Joey.

domenic · 2022-10-16T12:55:47Z

Thanks @sefeng211! Do you have a link to the tests you've written handy? And if so, do you believe those tests cover both the listed issues? If so we're ready to merge this PR.

annevk

Looks good editorially.

sefeng211 · 2022-10-17T15:29:42Z

@domenic Yeah, I fixed them and it's web-platform-tests/wpt#36449. I need to figure out why it's not automatically merged.

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

josepharhar · 2022-10-17T16:28:34Z

Ah I also just finished my test changes here: web-platform-tests/wpt#36498
I'm happy to add my changes after yours are merged, it sounds like they should be compatible

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

Yet more fixes to focus delegation

a93d242

Fixes #8156. Fixes #8157.

domenic added topic: shadow Relates to shadow trees (as defined in DOM) topic: focus labels Aug 9, 2022

emilio approved these changes Aug 10, 2022

View reviewed changes

past mentioned this pull request Aug 19, 2022

Upcoming HTML standard issue triage meeting on 8/18/2022 (new time!) #8162

Closed

domenic mentioned this pull request Aug 19, 2022

Implement dialog initial focus proposal #8199

Merged

3 tasks

josepharhar mentioned this pull request Oct 10, 2022

Add popover attribute #8221

Merged

3 tasks

domenic requested a review from annevk October 16, 2022 12:55

annevk approved these changes Oct 17, 2022

View reviewed changes

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

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

Merged

domenic merged commit 4d6609f into main Oct 22, 2022

domenic deleted the focus-delegate-fixes branch October 22, 2022 06:28

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

Yet more fixes to focus delegation #8174

Yet more fixes to focus delegation #8174

domenic commented Aug 9, 2022 •

edited by pr-preview bot

Loading

emilio left a comment

nt1m commented Aug 10, 2022

josepharhar commented Sep 1, 2022

josepharhar commented Oct 10, 2022

sefeng211 commented Oct 13, 2022

domenic commented Oct 16, 2022

annevk left a comment

sefeng211 commented Oct 17, 2022

josepharhar commented Oct 17, 2022

Yet more fixes to focus delegation #8174

Yet more fixes to focus delegation #8174

Conversation

domenic commented Aug 9, 2022 • edited by pr-preview bot Loading

emilio left a comment

Choose a reason for hiding this comment

nt1m commented Aug 10, 2022

josepharhar commented Sep 1, 2022

josepharhar commented Oct 10, 2022

sefeng211 commented Oct 13, 2022

domenic commented Oct 16, 2022

annevk left a comment

Choose a reason for hiding this comment

sefeng211 commented Oct 17, 2022

josepharhar commented Oct 17, 2022

domenic commented Aug 9, 2022 •

edited by pr-preview bot

Loading