Issue 1803933002: Use correct cascading order for Shadow DOM v1

Issue 1803933002: Use correct cascading order for Shadow DOM v1 (Closed)

Created:
4 years, 9 months ago by kochi

Modified:
4 years, 9 months ago

Reviewers:
rune

CC:
chromium-reviews, blink-reviews-w3ctests_chromium.org, blink-reviews-style_chromium.org, tfarina, blink-reviews-css, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis, hayato

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Use correct cascading order for Shadow DOM v1 This implements the proposed correct cascading order for Shadow DOM. The most of the code comes from Rune's original series of CLs: https://codereview.chromium.org/1282243002/ https://codereview.chromium.org/1298173004/ https://codereview.chromium.org/1291873005/ https://codereview.chromium.org/1322753006/ Spec discussion: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/Shadow-DOM-Cascade-Order.md https://github.com/w3c/webcomponents/issues/316 Detaild doc: https://docs.google.com/document/d/1pQgj1_62dR99uNyN5Mww4Vh5zDcbCFg_JgpmysPTxic/pub BUG=487125 TEST=passes WPT shadow-dom/style Committed: https://crrev.com/2b4a245a4ea940c040af1006d9a3f49c4819d9d9 Cr-Commit-Position: refs/heads/master@{#382545}

Patch Set 1 : wip #

Patch Set 2 : tests update #

Patch Set 3 : fix inline style #

Total comments: 9

Patch Set 4 : fix for comments #

Total comments: 4

Patch Set 5 : Fix for review comment #16 #

Total comments: 2

Patch Set 6 : fix enum #

Created: 4 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+433 lines, -108 lines)			Patch
M	third_party/WebKit/LayoutTests/fast/css/content-distributed-nodes.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css/content-distributed-nodes-expected.txt	View		1 chunk	+1 line, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css/deep-cascade-order.html	View		1 chunk	+25 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css/deep-cascade-order-expected.txt	View		1 chunk	+14 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-redistribution.html	View		1 chunk	+1 line, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/dom/shadow/inner-scope-important-wins.html	View	1 2 3 4	1 chunk	+21 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/dom/shadow/inner-scope-important-wins-expected.txt	View		1 chunk	+10 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/dom/shadow/outer-scope-lower-specificity-wins.html	View	1 2 3 4	1 chunk	+21 lines, -0 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/fast/dom/shadow/outer-scope-lower-specificity-wins-expected.txt	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/dom/shadow/style-with-shadow-pseudo-element.html	View		1 chunk	+0 lines, -1 line	0 comments	Download
D	third_party/WebKit/LayoutTests/imported/web-platform-tests/shadow-dom/styles/shadow-cascade-order-expected.txt	View		1 chunk	+0 lines, -67 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-deep-in-v1.html	View	1 2 3 4	1 chunk	+30 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-deep-in-v1-expected.txt	View	1 2 3 4	1 chunk	+14 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-inner-scope-important.html	View	1 2 3 4	1 chunk	+22 lines, -0 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/shadow-dom/cascade-inner-scope-important-expected.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-multiple-shadow-with-host-rules.html	View	1 2 3 4	1 chunk	+26 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-outer-scope.html	View	1 2 3 4	1 chunk	+22 lines, -0 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/shadow-dom/cascade-outer-scope-expected.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-outer-scope2.html	View	1 2 3 4	1 chunk	+13 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-outer-scope2-expected.txt	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-slot-distributed.html	View	1	1 chunk	+15 lines, -0 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/shadow-dom/cascade-slot-distributed-expected.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/shadow-dom/cascade-upgrade-from-v0-to-v1.html	View	1 2 3 4	1 chunk	+21 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/ElementRuleCollector.h	View		3 chunks	+4 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/resolver/StyleResolver.h	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp	View	1 2 3 4	4 chunks	+124 lines, -26 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/DocumentOrderedList.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/Element.cpp	View	1 2 3 4	2 chunks	+4 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/StyleEngine.h	View	1 2 3 4 5	3 chunks	+12 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/StyleEngine.cpp	View	1 2 3 4	1 chunk	+12 lines, -0 lines	0 comments	Download

Messages

Total messages: 26 (12 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803933002/40001

4 years, 9 months ago (2016-03-16 12:22:23 UTC) #6

kochi

Hi Rune, This is the roll-up relanging of the reverted CLs, and V0/V1 switch as ...

4 years, 9 months ago (2016-03-16 12:23:18 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/189787)

4 years, 9 months ago (2016-03-16 15:52:14 UTC) #9

rune

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode420 third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:420: for (ShadowRoot* shadowRoot = shadow->oldestShadowRoot(); shadowRoot; shadowRoot = shadowRoot->youngerShadowRoot()) ...

4 years, 9 months ago (2016-03-17 10:59:03 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803933002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803933002/60001

4 years, 9 months ago (2016-03-18 08:59:54 UTC) #12

kochi

Thanks for the review! https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode420 third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:420: for (ShadowRoot* shadowRoot = shadow->oldestShadowRoot(); ...

4 years, 9 months ago (2016-03-18 09:00:52 UTC) #13

Thanks for the review!

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:420: for
(ShadowRoot* shadowRoot = shadow->oldestShadowRoot(); shadowRoot; shadowRoot =
shadowRoot->youngerShadowRoot()) {
On 2016/03/17 10:59:02, rune wrote:
> This method is used for V1. Can we have multiple shadow roots here?
> 
> Ah, we use v1 cascading for v0 trees as well... So we want to apply host rules
> from all roots, I guess?

Yeah, it is counterintuitive at a glance but once attachShadow() is used in
a document, this code has to run also on V1 shadows.

Do you have any concern?
I'll add a test for this, like multiple v0 shadow having :host rules but v1
ordering
is applied.

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:485: for (auto it
= m_treeBoundaryCrossingScopes.rbegin(); it !=
m_treeBoundaryCrossingScopes.rend(); ++it) {
On 2016/03/17 10:59:02, rune wrote:
> We should not need to walk m_treeBoundaryCrossingScopes here if we have a pure
> v1 document, but walk the chain of scopes for assigned slots. In order to get
> the performance we want for trees with many shadow trees which contains
> ::slotted rules, we should ideally not use m_treeBoundaryCrossingScopes at
all.
> 
> I see that this is an effect of the need/wish to support /deep/, ::shadow,
> ::content in documents with mixed v0 and v1 trees.

Added TODO comment.
Probably we can have an "pure v1 document" flag in StyleEngine as well, to
optimize
v1 document without waiting for v0 deprecation.

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1303: bool
m_useCascadeOrderForShadowDOMV1;
On 2016/03/17 10:59:02, rune wrote:
> I think StyleEngine would be a better place for this.

Done.

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 9 months ago (2016-03-18 11:21:43 UTC) #15

rune

I found more issues going through the second time. Since this is not currently shipping, ...

4 years, 9 months ago (2016-03-18 12:04:12 UTC) #16

I found more issues going through the second time.

Since this is not currently shipping, we can do this in steps, but I'm
commenting so we don't forget.

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:420: for
(ShadowRoot* shadowRoot = shadow->oldestShadowRoot(); shadowRoot; shadowRoot =
shadowRoot->youngerShadowRoot()) {
On 2016/03/18 09:00:52, kochi wrote:
> On 2016/03/17 10:59:02, rune wrote:
> > This method is used for V1. Can we have multiple shadow roots here?
> > 
> > Ah, we use v1 cascading for v0 trees as well... So we want to apply host
rules
> > from all roots, I guess?
> 
> Yeah, it is counterintuitive at a glance but once attachShadow() is used in
> a document, this code has to run also on V1 shadows.
> 
> Do you have any concern?

No, it was just me thinking out loud to confirm it's necessary in the presence
of v0.

https://codereview.chromium.org/1803933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:485: for (auto it
= m_treeBoundaryCrossingScopes.rbegin(); it !=
m_treeBoundaryCrossingScopes.rend(); ++it) {
On 2016/03/18 09:00:52, kochi wrote:
> On 2016/03/17 10:59:02, rune wrote:
> > We should not need to walk m_treeBoundaryCrossingScopes here if we have a
pure
> > v1 document, but walk the chain of scopes for assigned slots. In order to
get
> > the performance we want for trees with many shadow trees which contains
> > ::slotted rules, we should ideally not use m_treeBoundaryCrossingScopes at
> all.
> > 
> > I see that this is an effect of the need/wish to support /deep/, ::shadow,
> > ::content in documents with mixed v0 and v1 trees.
> 
> Added TODO comment.
> Probably we can have an "pure v1 document" flag in StyleEngine as well, to
> optimize
> v1 document without waiting for v0 deprecation.

Acknowledged.

We can iterate on this.

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:441: if
(element.isStyledElement() && element.inlineStyle() &&
!collector.isCollectingForPseudoElement()) {
Adding inline style here means we'll have inline style between /deep/ rules from
outer scope and :host rules in the case where you have mixed v0/v1. Is that the
intention?

Could you point to were the final decision/spec for the v1 cascade order was?

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.h:181: void
setUseCascadeOrderForShadowDOMV1() { m_useCascadeOrderForShadowDOMV1 = true; }
We are not recalculating style setting this flag, so currently we may end up
with different elements using different cascading order, but will gradually
change style as their style is invalidated for different mutations.

Attaching one v1 shadow will forever make this document use the v1 cascading
order, which is probably fine.

If you want to have consistent cascading across the document, you could have a
cascade order enum { SHADOW_CASCADE_NONE, SHADOW_CASCADE_V0, SHADOW_CASCADE_V1
}, start at SHADOW_CASCADE_NONE, always increase, and mark the whole document
for style recalc when changing from SHADOW_CASCADE_V0 to SHADOW_CASCADE_V1.

kochi

Thanks for the review! (We had a national holiday yesterday) https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode441 ...

4 years, 9 months ago (2016-03-22 08:16:59 UTC) #17

Thanks for the review!
(We had a national holiday yesterday)

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:441: if
(element.isStyledElement() && element.inlineStyle() &&
!collector.isCollectingForPseudoElement()) {
On 2016/03/18 12:04:12, rune wrote:
> Adding inline style here means we'll have inline style between /deep/ rules
from
> outer scope and :host rules in the case where you have mixed v0/v1. Is that
the
> intention?
> 
> Could you point to were the final decision/spec for the v1 cascade order was?

Yes, in v0/v1 mix, /deep/ wins against inline style.

The spec is not in the final state, but this issue is the resolution so far:
https://github.com/w3c/webcomponents/issues/316
I'm working on upstreaming to CSS scoping spec with Tab.

Adding a v1 shadow on v0 site will break in various degree, we don't have to
care
about cases when adding v1 breaks v0 site (hopefully:) ).

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/1803933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.h:181: void
setUseCascadeOrderForShadowDOMV1() { m_useCascadeOrderForShadowDOMV1 = true; }
On 2016/03/18 12:04:12, rune wrote:
> We are not recalculating style setting this flag, so currently we may end up
> with different elements using different cascading order, but will gradually
> change style as their style is invalidated for different mutations.
> 
> Attaching one v1 shadow will forever make this document use the v1 cascading
> order, which is probably fine.
> 
> If you want to have consistent cascading across the document, you could have a
> cascade order enum { SHADOW_CASCADE_NONE, SHADOW_CASCADE_V0, SHADOW_CASCADE_V1
> }, start at SHADOW_CASCADE_NONE, always increase, and mark the whole document
> for style recalc when changing from SHADOW_CASCADE_V0 to SHADOW_CASCADE_V1.

Very good point.
I've added the enum and set style recalc flag on V0->V1 upgrade timing.
Also added a test for the case.

rune

lgtm with nit. https://codereview.chromium.org/1803933002/diff/80001/third_party/WebKit/Source/core/dom/StyleEngine.h File third_party/WebKit/Source/core/dom/StyleEngine.h (right): https://codereview.chromium.org/1803933002/diff/80001/third_party/WebKit/Source/core/dom/StyleEngine.h#newcode59 third_party/WebKit/Source/core/dom/StyleEngine.h:59: ShadowCascadeNone = 0, Enums start at ...

4 years, 9 months ago (2016-03-22 08:57:07 UTC) #18

kochi

Thanks for the reviews! Let's see how this sticks in dev/canary. https://codereview.chromium.org/1803933002/diff/80001/third_party/WebKit/Source/core/dom/StyleEngine.h File third_party/WebKit/Source/core/dom/StyleEngine.h (right): ...

4 years, 9 months ago (2016-03-22 09:36:26 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803933002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803933002/100001

4 years, 9 months ago (2016-03-22 09:36:45 UTC) #22

commit-bot: I haz the power

4 years, 9 months ago (2016-03-22 10:48:05 UTC) #26

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/2b4a245a4ea940c040af1006d9a3f49c4819d9d9
Cr-Commit-Position: refs/heads/master@{#382545}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages