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

Handle shadow DOM and <dialog> focusing #7285

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Nov 1, 2021

Closes #2393, by making behave the same as delegatesFocus: they both "delegate focus" to either autofocus="" if it's specified on a (non-shadow-including) descendant, or the first focusable element among their shadow-including descendants if not.

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


/interaction.html ( diff )
/interactive-elements.html ( diff )

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

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Looks good, but I guess root needs to be initialized to focus target's shadow root (if there is one?), or perhaps it needs to be replaced with focus target.

source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Nov 2, 2021

cc @sefeng211 @emilio

Base automatically changed from another-focus-fixup to main November 2, 2021 14:50
Closes #2393, by making <dialog> behave the same as delegatesFocus: they both "delegate focus" to either autofocus="" if it's specified on a (non-shadow-including) descendant, or the first focusable element among their shadow-including descendants if not.
@sefeng211
Copy link
Contributor

Looks good to me!

domenic added a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2021
@domenic
Copy link
Member Author

domenic commented Nov 5, 2021

I created tests for this at web-platform-tests/wpt#31523. Both Firefox and Chrome fail them in interesting and different ways. It's possible I made a mistake when writing them, so careful review appreciated.

It'd be great if @sefeng211 and @mfreed7 could review those tests to make sure the results expected there are good. Then we can proceed to merge this/merge the tests/file bugs.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2021
@domenic
Copy link
Member Author

domenic commented Nov 15, 2021

Ping @sefeng211 @mfreed7 to review the tests in web-platform-tests/wpt#31523 , and @mfreed7 to confirm this looks like a reasonable route from Chromium's perspective.

@josepharhar
Copy link
Contributor

After seeing the crbug from the attached issue, is the basic idea that we will focus the first focusable item even if its in shadow DOM, which chrome doesn't do right now?

@domenic
Copy link
Member Author

domenic commented Nov 18, 2021

The basic idea is what's outlined in the OP: focus either an element with autofocus="" if autofocus="" is specified on a (non-shadow-including) descendant, or focus the first focusable element among their shadow-including descendants if there is no such autofocus="".

So, yes, but also with autofocus="" interaction.

descendant</span> of <var>focus target</var>.</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
Copy link
Contributor

Choose a reason for hiding this comment

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

This "DOM anchor" span doesn't have data-x="DOM anchor", but the "DOM anchor" span below has data-x="DOM anchor". Does it make a difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's necessary below because the actual text is DOM anchors with a plural "s". Here it can be inferred based on the text content. https://github.com/whatwg/wattsi/blob/main/Syntax.md for more.

@josepharhar
Copy link
Contributor

I may have followup questions when I actually implement the behavior in chrome, but LGTM!

@domenic
Copy link
Member Author

domenic commented Nov 18, 2021

Awesome, thanks for the review; I'll merge this and the tests now.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Nov 18, 2021
@domenic domenic merged commit 32a94ad into main Nov 18, 2021
@domenic domenic deleted the dialog-focusing-shadow-dom branch November 18, 2021 18:13
@domenic domenic mentioned this pull request Nov 23, 2021
3 tasks
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 30, 2021
…only

Automatic update from web-platform-tests
Test <dialog> + shadow DOM focus

Follows whatwg/html#7285.
--

wpt-commits: 628af0fd55db95d785a63484882998343e3a069e
wpt-pr: 31523
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 30, 2021
…only

Automatic update from web-platform-tests
Test <dialog> + shadow DOM focus

Follows whatwg/html#7285.
--

wpt-commits: 628af0fd55db95d785a63484882998343e3a069e
wpt-pr: 31523
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.

Define focus behavior for <dialog> element with Shadow DOM
4 participants