Implement CSS parser part for ::slotted pseudo element

Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=fast/dom/shadow/slotted-pseudo-element-css-text.html

Committed: https://crrev.com/5dc8704a6c571f65f3d00731da773c2c5ebd8f2e
Cr-Commit-Position: refs/heads/master@{#370358}

[kochi, 2016-01-13 07:46:32]

Description was changed from

==========
Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=
==========

to

==========
Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=fast/dom/shadow/slotted-pseudo-element-css-text.html
==========

[kochi, 2016-01-13 07:46:32]

kochi@chromium.org changed reviewers:
+ rune@opera.com, timloh@chromium.org

[kochi, 2016-01-13 07:48:31]

Tim,

Could you take a look at this?

Rune,
For the "cssText" layout test I reflected your comments
in the previous review.

[rune, 2016-01-13 09:59:09]

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

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelector.cpp:486: case PseudoSlotted:
The selectors are web-facing in the sense that ::slotted(*) {} rules are not
dropped from the CSSOM unless you do a runtime flag check for shadow dom V1
somewhere.

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

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.cpp:174: return
selector.relationIsAffectedByPseudoSlotted();
If you remove the relationIsAffectedByPseudoSlotted(), I think you can check for
PseudoSlotted instead.

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelectorList.h (left):

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.h:84: bool
selectorHasShadowDistributed(size_t index) const;
I would have renamed this to selectorHasContentPseudo.

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelectorList.h (right):

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.h:83: bool
selectorHasShadowSlotted(size_t index) const;
... and selectorHasSlottedPseudo perhaps?

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:391:
selector->setRelationIsAffectedByPseudoSlotted();
I don't think you need this flag at all, only the ShadowSlot combinator. The
reason we need it for ::content, is that we have explicit combinators in the
selector source and instead of inventing DescendantInsertionPointCrossing,
ChildInsertionPointCrossing, we use Descendant and Child with the
relation-is-affected bit set. In the css text, ::slotted will part of the same
compound as the simple selectors matching the slot, but we introduce a
ShadowSlot combinator and split the compound into two like we do for custom
pseudo elements like in input::-webkit-*.

I've been trying to simplify this splitting process in
https://codereview.chromium.org/1574323003/

[kochi, 2016-01-14 09:02:20]

Rune, thanks for the review!

You mentioned we can go without ShadowSlot combinator
in the other CL. I'm trying to do the change (combined
with the matching part) but still taking time.

I think I can work on it while waiting for your CL
https://codereview.chromium.org/1574323003/
to land.

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

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelector.cpp:486: case PseudoSlotted:
On 2016/01/13 09:59:09, rune wrote:
> The selectors are web-facing in the sense that ::slotted(*) {} rules are not
> dropped from the CSSOM unless you do a runtime flag check for shadow dom V1
> somewhere.

Done in CSSSelectorParser.cpp.

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

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.cpp:174: return
selector.relationIsAffectedByPseudoSlotted();
On 2016/01/13 09:59:09, rune wrote:
> If you remove the relationIsAffectedByPseudoSlotted(), I think you can check
for
> PseudoSlotted instead.

Done.

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelectorList.h (left):

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.h:84: bool
selectorHasShadowDistributed(size_t index) const;
On 2016/01/13 09:59:09, rune wrote:
> I would have renamed this to selectorHasContentPseudo.

Done.

I'd like to rename other instances of "shadow distributed" (e.g. RuleSet has
m_shadowDistributedRules) in another CL.

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelectorList.h (right):

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.h:83: bool
selectorHasShadowSlotted(size_t index) const;
On 2016/01/13 09:59:09, rune wrote:
> ... and selectorHasSlottedPseudo perhaps?

Done.

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1565263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:391:
selector->setRelationIsAffectedByPseudoSlotted();
On 2016/01/13 09:59:09, rune wrote:
> I don't think you need this flag at all, only the ShadowSlot combinator. The
> reason we need it for ::content, is that we have explicit combinators in the
> selector source and instead of inventing DescendantInsertionPointCrossing,
> ChildInsertionPointCrossing, we use Descendant and Child with the
> relation-is-affected bit set. In the css text, ::slotted will part of the same
> compound as the simple selectors matching the slot, but we introduce a
> ShadowSlot combinator and split the compound into two like we do for custom
> pseudo elements like in input::-webkit-*.
> 
> I've been trying to simplify this splitting process in
> https://codereview.chromium.org/1574323003/

Acknowledged.

[rune, 2016-01-15 11:01:16]

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html
(right):

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html:43:
"*::slotted(*) { display: block; }",
I thought the '*' should be dropped from this one? If it passes it's probably in
line with other explicit universal selectors.

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSSelector.cpp:802: case
CSSSelector::PseudoSlotted:
This method is used for validating selectors in <content select="...">. I doubt
PseudoSlotted should be allowed there?

[kochi, 2016-01-19 07:25:56]

I tried removing ShadowSlot implicit combinator, but
so far the change will be no simpler than using the
combinator, so I'd like to go with the current way,
using ShadowSlot implicit combinator.

The idea was to convert .slot::slotted(div) to
::slotted(div) - (relation:subselector) -> .slot
but it made the PseudoSlotted matching code more
complex for preparing the next SelectorMatchingContext
and satisfy scopeContaisLastMatchedElement() for all
use cases (e.g. "::slotted(div)", "slot::slotted(div)",
"span ::slotted(div)") at the same time.

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html
(right):

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html:43:
"*::slotted(*) { display: block; }",
On 2016/01/15 11:01:16, rune wrote:
> I thought the '*' should be dropped from this one? If it passes it's probably
in
> line with other explicit universal selectors.

This one has an explicit '*' on the left (see line 8), so it's expected, right?
Shall we drop '*' even when it is explicitly specified?
It doesn't improve any performance at matching, because we add an implicit
'*' if it's omitted.

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html:58:
"::slotted(span)::slotted(span) { color: red; }"
This case is now invalid and removed from this expectation list.

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSSelector.cpp:802: case
CSSSelector::PseudoSlotted:
On 2016/01/15 11:01:16, rune wrote:
> This method is used for validating selectors in <content select="...">. I
doubt
> PseudoSlotted should be allowed there?

Good catch.

It seems I mistakenly added this line.

This function should have returned false for pseudo elements
in another switch-case above
(case CSSSelector::PseudoElement: return false) already.

Removed this line.

[kochi, 2016-01-19 07:33:15]

Tim, could you also take a look?

[rune, 2016-01-19 09:08:28]

On 2016/01/19 07:25:56, kochi wrote:
> I tried removing ShadowSlot implicit combinator, but
> so far the change will be no simpler than using the
> combinator, so I'd like to go with the current way,
> using ShadowSlot implicit combinator.

Ack.

[rune, 2016-01-19 09:16:09]

lgtm

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html
(right):

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html:43:
"*::slotted(*) { display: block; }",
On 2016/01/19 07:25:56, kochi wrote:
> On 2016/01/15 11:01:16, rune wrote:
> > I thought the '*' should be dropped from this one? If it passes it's
probably
> in
> > line with other explicit universal selectors.
> 
> This one has an explicit '*' on the left (see line 8), so it's expected,
right?
> Shall we drop '*' even when it is explicitly specified?
> It doesn't improve any performance at matching, because we add an implicit
> '*' if it's omitted.

It's not expected as "*::before" is serialized as "::before", but I see that we
add unnecessary universal selectors in the serialization for "*::cue(i)" and
"*::-webkit-media-slider-container" as well, and ::slotted just follows that
pattern. No need to fix that here.

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html:58:
"::slotted(span)::slotted(span) { color: red; }"
On 2016/01/19 07:25:56, kochi wrote:
> This case is now invalid and removed from this expectation list.

Acknowledged.

https://codereview.chromium.org/1565263003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1565263003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:733: while
(splitAfter->tagHistory() &&
!splitAfter->tagHistory()->needsImplicitShadowCrossingCombinatorForMatching() &&
splitAfter->tagHistory()->pseudoType() != CSSSelector::PseudoSlotted)
You may move the PseudoSlotted into
needsImplicitShadowCrossingCombinatorForMatching() (and possibly drop "Crossing"
from the name, which might yield a better common name).

[kochi, 2016-01-19 11:09:12]

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html
(right):

https://codereview.chromium.org/1565263003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/shadow/slotted-pseudo-element-css-text.html:58:
"::slotted(span)::slotted(span) { color: red; }"
On 2016/01/19 09:16:09, rune wrote:
> On 2016/01/19 07:25:56, kochi wrote:
> > This case is now invalid and removed from this expectation list.
> 
> Acknowledged.

Thanks to your change!

https://codereview.chromium.org/1565263003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1565263003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:733: while
(splitAfter->tagHistory() &&
!splitAfter->tagHistory()->needsImplicitShadowCrossingCombinatorForMatching() &&
splitAfter->tagHistory()->pseudoType() != CSSSelector::PseudoSlotted)
On 2016/01/19 09:16:09, rune wrote:
> You may move the PseudoSlotted into
> needsImplicitShadowCrossingCombinatorForMatching() (and possibly drop
"Crossing"
> from the name, which might yield a better common name).

Done.

[rune, 2016-01-19 14:14:39]

On 2016/01/19 09:16:09, rune wrote:

> It's not expected as "*::before" is serialized as "::before", but I see that
we
> add unnecessary universal selectors in the serialization for "*::cue(i)" and
> "*::-webkit-media-slider-container" as well, and ::slotted just follows that
> pattern. No need to fix that here.

FYI, I have a fix for that here: https://codereview.chromium.org/1607873002/

[Timothy Loh, 2016-01-20 00:02:52]

lgtm

[kochi, 2016-01-20 02:11:05]

The CQ bit was checked by kochi@chromium.org

[kochi, 2016-01-20 02:11:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/1565263003/#ps180001
(title: "merge ::slotted checking in
needsImplicitShadowCombinatorForMatching()")

[commit-bot: I haz the power, 2016-01-20 02:13:42]

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

[commit-bot: I haz the power, 2016-01-20 02:45:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 02:45:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kochi, 2016-01-20 05:53:57]

On 2016/01/20 02:45:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Due to upstream change (by rune):
https://codereview.chromium.org/1600793002
now more than one pseudo elements in one selector list is not allowed.

Updated the layout test.

Rune, could you take another look?

[rune, 2016-01-20 08:15:54]

On 2016/01/20 05:53:57, kochi wrote:
> On 2016/01/20 02:45:47, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
> > (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> >   linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Due to upstream change (by rune):
> https://codereview.chromium.org/1600793002
> now more than one pseudo elements in one selector list is not allowed.
> 
> Updated the layout test.
> 
> Rune, could you take another look?

Still lgtm

[kochi, 2016-01-20 08:21:45]

The CQ bit was checked by kochi@chromium.org

[kochi, 2016-01-20 08:21:45]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1565263003/#ps200001
(title: "rebase, fix layout test.")

[commit-bot: I haz the power, 2016-01-20 08:22:47]

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

[commit-bot: I haz the power, 2016-01-20 10:13:58]

Description was changed from

==========
Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=fast/dom/shadow/slotted-pseudo-element-css-text.html
==========

to

==========
Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=fast/dom/shadow/slotted-pseudo-element-css-text.html
==========

[commit-bot: I haz the power, 2016-01-20 10:13:59]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-01-20 10:15:39]

Description was changed from

==========
Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=fast/dom/shadow/slotted-pseudo-element-css-text.html
==========

to

==========
Implement CSS parser part for ::slotted pseudo element

This CL is split off of
https://codereview.chromium.org/1523843004/

Implements ::slotted() pseudo element, whose spec is discussed at
https://github.com/w3c/webcomponents/issues/331
for Shadow DOM v1.

BUG=531990
TEST=fast/dom/shadow/slotted-pseudo-element-css-text.html

Committed: https://crrev.com/5dc8704a6c571f65f3d00731da773c2c5ebd8f2e
Cr-Commit-Position: refs/heads/master@{#370358}
==========

[commit-bot: I haz the power, 2016-01-20 10:15:40]

Patchset 11 (id:??) landed as
https://crrev.com/5dc8704a6c571f65f3d00731da773c2c5ebd8f2e
Cr-Commit-Position: refs/heads/master@{#370358}