| Summary: | [Shadow DOM]: scoped styles are not applied in the cascade order. | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Takashi Sakamoto <tasak> | ||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | 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
Takashi Sakamoto
2012-11-26 03:56:23 PST
Created attachment 176175 [details]
Patch
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 Created attachment 176204 [details]
Patch
Created attachment 176214 [details]
Patch
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
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 Created attachment 178152 [details]
WIP
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. Created attachment 178517 [details]
WIP
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 Created attachment 178716 [details]
WIP
Created attachment 178718 [details]
WIP
Created attachment 178721 [details]
WIP
Created attachment 178733 [details]
WIP
Created attachment 178740 [details]
Patch
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 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 I cannot reproduce the following failure locally: Regressions: Unexpected crashes (1) inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html [ Crash ] (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. 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? Created attachment 178968 [details]
Patch
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…).... Created attachment 179184 [details]
Patch
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 Created attachment 179205 [details]
Patch
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 Created attachment 179208 [details]
Patch
Comment on attachment 179208 [details] Patch Clearing flags on attachment: 179208 Committed r137708: <http://trac.webkit.org/changeset/137708> All reviewed patches have been landed. Closing bug. |