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

Add shadow host consideration for :focus selector #4731

Merged
merged 10 commits into from Sep 24, 2019

Conversation

@rakina
Copy link
Contributor

commented Jun 25, 2019

A bit related to #2013 and whatwg/dom#768. Will send a separate PR that updates the sequential navigation parts later.

Tests: web-platform-tests/wpt#17493


/acknowledgements.html ( diff )
/infrastructure.html ( diff )
/semantics-other.html ( diff )

source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

left a comment

Editorial comments aside, I think this works. The tests in https://github.com/web-platform-tests/wpt/pull/17493/files seem correct, and here is my walkthrough of a couple of them:

Delegated

<div id="outer">
  #shadow delegatesFocus = true
  <div id="mid">
    #shadow delegatesFocus = true
      <div id="inner" tabindex="0">
  
inner.focus()
  • inner has the focus because it is the currently focused area of the top-level browsing context
  • mid has the focus because its shadow root is not null, its shadow root's delegates focus is true, and its shadow root is the root of inner
  • outer has the focus because its shadow root is not null, its shadow root's delegates focus is true, and its shadow root is the root of mid

No delegation

<div id="outer">
  #shadow delegatesFocus = true
  <div id="mid">
    #shadow delegatesFocus = false
      <div id="inner" tabindex="0">
  
inner.focus()
  • inner has the focus because it is one of the elements listed in the focus chain of the currently focused area of the top-level browsing context
  • mid does not have the focus, because its shadow root's delegates focus is false, and it is not listed in the focus chain of inner. (As Rakian says, focus chains basically only contain the parent BC container elements)
  • outer does not have the focus because it is not the root of any element that has the focus, and it is not listed in the focus chain of inner.
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

left a comment

LGTM with nits. (If we were in different timezones I would fix them myself to avoid overnight latency, but since we are in the same timezone, I will let you fix them. Lucky you ;).)

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
rakina and others added 2 commits Jun 27, 2019
@annevk

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Is there no interaction with the composed tree here? In particular if there's a focused slotted element nothing encompassing the slot in the shadow tree gets/has to know about this? (I'm not entirely sure what the answer should be, but it would be good to add another test for this scenario as I only see it being covered that the host does not get focus, which does seem correct from an encapsulation perspective.)

@rakina

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Is there no interaction with the composed tree here? In particular if there's a focused slotted element nothing encompassing the slot in the shadow tree gets/has to know about this?

Yes, if a slotted element is focused, nothing in the shadow tree gets to know about it, since it's not actually in the shadow tree.

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

So if you have a focusable div in a shadow tree that contains a slot through which you slot another focusable div and you focus the latter, the focusable div in the shadow tree won't be focused? I guess that makes sense given how nested focus structures work outside shadow trees, but it'd be good to add asserts.

@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

So if you have a focusable div in a shadow tree that contains a slot through which you slot another focusable div and you focus the latter, the focusable div in the shadow tree won't be focused?

shadowRoot.activeElement would still find the slot, and :focus-within would match the slot. Conceptually, :focus only applies to the very element which is the focus anchor at that moment.

But now thinking about this more carefully, this may leak the existence of shadow root in that if there is a focused element inside a shadow tree, document.activeElement and shadowRoot.activeElement would still refer to its shadow host. If :focus is false on that shadow host, it basically means the element has a shadow root.

See w3c/webcomponents#804 for some discussions.

But perhaps we should first spec the status quo and fix it as an independent effort.

@domenic domenic self-assigned this Jul 2, 2019
Copy link
Member

left a comment

I may be misreading, but I think the PR works in the following way:

<div id="outer">
  #shadow delegatesFocus = true
    <div id="inner1" tabindex="0">
    <div id="inner2" tabindex="0">
 
 inner2.focus();
 inner1.matches(":focus");

This seems a bit strange. Is this intentional? I don't think I see any tests for it.


Draft commit message:

Handle elements inside delegatesFocus shadow trees for :focus

Part of #2013.

Tests: https://github.com/web-platform-tests/wpt/pull/17493
source Show resolved Hide resolved
@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

The current PR would mean that :focus would match whenever a shadow tree contains an element and the shadow root itself has delegatesFocus. I still think that might be bad and we may want to make :focus always apply whenever element's shadow root contains a focused element.

However, the current PR matches the existing implementation (Chrome) and other implementations (WebKit & Gecko) don't match :focus on these cases so it's probably okay to keep this PR as is, and approach this issue in a separate PR.

@domenic

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Thanks. I agree with that approach and see no problems with revisiting the issue going forward.

Can you confirm if WebKit still opposes merging this PR, and its dependency of whatwg/dom#768, or has that changed since whatwg/dom#768 (comment) ?

Copy link
Collaborator

left a comment

There is no reasoning for this change.

@rakina

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Why would delegatesFocus affect to :focus in any way? Feels really weird.

As rniwa hinted, we shouldn't expose the existence of shadowroot, so host should get :focus when it shows up as the .activeElement in its context.

It does feel a little weird for the behavior to differ between hosts with delegateFocus vs not. I'm not quite sure about the past reasoning for this behavior, maybe @hayatoito knows more?

Would you rather this PR change the behavior so that both hosts with & without delegatesFocus will match :focus when an element in its shadow tree is focused then?

@rakina rakina changed the title Add delegatesFocus consideration for :focus selector Add shadow host consideration for :focus selector Sep 20, 2019
@rakina

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

This was discussed in TPAC web components session, and we decided to make the host match :focus too if it shadow tree contains a focused element. Updated the spec part here, will update the WPT PR too.

Copy link
Member

left a comment

This matches the TPAC consensus, and has tests; merging!

@domenic domenic dismissed smaug----’s stale review Sep 24, 2019

Changes addressed at TPAC

@domenic domenic merged commit 500f4fe into whatwg:master Sep 24, 2019
2 checks passed
2 checks passed
Participation rakina participates on behalf of Google LLC
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
domenic added a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2019
@annevk

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Can someone file the relevant implementation bugs too?

@domenic

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

My plan was to late until the last PR in the series landed, which should be today

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 26, 2019
…osts, a=testonly

Automatic update from web-platform-tests
CSS: :focus selector effects on shadow hosts

Follows whatwg/html#4731.
--

wpt-commits: 0693b81665fcc5a7258535e4403b748a810730ae
wpt-pr: 17493
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Sep 27, 2019
…osts, a=testonly

Automatic update from web-platform-tests
CSS: :focus selector effects on shadow hosts

Follows whatwg/html#4731.
--

wpt-commits: 0693b81665fcc5a7258535e4403b748a810730ae
wpt-pr: 17493
@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2019

@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2019

It's unfortunate that this got landed with a test (shadow-dom/focus/focus-selector-delegatesFocus.html), which relies on delegatesFocus. Also, it doesn't seem to test enough edge cases. I'll add more tests in my WebKit patch.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…osts, a=testonly

Automatic update from web-platform-tests
CSS: :focus selector effects on shadow hosts

Follows whatwg/html#4731.
--

wpt-commits: 0693b81665fcc5a7258535e4403b748a810730ae
wpt-pr: 17493

UltraBlame original commit: 319b49b753b936cafa788a116ebd3707d9dad2de
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…osts, a=testonly

Automatic update from web-platform-tests
CSS: :focus selector effects on shadow hosts

Follows whatwg/html#4731.
--

wpt-commits: 0693b81665fcc5a7258535e4403b748a810730ae
wpt-pr: 17493

UltraBlame original commit: 319b49b753b936cafa788a116ebd3707d9dad2de
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 5, 2019
…osts, a=testonly

Automatic update from web-platform-tests
CSS: :focus selector effects on shadow hosts

Follows whatwg/html#4731.
--

wpt-commits: 0693b81665fcc5a7258535e4403b748a810730ae
wpt-pr: 17493

UltraBlame original commit: 319b49b753b936cafa788a116ebd3707d9dad2de
@rakina

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

It's unfortunate that this got landed with a test (shadow-dom/focus/focus-selector-delegatesFocus.html), which relies on delegatesFocus. Also, it doesn't seem to test enough edge cases. I'll add more tests in my WebKit patch.

It tests both values for delegatesFocus though? Do you mean it might not work in some browsers because they don't actually have that option? I thought they would just ignore that in that case - if not, oops, sorry D:

@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2019

Well, delegatesFocus is a new feature not implemented in any browser but Chrome whereas :focus and shadow DOM is a feature implemented by all browsers. Ideally, these things are independently of one another so that the test for :focus doesn’t depend on delegatesFocus.

@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2019

Alright, this new behavior has been implemented as of https://trac.webkit.org/changeset/250788.

@domenic

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

The test doesn't behave on it. It in fact explicitly tests that delegatesFocus has no effect on the behavior.

@rniwa

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2019

The test doesn't behave on it. It in fact explicitly tests that delegatesFocus has no effect on the behavior.

The fact it tests whether the behavior of it depends on delegatesFocus is the problem. Since it's a new feature, there should have been a separate test that doesn't test or mention anything about delegatesFocus.

@domenic

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Ok, we'll have to agree to disagree.

pmatos pushed a commit to pmatos/WebKit that referenced this pull request Oct 8, 2019
…ins the focused element

https://bugs.webkit.org/show_bug.cgi?id=202432

Reviewed by Antti Koivisto.

Source/WebCore:

Note that focus pseudo class does not match a shadow host when its shadow tree contains a slot element
with a focused element or its ancestor assigned since such a shadow host has the actual focused element
in the same tree as the shadow host. (e.g. the focused element can be a direct child of the host).

In order to preserve the behavior of focus ring, which should be only drawn on the currently focused
element and not any shadow host which contains such an element, this patch introduces a new pseudo class,
-webkit-direct-focus, which is only available in the user agent stylesheet. Putting :host(:focus) rule
isn't sufficient because style rules inside shadow trees always has a lower precendence than rules
outside the shadow tree (the tree of its shadow host).

[1] Also see whatwg/html#4731

Tests: fast/shadow-dom/direct-focus-pseudo-does-not-match-in-author-stylesheet.html
       fast/shadow-dom/focus-pseudo-matches-on-shadow-host.html
       fast/shadow-dom/focus-pseudo-on-shadow-host-1.html
       fast/shadow-dom/focus-pseudo-on-shadow-host-2.html
       fast/shadow-dom/focus-pseudo-on-shadow-host-3.html

* css/CSSSelector.cpp:
(WebCore::CSSSelector::selectorText const): Added the support for -webkit-direct-focus.
* css/CSSSelector.h:
* css/RuleSet.cpp:
(WebCore::RuleSet::addRule): Ditto.
* css/SelectorChecker.cpp:
(WebCore::SelectorChecker::checkOne const):: Ditto.
(WebCore::doesShadowTreeContainFocusedElement):: Ditto.
(WebCore::SelectorChecker::matchesFocusPseudoClass): Implemented the new behavior.
(WebCore::SelectorChecker::matchesDirectFocusPseudoClass): Added. Implements the old behavior for
the focus ring via -webkit-direct-focus pseudo class.
* css/SelectorChecker.h:
* css/SelectorPseudoClassAndCompatibilityElementMap.in: Added -webkit-direct-focus.
* css/html.css: Use -webkit-direct-focus pseudo class to preserve the existing behavior of focus ring.
* css/parser/CSSSelectorParser.cpp:
(WebCore::CSSSelectorParser::consumePseudo): Ignore -webkit-direct-focus in author and user stylesheets.
* cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::addPseudoClassType): Added the support for -webkit-direct-focus.
* dom/Element.cpp:
(WebCore::Element::setFocus): Invoke setContainsFocusedElement on each shadow ancestor root of
the newly focused element. Note that we can't use :focus-within pseudo class since that would also match
the host of a shadow root which contains a slotted focused element, causing both the shadow host and
the slotted element to match :focus pseudo class in the host's tree.
* dom/ShadowRoot.h:

LayoutTests:

Added W3C style testharness tests and ref tests.

* fast/shadow-dom/direct-focus-pseudo-does-not-match-in-author-stylesheet-expected.txt: Added.
* fast/shadow-dom/direct-focus-pseudo-does-not-match-in-author-stylesheet.html: Added.
* fast/shadow-dom/focus-pseudo-matches-on-shadow-host-expected.txt: Added.
* fast/shadow-dom/focus-pseudo-matches-on-shadow-host.html: Added.
* fast/shadow-dom/focus-pseudo-on-shadow-host-1-expected.html: Added.
* fast/shadow-dom/focus-pseudo-on-shadow-host-1.html: Added.
* fast/shadow-dom/focus-pseudo-on-shadow-host-2-expected.html: Added.
* fast/shadow-dom/focus-pseudo-on-shadow-host-2.html: Added.
* fast/shadow-dom/focus-pseudo-on-shadow-host-3-expected.html: Added.
* fast/shadow-dom/focus-pseudo-on-shadow-host-3.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@250788 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.