Summary: | Mutating styleSheet in shadow tree doesn't update the style | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, koivisto, rniwa, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | Safari Technology Preview | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 148695, 160683 | ||||||||||||||||||||||||||
Attachments: |
|
Created attachment 290812 [details]
patch
Created attachment 290814 [details]
patch
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 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 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 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 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 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 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 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
Created attachment 290846 [details]
patch
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 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 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? Created attachment 290854 [details]
patch
> 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.
> 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.
Created attachment 290857 [details]
patch
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? |
Created attachment 290231 [details] Demo Mutating StyleSheet object by calling insertRule, disabling style element itself, etc... doesn't work inside a shadow tree.