Bug 162744 – Mutating styleSheet in shadow tree doesn't update the style

• Component: CSS
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 290231 [details]
Demo

Mutating StyleSheet object by calling insertRule, disabling style element itself, etc... doesn't work inside a shadow tree.

[Radar WebKit Bug Importer]

<rdar://problem/28550588>

[Antti Koivisto]

Created attachment 290812 [details]
patch

[Antti Koivisto]

Created attachment 290814 [details]
patch

[Build Bot]

Comment on attachment 290814 [details]
patch

Attachment 290814 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2231217

New failing tests:
fast/dom/StyleSheet/detached-parent-rule-without-wrapper.html

[Build Bot]

Created attachment 290817 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290814 [details]
patch

Attachment 290814 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2231220

New failing tests:
fast/dom/StyleSheet/detached-parent-rule-without-wrapper.html

[Build Bot]

Created attachment 290818 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290814 [details]
patch

Attachment 290814 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2231225

New failing tests:
fast/dom/StyleSheet/detached-parent-rule-without-wrapper.html

[Build Bot]

Created attachment 290819 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 290814 [details]
patch

Attachment 290814 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2231222

New failing tests:
fast/dom/StyleSheet/detached-parent-rule-without-wrapper.html

[Build Bot]

Created attachment 290820 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antti Koivisto]

Created attachment 290846 [details]
patch

[Build Bot]

Comment on attachment 290846 [details]
patch

Attachment 290846 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2232943

New failing tests:
fast/shadow-dom/mutating-stylesheet-in-shadow-tree.html

[Build Bot]

Created attachment 290853 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Comment on attachment 290846 [details]
patch

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

It looks like two test cases are failing on WK1?

> Source/WebCore/css/CSSStyleSheet.cpp:173
> +    auto* scope = styleSheetScope();

I'm not sure scope is the most descriptive name of AuthorStyleSheets objects but perhaps you're planning to rename it?

> Source/WebCore/css/CSSStyleSheet.cpp:-205
> -    owner->authorStyleSheets().didChangeCandidatesForActiveSet();

Could you clarify in the change log why it's okay to remove this call?

> Source/WebCore/css/CSSStyleSheet.cpp:410
> +{
> +    return const_cast<CSSStyleSheet&>(*this).rootStyleSheet();
> +}

Maybe we can just inline this in the header?

> Source/WebCore/dom/AuthorStyleSheets.cpp:88
> +AuthorStyleSheets& AuthorStyleSheets::forNode(Node& node)

It's pretty neat that we just needed three functions to abstract Document/ShadowRoot difference!

> Source/WebCore/dom/AuthorStyleSheets.cpp:337
> +    if (m_shadowRoot)
> +        updateType = UpdateType::ContentsOrInterpretation;
> +

Why do we want to override this? It seems that we do want to analyzeStyleSheetChange even inside a shadow tree?
Please clarify that in the change log.

> Source/WebCore/html/HTMLLinkElement.cpp:-333
> -
> -    if (document().hasLivingRenderTree())
> -        document().authorStyleSheets().didChangeCandidatesForActiveSet();

Could you clarify why it's okay to remove this in the change log?

[Antti Koivisto]

Created attachment 290854 [details]
patch

[Antti Koivisto]

> I'm not sure scope is the most descriptive name of AuthorStyleSheets objects
> but perhaps you're planning to rename it?

Yes, in a separate patch.

[Antti Koivisto]

> Why do we want to override this? It seems that we do want to
> analyzeStyleSheetChange even inside a shadow tree?

We don't currently do optimized updates in shadow trees which is not changed by this patch. I'll enable it later.

[Antti Koivisto]

Created attachment 290857 [details]
patch

[Ryosuke Niwa]

Comment on attachment 290857 [details]
patch

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

r=me assuming tests pass on WK1

> Source/WebCore/dom/AuthorStyleSheets.cpp:337
> +    if (m_shadowRoot)
> +        updateType = UpdateType::ContentsOrInterpretation;

Should we add a FIXME saying that we should eventually be able to remove this special case?

[Antti Koivisto]

https://trac.webkit.org/r206880