focus pseudo class should match a shadow host whose shadow tree contains the foc...

Bug 202432 - focus pseudo class should match a shadow host whose shadow tree contains the focused element

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.

Created attachment 380187 [details]
Updates the behavior

Created attachment 380188 [details]
Updates the behavior

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 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.

Created attachment 380277 [details]
Fixed the tests

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?

Created attachment 380278 [details]
Reverted unintended test change

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)

> 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.

(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.

Thanks for the review!

Comment on attachment 380278 [details]
Reverted unintended test change

Clearing flags on attachment: 380278

Committed r250788: <https://trac.webkit.org/changeset/250788>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/56049598>

I think :-webkit-direct-focus should be renamed to :focus-visible.

https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo

> 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.

(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.