Bug 202432 - focus pseudo class should match a shadow host whose shadow tree contains the focused element
Summary: focus pseudo class should match a shadow host whose shadow tree contains the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695 166484
  Show dependency treegraph
 
Reported: 2019-10-01 14:01 PDT by Ryosuke Niwa
Modified: 2019-10-15 15:13 PDT (History)
18 users (show)

See Also:


Attachments
Updates the behavior (15.34 KB, patch)
2019-10-03 22:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updates the behavior (15.43 KB, patch)
2019-10-03 22:26 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the tests (29.22 KB, patch)
2019-10-04 21:35 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Reverted unintended test change (28.26 KB, patch)
2019-10-04 23:26 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-10-01 14:01:12 PDT
Per https://github.com/whatwg/html/pull/4731, `:focus` pseudo class should match a shadow host element
whose shadow root (shadow inclusively) contains the currently focused element.
Comment 1 Ryosuke Niwa 2019-10-03 22:25:00 PDT
Created attachment 380187 [details]
Updates the behavior
Comment 2 Ryosuke Niwa 2019-10-03 22:26:49 PDT
Created attachment 380188 [details]
Updates the behavior
Comment 3 Antti Koivisto 2019-10-03 23:56:04 PDT
Comment on attachment 380188 [details]
Updates the behavior

View in context: https://bugs.webkit.org/attachment.cgi?id=380188&action=review

> Source/WebCore/dom/Element.cpp:682
> +        root->setContainsFocusedElement(flag);

I suppose setFocus is already guaranteed to be called consistently in all DOM mutations? This sort of stuff easily leaves stuck flags behind.
Comment 4 Ryosuke Niwa 2019-10-04 00:21:56 PDT
Comment on attachment 380188 [details]
Updates the behavior

View in context: https://bugs.webkit.org/attachment.cgi?id=380188&action=review

>> Source/WebCore/dom/Element.cpp:682
>> +        root->setContainsFocusedElement(flag);
> 
> I suppose setFocus is already guaranteed to be called consistently in all DOM mutations? This sort of stuff easily leaves stuck flags behind.

Yeah, it's called whenever the focus state is changed.

And if the tree relationship between any of ancestor nodes were to change,
then this element would have lost the focus so setFocus would have to be called first anyway.

It's done as the last step before removing nodes in ContainerNode.
Comment 5 Ryosuke Niwa 2019-10-04 21:35:27 PDT
Created attachment 380277 [details]
Fixed the tests
Comment 6 Ryosuke Niwa 2019-10-04 21:36:26 PDT
Antti: asking for another review because I had to introduce a new pseudo class.

Here's one question I have. Should we add -webkit-direct-focus to SelectorChecker::isCommonPseudoClassSelector?
Comment 7 Ryosuke Niwa 2019-10-04 23:26:54 PDT
Created attachment 380278 [details]
Reverted unintended test change
Comment 8 Antti Koivisto 2019-10-05 01:40:05 PDT
Comment on attachment 380278 [details]
Reverted unintended test change

View in context: https://bugs.webkit.org/attachment.cgi?id=380278&action=review

> Source/WebCore/css/CSSSelector.cpp:443
> +            case CSSSelector::PseudoClassDirectFocus:
> +                str.appendLiteral(":-webkit-direct-focus");
> +                break;

I suppose some code generator strips 'webkit' out from the enum values? I wonder if we could avoid that for internal properties (have PseudoClassWebKitDirectFocus)

> Source/WebCore/css/html.css:1168
> -:focus {
> +:-webkit-direct-focus {

Do other instances of :focus on UA sheets also need to be changed to :-webkit-direct-focus? html4.css has more, and svg.css and mathml.css also have instances.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:501
> +            if (m_context.mode != UASheetMode && selector->pseudoClassType() == CSSSelector::PseudoClassDirectFocus)
> +                return nullptr;

We should have some sort of more general mechanism for hiding internal pseudo classes. (like hide based on the name)
Comment 9 Antti Koivisto 2019-10-05 01:47:21 PDT
> Do other instances of :focus on UA sheets also need to be changed to
> :-webkit-direct-focus? html4.css has more, and svg.css and mathml.css also
> have instances.

If all UA sheet instances should be direct focus then we could just do the mapping to PseudoClassDirectFocus in the parser and avoid adding a new pseudo class string. But maybe that is overly opaque.
Comment 10 Ryosuke Niwa 2019-10-07 13:55:34 PDT
(In reply to Antti Koivisto from comment #8)
> Comment on attachment 380278 [details]
> Reverted unintended test change
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380278&action=review
> 
> > Source/WebCore/css/CSSSelector.cpp:443
> > +            case CSSSelector::PseudoClassDirectFocus:
> > +                str.appendLiteral(":-webkit-direct-focus");
> > +                break;
> 
> I suppose some code generator strips 'webkit' out from the enum values? I
> wonder if we could avoid that for internal properties (have
> PseudoClassWebKitDirectFocus)

Yeah, that would be a nice refactoring. I stayed away from it because it seemed like it's going to be a pretty big change. I think if we end up having to a few more of these, it probably makes sense to have the generator support it directly so that we can specify it as an option; e.g. UAOnly
> > Source/WebCore/css/html.css:1168
> > -:focus {
> > +:-webkit-direct-focus {
> 
> Do other instances of :focus on UA sheets also need to be changed to
> :-webkit-direct-focus? html4.css has more, and svg.css and mathml.css also
> have instances.

As far as I can tell not. Because elements like select and input don't support a shadow root and neither does SVG and MathML.
> > Source/WebCore/css/parser/CSSSelectorParser.cpp:501
> > +            if (m_context.mode != UASheetMode && selector->pseudoClassType() == CSSSelector::PseudoClassDirectFocus)
> > +                return nullptr;
> 
> We should have some sort of more general mechanism for hiding internal
> pseudo classes. (like hide based on the name)

Indeed. I think it makes sense to add it as a generator option in SelectorPseudoClassAndCompatibilityElementMap.in but probably needs to be done in a separate patch because it seems pretty involved.
Comment 11 Ryosuke Niwa 2019-10-07 13:55:49 PDT
Thanks for the review!
Comment 12 Ryosuke Niwa 2019-10-07 13:58:05 PDT
Comment on attachment 380278 [details]
Reverted unintended test change

Clearing flags on attachment: 380278

Committed r250788: <https://trac.webkit.org/changeset/250788>
Comment 13 Ryosuke Niwa 2019-10-07 13:58:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-10-07 13:59:18 PDT
<rdar://problem/56049598>
Comment 15 Kent Tamura 2019-10-14 22:03:56 PDT
I think :-webkit-direct-focus should be renamed to :focus-visible.

https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo
Comment 16 Antti Koivisto 2019-10-15 00:17:33 PDT
> As far as I can tell not. Because elements like select and input don't
> support a shadow root and neither does SVG and MathML.

We have

html:focus, body:focus, input[readonly]:focus, applet:focus, embed:focus, iframe:focus, object:focus {
    outline: none;
}

and canAttachAuthorShadowRoot returns true for body.
Comment 17 Ryosuke Niwa 2019-10-15 15:13:52 PDT
(In reply to Antti Koivisto from comment #16)
> > As far as I can tell not. Because elements like select and input don't
> > support a shadow root and neither does SVG and MathML.
> 
> We have
> 
> html:focus, body:focus, input[readonly]:focus, applet:focus, embed:focus,
> iframe:focus, object:focus {
>     outline: none;
> }
> 
> and canAttachAuthorShadowRoot returns true for body.

But that's okay because they're simply removing the outline by default. Even if body which is a shadow host which contains the focused element matched this rule, it would simply disable the outline on the body, which is the correct behavior anyway.