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.

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

Don't change focus when re-focusing delegatesFocus shadowhost #8453

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Oct 29, 2022

I was implementing the new dialog shadowdom initial focus goodness when I noticed this in chromium:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc;l=4714-4718;drc=82b10d45463bbc3d019f5ef981dc8afeb8900a6d

When getting the focusable area of a target, in the case that the target is a shadow-incuding inclusive ancestor of the currently focused element, the spec says to return null, but chromium's implementation returns the currently focused element instead and cites this issue: WICG/webcomponents#840

I'm not certain exactly what the consensus was, but I do see that this spec PR and test was made in response:
#5039
web-platform-tests/wpt#19867 The added test doesn't fail if I return null like the spec says to do, but dialog-focus-shadow-double-nested.html does fail if I return null with this error message:

FAIL showModal() assert_equals: document.activeElement expected Element node <div></div> but got Element node <button tabindex="-1">Focusable</button>

Based on this test failure and reading the WICG issue, I believe that we should be returning the currently focused element, which this patch does.

  • At least two implementers are interested (and none opposed):
    • Chrome
    • We can assume others are implemented as this is just a bug fix to previous work that we've agreed on.
  • Tests are written and can be reviewed and commented upon at:
    • dialog-focus-shadow-double-nested.html
  • Implementation bugs are filed:
  • MDN issue is filed: N/A

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


/interaction.html ( diff )

I was implementing the new dialog shadowdom initial focus goodness when
I noticed this in chromium:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc;l=4714-4718;drc=82b10d45463bbc3d019f5ef981dc8afeb8900a6d

When getting the focusable area of a target, in the case that the
target is a shadow-incuding inclusive ancestor of the currently focused
element, the spec says to return null, but chromium's implementation
returns the currently focused element instead and cites this issue:
WICG/webcomponents#840

I'm not certain exactly what the consensus was, but I do see that this
spec PR and test was made in response:
whatwg#5039
web-platform-tests/wpt#19867
The added test doesn't fail if I return null like the spec says to do,
but dialog-focus-shadow-double-nested.html does fail if I return null
with this error message:
```
FAIL showModal() assert_equals: document.activeElement expected Element node <div></div> but got Element node <button tabindex="-1">Focusable</button>
```

Based on this test failure and reading the WICG issue, I believe that we
should be returning the currently focused element, which this patch
does.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thank you for the detective work and the neat patch! I'll merge this since it looks like a straightforward bug, and as you noted, there are tests for this case.

Deal with browsing context -> traversables
@domenic domenic added topic: shadow Relates to shadow trees (as defined in DOM) topic: focus topic: dialog The <dialog> element labels Nov 1, 2022
@domenic domenic merged commit 5ded2d8 into whatwg:main Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: dialog The <dialog> element topic: focus topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

2 participants