Bug 103239

Summary: [Shadow DOM]: scoped styles are not applied in the cascade order.
Product: WebKit Reporter: Takashi Sakamoto <tasak>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, dglazkov, dominicc, ericbidelman, macpherson, menard, mibalan, morrita, ojan.autocc, ojan, stearns, tasak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103600    
Bug Blocks: 97282    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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
DetailsFormatted DiffDiff
Patch (24.66 KB, patch)
2012-11-27 02:03 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (24.73 KB, patch)
2012-11-27 02:42 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (27.12 KB, patch)
2012-12-06 21:05 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (16.91 KB, patch)
2012-12-10 04:37 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (22.81 KB, patch)
2012-12-10 22:05 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (22.91 KB, patch)
2012-12-10 22:21 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (22.53 KB, patch)
2012-12-10 22:32 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (28.19 KB, patch)
2012-12-10 23:56 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (22.48 KB, patch)
2012-12-11 00:30 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (22.43 KB, patch)
2012-12-11 22:23 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (18.10 KB, patch)
2012-12-12 19:57 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (25.47 KB, patch)
2012-12-12 23:00 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (26.77 KB, patch)
2012-12-12 23:16 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
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.