Bug 103239 - [Shadow DOM]: scoped styles are not applied in the cascade order.
Summary: [Shadow DOM]: scoped styles are not applied in the cascade order.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on: 103600
Blocks: 97282
  Show dependency treegraph
 
Reported: 2012-11-26 03:56 PST by Takashi Sakamoto
Modified: 2012-12-13 20:41 PST (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 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.
Comment 1 Takashi Sakamoto 2012-11-26 22:16:14 PST
Created attachment 176175 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Takashi Sakamoto 2012-11-27 02:03:58 PST
Created attachment 176204 [details]
Patch
Comment 4 Takashi Sakamoto 2012-11-27 02:42:36 PST
Created attachment 176214 [details]
Patch
Comment 5 Takashi Sakamoto 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
Comment 6 WebKit Review Bot 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
Comment 7 Takashi Sakamoto 2012-12-06 21:05:26 PST
Created attachment 178152 [details]
WIP
Comment 8 Dominic Cooney 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.
Comment 9 Takashi Sakamoto 2012-12-10 04:37:39 PST
Created attachment 178517 [details]
WIP
Comment 10 WebKit Review Bot 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
Comment 11 Takashi Sakamoto 2012-12-10 22:05:16 PST
Created attachment 178716 [details]
WIP
Comment 12 Takashi Sakamoto 2012-12-10 22:21:37 PST
Created attachment 178718 [details]
WIP
Comment 13 Takashi Sakamoto 2012-12-10 22:32:54 PST
Created attachment 178721 [details]
WIP
Comment 14 Takashi Sakamoto 2012-12-10 23:56:10 PST
Created attachment 178733 [details]
WIP
Comment 15 Takashi Sakamoto 2012-12-11 00:30:28 PST
Created attachment 178740 [details]
Patch
Comment 16 Takashi Sakamoto 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
Comment 17 WebKit Review Bot 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
Comment 18 Takashi Sakamoto 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 ]
Comment 19 Dimitri Glazkov (Google) 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.
Comment 20 Dimitri Glazkov (Google) 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?
Comment 21 Takashi Sakamoto 2012-12-11 22:23:59 PST
Created attachment 178968 [details]
Patch
Comment 22 Takashi Sakamoto 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…)....
Comment 23 Takashi Sakamoto 2012-12-12 19:57:53 PST
Created attachment 179184 [details]
Patch
Comment 24 Takashi Sakamoto 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
Comment 25 Takashi Sakamoto 2012-12-12 23:00:32 PST
Created attachment 179205 [details]
Patch
Comment 26 Takashi Sakamoto 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
Comment 27 Takashi Sakamoto 2012-12-12 23:16:58 PST
Created attachment 179208 [details]
Patch
Comment 28 WebKit Review Bot 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>
Comment 29 WebKit Review Bot 2012-12-13 20:41:55 PST
All reviewed patches have been landed.  Closing bug.