CSS: :focus selector effects on shadow hosts #17493
Conversation
There was a problem hiding this comment.
I am a bit worried how these tests share state between them. It would be easier to understand if they re-created entire trees each test so there were no cross-test dependencies. For example you could have factories for shadowRootDelegatesFocus and shadowRootNonDelegatesFocus.
Also although the current style is fine I'll note that you can also test el.matches(":focus") which seem a bit more direct.
|
Also I am wondering if CSS or components folks (e.g. @rniwa) have any opinions on whether this should be in the css/selectors directory or the shadow-dom directory or even the HTML focus directory. I am fine leaving it with the css/selectors directory myself but perhaps others have stronger preferences. (This does not block merging as we can always move it later.) |
|
I would've preferred to have this in shadow-dom directory given |
Yeah that makes more sense - factored them out.
Oh hey didn't know something like that exists! thanks :)
Moved to shadow-dom. |
|
Alright, verified that if we implement what's being spec'ed in whatwg/html#4731, these test cases would pass, and they currently fail in WebKit. |
rakina
force-pushed
the
focusselectordelegatesFocus
branch
from
950005c to
bfdf9b8
Compare
rakina
changed the title
|
Updated the tests to expect |
| @@ -60,8 +60,13 @@ function removeTabIndex(elements) { | |||
| } | |||
| } | |||
|
|
|||
| function resetFocusOnRoot(root) { | |||
There was a problem hiding this comment.
Can we just have one function, instead of two?
There was a problem hiding this comment.
resetFocus() is already used by the sequential focus navigation tests so we still need it (unless we change it too I guess?), and we need to give the root here because we don't know the tree structure?
There was a problem hiding this comment.
I see, I think I misread things. I'm OK with merging this as-is. However it could be clearer to just have function resetFocus(root = document) { /* contents of current resetFocusOnRoot */ } and update all tests to use that.
There was a problem hiding this comment.
Oh yeah sorry the non-document one is not actually used in this WPT, oops (but will be used in other ones so I think it's OK to have the general version here). Also changed it to your suggested version. Thanks!
Part of #2013. Note that the delegates focus flag is not consulted. Tests: web-platform-tests/wpt#17493
| slotted.focus(); | ||
| assert_true(slotted.matches(":focus"), "slotted element matches :focus"); | ||
| assert_true(host.matches(":focus"), "host matches :focus"); | ||
| }, `:focus applies to host with delegatesFocus=${delegatesFocus} when slotted element has focus`); |
There was a problem hiding this comment.
This is wrong. If a slotted element has a focus, then the shadow host is its parent (or ancestor). If :focus shouldn't match both the slotted element and the host of the shadow tree to which it was assigned.
I'm fixing this in #19530
Part of #2013. Note that the delegates focus flag is not consulted. Tests: web-platform-tests/wpt#17493
See whatwg/html#4731 for the spec change. PTAL :)