Bug 162744 - Mutating styleSheet in shadow tree doesn't update the style
Summary: Mutating styleSheet in shadow tree doesn't update the style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 148695 160683
  Show dependency treegraph
 
Reported: 2016-09-29 12:18 PDT by Ryosuke Niwa
Modified: 2016-10-06 13:56 PDT (History)
4 users (show)

See Also:


Attachments
Demo (1.56 KB, text/html)
2016-09-29 12:18 PDT, Ryosuke Niwa
no flags Details
patch (1.56 KB, patch)
2016-10-06 04:55 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (14.43 KB, patch)
2016-10-06 05:14 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1015.19 KB, application/zip)
2016-10-06 06:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (983.57 KB, application/zip)
2016-10-06 06:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2 (7.60 MB, application/zip)
2016-10-06 06:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.75 MB, application/zip)
2016-10-06 06:32 PDT, Build Bot
no flags Details
patch (24.76 KB, patch)
2016-10-06 12:09 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.30 MB, application/zip)
2016-10-06 12:51 PDT, Build Bot
no flags Details
patch (25.16 KB, patch)
2016-10-06 13:01 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (25.58 KB, patch)
2016-10-06 13:12 PDT, Antti Koivisto
rniwa: 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-09-29 12:18:21 PDT
Created attachment 290231 [details]
Demo

Mutating StyleSheet object by calling insertRule, disabling style element itself, etc... doesn't work inside a shadow tree.
Comment 1 Radar WebKit Bug Importer 2016-09-29 13:57:00 PDT
<rdar://problem/28550588>
Comment 2 Antti Koivisto 2016-10-06 04:55:51 PDT
Created attachment 290812 [details]
patch
Comment 3 Antti Koivisto 2016-10-06 05:14:59 PDT
Created attachment 290814 [details]
patch
Comment 4 Build Bot 2016-10-06 06:15:30 PDT
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
Comment 5 Build Bot 2016-10-06 06:15:33 PDT
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
Comment 6 Build Bot 2016-10-06 06:18:40 PDT
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
Comment 7 Build Bot 2016-10-06 06:18:42 PDT
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
Comment 8 Build Bot 2016-10-06 06:29:05 PDT
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
Comment 9 Build Bot 2016-10-06 06:29:08 PDT
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
Comment 10 Build Bot 2016-10-06 06:32:26 PDT
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
Comment 11 Build Bot 2016-10-06 06:32:28 PDT
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
Comment 12 Antti Koivisto 2016-10-06 12:09:03 PDT
Created attachment 290846 [details]
patch
Comment 13 Build Bot 2016-10-06 12:51:35 PDT
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
Comment 14 Build Bot 2016-10-06 12:51:38 PDT
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
Comment 15 Ryosuke Niwa 2016-10-06 12:56:37 PDT
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?
Comment 16 Antti Koivisto 2016-10-06 13:01:57 PDT
Created attachment 290854 [details]
patch
Comment 17 Antti Koivisto 2016-10-06 13:03:15 PDT
> 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.
Comment 18 Antti Koivisto 2016-10-06 13:09:47 PDT
> 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.
Comment 19 Antti Koivisto 2016-10-06 13:12:29 PDT
Created attachment 290857 [details]
patch
Comment 20 Ryosuke Niwa 2016-10-06 13:18:34 PDT
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?
Comment 21 Antti Koivisto 2016-10-06 13:56:07 PDT
https://trac.webkit.org/r206880