Bug 158900 - Updating class name of a shadow host does not update the style applied by :host()
Summary: Updating class name of a shadow host does not update the style applied by :ho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 148695 170762
  Show dependency treegraph
 
Reported: 2016-06-17 22:03 PDT by Ryosuke Niwa
Modified: 2017-04-11 21:29 PDT (History)
4 users (show)

See Also:


Attachments
Demo (518 bytes, text/html)
2016-06-17 22:03 PDT, Ryosuke Niwa
no flags Details
Patch (9.38 KB, patch)
2016-06-19 14:25 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-06-17 22:03:41 PDT
When a class name is updated, a functional shadow host rule such as :host(.foo) would not be applied dynamically.
Comment 1 Ryosuke Niwa 2016-06-17 22:03:57 PDT
Created attachment 281606 [details]
Demo
Comment 2 Radar WebKit Bug Importer 2016-06-19 14:19:38 PDT
<rdar://problem/26883707>
Comment 3 Antti Koivisto 2016-06-19 14:25:28 PDT
Created attachment 281623 [details]
Patch
Comment 4 Darin Adler 2016-06-19 14:57:58 PDT
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?
Comment 5 Antti Koivisto 2016-06-19 16:27:40 PDT
> 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.
Comment 6 Simon Fraser (smfr) 2016-06-19 22:10:56 PDT
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?
Comment 7 Antti Koivisto 2016-06-20 01:13:00 PDT
> 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.
Comment 8 Antti Koivisto 2016-06-20 01:54:57 PDT
https://trac.webkit.org/r202227
Comment 9 Keanu Lee 2017-04-11 12:26:02 PDT
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
Comment 10 Ryosuke Niwa 2017-04-11 21:29:16 PDT
(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.