Updating class name of a shadow host does not update the style applied by :host()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

When a class name is updated, a functional shadow host rule such as :host(.foo) would not be applied dynamically.

[Ryosuke Niwa]

Created attachment 281606 [details]
Demo

[Radar WebKit Bug Importer]

<rdar://problem/26883707>

[Antti Koivisto]

Created attachment 281623 [details]
Patch

[Darin Adler]

Comment on attachment 281623 [details]
Patch

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

> Source/WebCore/style/AttributeChangeInvalidation.cpp:42
> +    if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
> +        return false;

Is there a solid guarantee that authorStyle is non-null? I noticed this pattern repeating in the code so the question applies not just here but at many other sites too.

> Source/WebCore/style/AttributeChangeInvalidation.cpp:61
> +    auto* shadowRoot = m_element.shadowRoot();
> +    if (shadowRoot && mayBeAffectedByHostStyle(*shadowRoot, isHTML, attributeName))
> +        mayAffectStyle = true;

Can we skip this work if mayAffectStyle is already true?

> Source/WebCore/style/ClassChangeInvalidation.cpp:108
> +        if (shadowRoot && mayBeAffectedByHostStyle(*shadowRoot, changedClass))
> +            mayAffectStyle = true;

Can we skip this work if mayAffectStyle is already true?

> Source/WebCore/style/IdChangeInvalidation.cpp:57
> +    auto* shadowRoot = m_element.shadowRoot();
> +    if (shadowRoot && mayBeAffectedByHostStyle(*shadowRoot, changedId))
> +        mayAffectStyle = true;

Can we skip this work if mayAffectStyle is already true?

[Antti Koivisto]

> Is there a solid guarantee that authorStyle is non-null? I noticed this
> pattern repeating in the code so the question applies not just here but at
> many other sites too.

Yes, StyleResolver constructor calls resetAuthorStyle() which creates authorStyle. There is no path to delete it. Definitely something that could be cleaned up.

> Can we skip this work if mayAffectStyle is already true?

Yes, though not super important.

[Simon Fraser (smfr)]

Comment on attachment 281623 [details]
Patch

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

> Source/WebCore/style/AttributeChangeInvalidation.cpp:38
> +static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, bool isHTML, const QualifiedName& attributeName)

Can shadowRoot be const?

> Source/WebCore/style/ClassChangeInvalidation.cpp:88
> +static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, AtomicStringImpl* changedClass)

const shadowRoot?

> Source/WebCore/style/IdChangeInvalidation.cpp:44
> +static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, const AtomicString& changedId)
> +{
> +    auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
> +    if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
> +        return false;
> +
> +    return shadowRuleSets.features().idsInRules.contains(changedId.impl());
> +}

Can this function that's almost repeated 3 times be shared?

[Antti Koivisto]

> Can shadowRoot be const?

Not without wider constness refactoring.

> Can this function that's almost repeated 3 times be shared?

It is only line that is repeated really. Still, would be nice to factor these to share the common pattern.

[Antti Koivisto]

https://trac.webkit.org/r202227

[Keanu Lee]

This issue still exists in webkit (r214532). If you try the linked demo below and click on the button, you'll see the the paragraph color does not toggle to red (the button does because its styles are invalidated when you click on it.

http://jsbin.com/masaxeq/edit?html,output

[Ryosuke Niwa]

(In reply to Keanu Lee from comment #9)
> This issue still exists in webkit (r214532). If you try the linked demo
> below and click on the button, you'll see the the paragraph color does not
> toggle to red (the button does because its styles are invalidated when you
> click on it.
> 
> http://jsbin.com/masaxeq/edit?html,output

Oh, this is a slightly different bug where we don't seem to be invalidating elements with rules that apply to a descendant or a child of :host.

Filed webkit.org/b/170762 to track this.