Make a '::distributed' pseudo element the first-ever client who can accept a relative selector.

Make a '::distributed' pseudo element the first-ever client who can accept a relative selector.

The spec of relative selectors is:
   http://dev.w3.org/csswg/selectors4/#relative

'::distributed' can be categorized as the following use case mentioned in the spec:
> if the reference element set consists of only a virtual scoping root:

This patch hasn't implemented the following case, which is now treated as a parsing error. I'll address that in a follow-up patch.
> 2. Otherwise, if the selector starts with any combinator other than the descendant combinator, change the selector to ‘:not(*)’.

I also have a plan to support relative selectors in other use cases such as Element.find(relative_selectors). But today is not the day.

BUG=234028
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151172

[hayato, 2013-05-22 04:47:36]

r?

[dglazkov, 2013-05-22 17:43:43]

https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSParser.cpp...
Source/core/css/CSSParser.cpp:11134:
end->setRelation(CSSSelector::ShadowVirtualChild);
This is where it's fairly clear that we're just overloading Relation.

https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSSelector.h
File Source/core/css/CSSSelector.h (right):

https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSSelector.h...
Source/core/css/CSSSelector.h:84: ShadowVirtualChild
Introducing "virtual" here is bugging me. What does the word mean in this
context? I understand that you're aiming for a "virtual scoping root", but worry
that it isn't clear.

Another thought: isn't adding the "virtual" bit into Relation sort of
overloading it?

https://codereview.chromium.org/15657003/diff/1/Source/core/css/SelectorCheck...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/15657003/diff/1/Source/core/css/SelectorCheck...
Source/core/css/SelectorChecker.cpp:230: SelectorChecker::Match
SelectorChecker::matchForVirtualChild(const Element* element, const
SiblingTraversalStrategy& siblingTraversalStrategy, PseudoId& dynamicPseudo,
SelectorCheckingContext& nextContext) const
This is interesting. Right now, it's clear that "virtual scoping root" is a very
shadow DOM-specific concept.

However, it might be helpful to think how would we implement _both_ .find and
::distributed()? I think we we would come up with some way to supply the
mechanism for matching "virtual scoping root" in the SelectorCheckingContext,
right?

And then we'd check some bit on the selector (that is now overloaded in
Relation) in Descendant/Child code and traverse up using that mechanism, rather
than parentElement.

This seems to be a slightly different approach from what you have. Can you help
me understand why I am wrong? :)

[hayato, 2013-05-23 07:34:44]

Thank you for the review. Let me expalin my thought and my approach more
clearly.

As for overloading '>', IMO,

 - The spec is overloading the meaning of '>'.
 - So the implementation has to resolve overloaded '>'. The result of resolution
should be stored as a different relation so that SelectorChecker can distinguish
them easier.

Until now, a relation of '>' is always used in the sense that 'A > B' implies
'B.parentNode == A'.

But the thing has changed. We cannot leave this relation as is if it is used in
virtual scoping root context since we never have such a A. A cannnot be
insertion point sicne 'B.parentNode != insertionPoint'

To keep CSSSelector and SelectorChecker simple one, the best idea so far for me
is assigning a new relation to that, 'virtual child', which implies the
following relation: 'B.virtualParent = insertionPoint'.


It might be worth to mention my original idea which I've abandoned.
Let's suppose we are given the following selector:
  C::distributed(> A B)

In my first approach, I've tried translate it to the following CSSSelector:

  [C] <-(ShadowDistributed relation)-  [A] <- [:virtual-scoping-root pseudo
class]   <-(Descendant relation)- [B]

But that makes SelectorChecker's implementation too complicated. We need to
store yet another dedicated virtual scoping root if we meet
':virtual-scoping-root' in SelectorCheckingContext so that we don't traverse up
ancestors when we have to resolve relation of '[C] <-(ShadowDistributed
relation)-  [A]' later. For me, this approach is complex one at least now.

So I've changed my mind. Now it is translated to the following CSSSelector using
a new relation:

  [C] <-(ShadowVirtualChild relation)-  [A]  <-(Descendant relation)- [B]

That makes implementation much simpler.



On 2013/05/22 17:43:43, Dimitri Glazkov wrote:
> https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSParser.cpp
> File Source/core/css/CSSParser.cpp (right):
> 
>
https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSParser.cpp...
> Source/core/css/CSSParser.cpp:11134:
> end->setRelation(CSSSelector::ShadowVirtualChild);
> This is where it's fairly clear that we're just overloading Relation.
> 
> https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSSelector.h
> File Source/core/css/CSSSelector.h (right):
> 
>
https://codereview.chromium.org/15657003/diff/1/Source/core/css/CSSSelector.h...
> Source/core/css/CSSSelector.h:84: ShadowVirtualChild
> Introducing "virtual" here is bugging me. What does the word mean in this
> context? I understand that you're aiming for a "virtual scoping root", but
worry
> that it isn't clear.

My original idea is using the following names: :)
  - ShadowDescendant (That's the motivation of the change of
https://codereview.chromium.org/15363004/. I'd like to use ShadowDescendant as a
replacement of ShadowDistributed).
  - ShadowChild

Yeah, 'virtual' came from a 'virtual scoping root' in the selectors spec. I'd
like to have consistent names for them (ShadowDescendant and ShadowChild). I
don't have a strong opinion about naming. Any suggestions are welcome.

What about these names?
 - ShadowDistributedDescendant
 - ShadowDistributedChild


> 
> Another thought: isn't adding the "virtual" bit into Relation sort of
> overloading it?
> 
>
https://codereview.chromium.org/15657003/diff/1/Source/core/css/SelectorCheck...
> File Source/core/css/SelectorChecker.cpp (right):
> 
>
https://codereview.chromium.org/15657003/diff/1/Source/core/css/SelectorCheck...
> Source/core/css/SelectorChecker.cpp:230: SelectorChecker::Match
> SelectorChecker::matchForVirtualChild(const Element* element, const
> SiblingTraversalStrategy& siblingTraversalStrategy, PseudoId& dynamicPseudo,
> SelectorCheckingContext& nextContext) const
> This is interesting. Right now, it's clear that "virtual scoping root" is a
very
> shadow DOM-specific concept.

Yeah, now, these relations are very specific to Shadow DOM. I have a concern for
that too.
But I'd like to defer a general implementation of 'virtual scoping root' until
we need a general one.


> 
> However, it might be helpful to think how would we implement _both_ .find and
> ::distributed()? I think we we would come up with some way to supply the
> mechanism for matching "virtual scoping root" in the SelectorCheckingContext,
> right?
> 
> And then we'd check some bit on the selector (that is now overloaded in
> Relation) in Descendant/Child code and traverse up using that mechanism,
rather
> than parentElement.

Good question. I have a local branch which is trying to implement a relative
selector which is used in Element.find(..) context.

These two kinds of relative selectors must be implemented differently, IMO.

For a relative selector used in Element.find() context, we can use '>' as is.
This is a good old relation. We don't need to resolve 'overloading' here.
Just absoluting the selector using ':scope' should be enough. That's
straightforward case.

I guess this patch supports a *special* relative selector as the first case. I'm
afraid that it makes things confusing. If I supprot a relative selector which is
used in Element.find(..), I could reduce confusion, but I'm not sure :)

Does the explanation make sense? I guess adding a relation should be avoided in
general. But I think we can consider the case this patch handles a new relation
that is worth adding if we can get benefits from that.


> 
> This seems to be a slightly different approach from what you have. Can you
help
> me understand why I am wrong? :)

[dglazkov, 2013-05-23 17:23:02]

On 2013/05/23 07:34:44, hayato wrote:
> So I've changed my mind. Now it is translated to the following CSSSelector
using
> a new relation:
> 
>   [C] <-(ShadowVirtualChild relation)-  [A]  <-(Descendant relation)- [B]
> 
> That makes implementation much simpler.

I agree. I think you have the right idea. What I was suggesting is not a great
departure:

[C] <-(Child relation + ["use virtual scoping root" bit])-  [A]  <-(Descendant
relation)- [B]
 
In other words, instead of storing the information in Relation, add a bit to the
CSSSelector (sort of like m_isForPage).

Then, in SelectorChecker, you split the path not at the point for testing for
Relation, but rather inside of CSSSelector::Child/Descendant case block.

This would allow you to avoid overwriting relation in CSSParser and avoid adding
a whole new relation, to keep CSSSelector::Relation in check with the Selectors
spec.

To reiterate -- I totally agree with your approach, just suggesting a way that's
more aligned with the terminology of the spec.

Does that make sense?

[hayato, 2013-05-24 02:17:48]

On 2013/05/23 17:23:02, Dimitri Glazkov wrote:
> On 2013/05/23 07:34:44, hayato wrote:
> > So I've changed my mind. Now it is translated to the following CSSSelector
> using
> > a new relation:
> > 
> >   [C] <-(ShadowVirtualChild relation)-  [A]  <-(Descendant relation)- [B]
> > 
> > That makes implementation much simpler.
> 
> I agree. I think you have the right idea. What I was suggesting is not a great
> departure:
> 
> [C] <-(Child relation + ["use virtual scoping root" bit])-  [A]  <-(Descendant
> relation)- [B]
>  
> In other words, instead of storing the information in Relation, add a bit to
the
> CSSSelector (sort of like m_isForPage).

Sounds good. Let me do it!

> 
> Then, in SelectorChecker, you split the path not at the point for testing for
> Relation, but rather inside of CSSSelector::Child/Descendant case block.
> 
> This would allow you to avoid overwriting relation in CSSParser and avoid
adding
> a whole new relation, to keep CSSSelector::Relation in check with the
Selectors
> spec.
> 
> To reiterate -- I totally agree with your approach, just suggesting a way
that's
> more aligned with the terminology of the spec.
> 
> Does that make sense?

That makes sense.

One cons is that we cannot get benefits from a compiler check for ensuring
completeness of switch statements any more if we don't use relations. But it's
trivial cons, I think.

[hayato, 2013-05-24 06:13:39]

I've updated the patch.
r?

[dglazkov, 2013-05-24 17:19:30]

lgtm

[commit-bot: I haz the power, 2013-05-27 02:22:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hayato@chromium.org/15657003/10001

[commit-bot: I haz the power, 2013-05-27 04:40:02]

Change committed as 151172