103239 – [Shadow DOM]: scoped styles are not applied in the cascade order.

RESOLVED FIXED 103239

[Shadow DOM]: scoped styles are not applied in the cascade order.

https://bugs.webkit.org/show_bug.cgi?id=103239

Summary [Shadow DOM]: scoped styles are not applied in the cascade order.

Takashi Sakamoto

Reported 2012-11-26 03:56:23 PST

From http://code.google.com/p/chromium/issues/detail?id=162517. Current WebKit implementation uses different document position for <style scoped> whose scoped element is different (i.e. RuleSet::m_ruleCount and doesn't share information about m_ruleCount with StyleResolver::m_authorStyle). So if some html document has <style> and <style scoped>, wrong rules might be applied.

Attachments
Patch (21.08 KB, patch) 2012-11-26 22:16 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (24.66 KB, patch) 2012-11-27 02:03 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (24.73 KB, patch) 2012-11-27 02:42 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
WIP (27.12 KB, patch) 2012-12-06 21:05 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
WIP (16.91 KB, patch) 2012-12-10 04:37 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
WIP (22.81 KB, patch) 2012-12-10 22:05 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
WIP (22.91 KB, patch) 2012-12-10 22:21 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
WIP (22.53 KB, patch) 2012-12-10 22:32 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
WIP (28.19 KB, patch) 2012-12-10 23:56 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (22.48 KB, patch) 2012-12-11 00:30 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (22.43 KB, patch) 2012-12-11 22:23 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (18.10 KB, patch) 2012-12-12 19:57 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (25.47 KB, patch) 2012-12-12 23:00 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (26.77 KB, patch) 2012-12-12 23:16 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.

Takashi Sakamoto

Comment 1 2012-11-26 22:16:14 PST

Created attachment 176175 [details] Patch

WebKit Review Bot

Comment 2 2012-11-26 23:53:11 PST

Comment on attachment 176175 [details] Patch Attachment 176175 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15013014 New failing tests: fast/regions/style-scoped-in-flow-override-region-styling.html animations/suspend-resume-animation-events.html fast/dom/shadow/athost-atrules.html http/tests/security/local-user-CSS-from-remote.html

Takashi Sakamoto

Comment 3 2012-11-27 02:03:58 PST

Created attachment 176204 [details] Patch

Takashi Sakamoto

Comment 4 2012-11-27 02:42:36 PST

Created attachment 176214 [details] Patch

Takashi Sakamoto

Comment 5 2012-11-27 02:51:17 PST

I would like to ask about fast/regions/style-scoped-in-flow-override-region-styling.html. The test has a style element under <head> and the style element has @region rule, i.e. #f3 { -webkit-flow-into: flow3; } #r3 { -webkit-flow-from: flow3; } @-webkit-region #r3 { p { background-color: lightgreen; } .c3 { background-color: lime; } #p3 { background-color: green; } } and the test also has a scoped style element which is a direct child of #f3: <div id='f3'> <style scoped='true'> p { background-color: yellow; } .c3 { background-color: orange; } #p3 { background-color: red; } </style> Since both rules have the same specificities, we have to compare the document positions of both rules. I think, the style scoped should be applied. Is this correct? The test's expected result says that @region rule should be applied... Best regards, Takashi Sakamoto

WebKit Review Bot

Comment 6 2012-11-27 11:00:38 PST

Comment on attachment 176214 [details] Patch Attachment 176214 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15003566 New failing tests: fast/regions/style-scoped-in-flow-override-region-styling.html fast/css/style-scoped/style-scoped-apply-author-styles.html

Takashi Sakamoto

Comment 7 2012-12-06 21:05:26 PST

Created attachment 178152 [details] WIP

Dominic Cooney

Comment 8 2012-12-09 21:29:47 PST

This part of the Shadow DOM spec might also be relevant, in that it deals with the application (via cascade) of scoped styles in Shadow DOM: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles For the purposes of the cascade order, the CSS rules for shadow trees that have the apply-author-styles flag set must be treated as scoped selectors with shadow root as their scope. In this context, for shadow trees A and B, the A's shadow root is treated as descendant of B's shadow root if A is enclosed by B.

Takashi Sakamoto

Comment 9 2012-12-10 04:37:39 PST

Created attachment 178517 [details] WIP

WebKit Review Bot

Comment 10 2012-12-10 07:52:06 PST

Comment on attachment 178517 [details] WIP Attachment 178517 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15241356 New failing tests: fast/regions/style-scoped-in-flow-override-region-styling.html

Takashi Sakamoto

Comment 11 2012-12-10 22:05:16 PST

Created attachment 178716 [details] WIP

Takashi Sakamoto

Comment 12 2012-12-10 22:21:37 PST

Created attachment 178718 [details] WIP

Takashi Sakamoto

Comment 13 2012-12-10 22:32:54 PST

Created attachment 178721 [details] WIP

Takashi Sakamoto

Comment 14 2012-12-10 23:56:10 PST

Created attachment 178733 [details] WIP

Takashi Sakamoto

Comment 15 2012-12-11 00:30:28 PST

Created attachment 178740 [details] Patch

Takashi Sakamoto

Comment 16 2012-12-11 00:51:59 PST

I don't know the reason why win-ews reported the following failure: --- Last 500 characters of output: tTests/fast/regions/style-scoped-in-flow-override-region-styling-expected.html Hunk #1 FAILED at 32. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/regions/style-scoped-in-flow-override-region-styling-expected.html.rej patching file LayoutTests/fast/regions/style-scoped-in-flow-override-region-styling.html Hunk #1 FAILED at 50. Hunk #2 FAILED at 87. 2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/fast/regions/style-scoped-in-flow-override-region-styling.html.rej Failed to run "['/home/buildbot/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /home/buildbot/WebKit ---- However style queue said: ---- Pass 16 minutes ago Style checked 16 minutes ago Watchlist applied 16 minutes ago Applied patch ^^^^^^^^^^^ ----- Best regards, Takashi Sakamoto

WebKit Review Bot

Comment 17 2012-12-11 01:20:47 PST

Comment on attachment 178740 [details] Patch Attachment 178740 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15277039 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

Takashi Sakamoto

Comment 18 2012-12-11 01:50:12 PST

I cannot reproduce the following failure locally: Regressions: Unexpected crashes (1) inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html [ Crash ]

Dimitri Glazkov (Google)

Comment 19 2012-12-11 09:48:25 PST

(In reply to comment #16) > I don't know the reason why win-ews reported the following failure: > --- > Last 500 characters of output: > tTests/fast/regions/style-scoped-in-flow-override-region-styling-expected.html > Hunk #1 FAILED at 32. > 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/regions/style-scoped-in-flow-override-region-styling-expected.html.rej > patching file LayoutTests/fast/regions/style-scoped-in-flow-override-region-styling.html > Hunk #1 FAILED at 50. > Hunk #2 FAILED at 87. > 2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/fast/regions/style-scoped-in-flow-override-region-styling.html.rej > > Failed to run "['/home/buildbot/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /home/buildbot/WebKit > ---- > > However style queue said: > ---- > Pass 16 minutes ago > Style checked 16 minutes ago > Watchlist applied 16 minutes ago > Applied patch > ^^^^^^^^^^^ There were some gremlins among the style elves last night.

Dimitri Glazkov (Google)

Comment 20 2012-12-11 11:21:27 PST

Comment on attachment 178740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178740&action=review > Source/WebCore/css/StyleResolver.cpp:891 > + if (r1.first == r2.first) { Early return might be better here? > Source/WebCore/css/StyleResolver.h:480 > + Vector<ScopeRuleData, 32> m_matchedRules; This doesn't quite seem like the right solution. The scope level idea is right, but it seems wrong to not store it in RuleData. It's a property of a rule, right?

Takashi Sakamoto

Comment 21 2012-12-11 22:23:59 PST

Created attachment 178968 [details] Patch

Takashi Sakamoto

Comment 22 2012-12-11 22:31:51 PST

Comment on attachment 178740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178740&action=review Thank you for reviewing. >> Source/WebCore/css/StyleResolver.cpp:891 >> + if (r1.first == r2.first) { > > Early return might be better here? I see. Done. >> Source/WebCore/css/StyleResolver.h:480 >> + Vector<ScopeRuleData, 32> m_matchedRules; > > This doesn't quite seem like the right solution. The scope level idea is right, but it seems wrong to not store it in RuleData. It's a property of a rule, right? Yeah, I agree that my ways is not good. I uploaded this patch to discuss what is a right solution. Talking about modifying class RuleData, I have following concerns: (1) memory usage. Class RuleData has many member variables with bit field parameters, e.g. unsigned position : 20 and so on. Is it ok to use more 5 or 6 bits per RuleData when there are neither shadow roots nor scoped styles? (Since I heard that shadow dom trees in ui-toolkit examples are nested to about 10 levels, I guess, we need 5 bits or 6 bits) (2) granularity. Each scoping level is prepared for each RuleSet. RuleData in the same RuleSet have the same scoping level. I'm afraid that we uses much more memory than we expect (related to (1)). (3) when and where we add scoping level information to RuleData. While creating RuleData, stylesheets are not sorted in the cascade order (Node::compareDocumentPosition doesn't provide any information when nodes in different tree scopes are given). Is it ok to see all RuleSets and to update all RuleData in the RuleSets after StyleResolver::appendAuthorStyleSheets? I'm also thinking of another way: (a) run sortAndTransfer for each scoping element (I tried in my @host @-rules patch and I reverted…)....

Takashi Sakamoto

Comment 23 2012-12-12 19:57:53 PST

Created attachment 179184 [details] Patch

Takashi Sakamoto

Comment 24 2012-12-12 20:04:59 PST

I removed scopingLevel. Instead, I modified to invoke sortAndTransferMatchedRules after collecting scoped rules and collecting @host @-rules. Now the cascading order is kept by the order of matchXXXRules. Best regards, Takashi Sakamoto

Takashi Sakamoto

Comment 25 2012-12-12 23:00:32 PST

Created attachment 179205 [details] Patch

Takashi Sakamoto

Comment 26 2012-12-12 23:01:36 PST

We need one more layout test for scoped stylesheets with important rules. So I added fast/css/style-scoped/style-scoped-with-important-rule.html. Best regards, Takashi Sakamoto

Takashi Sakamoto

Comment 27 2012-12-12 23:16:58 PST

Created attachment 179208 [details] Patch

WebKit Review Bot

Comment 28 2012-12-13 20:41:49 PST

Comment on attachment 179208 [details] Patch Clearing flags on attachment: 179208 Committed r137708: <http://trac.webkit.org/changeset/137708>

WebKit Review Bot

Comment 29 2012-12-13 20:41:55 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Takashi Sakamoto

Reported

2012-11-26 03:56 PST

Modified

2012-12-13 20:41 PST History

CC List

14 users Show

URL

Keywords

Depends on

103600

Blocks

97282

Dependencies

tree graph