1266283 - Implement CSS4 descendant combinator >>

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, enhancement)

Description

[Chris Rebert]

Selectors Level 4 adds a non-whitespace spelling of the descendant combinator in the form of the ">>" combinator.
See https://drafts.csswg.org/selectors-4/#descendant-combinators

Currently implemented in WebKit; see http://caniuse.com/#feat=css-descendant-gtgt

Comment 1

[Thomas Wisniewski]

Attachment: 1.diff

Hello, first time contributor here!

jfkthame on #developers suggested that I submit my patches with heycam as initial reviewer, so here's hoping that I'm doing it right.

Comment 2

[Thomas Wisniewski]

Attachment: 2.diff

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think it would probably be better to call mScanner->Next() directly than to add another (confusing-to-read) boolean to GetToken.  (Alternatively, we could perhaps pass an enum to GetToken, probably by renaming the enum passed to nsCSSScanner::Next to something a bit shorter.  But that's a far bigger change.)

Comment 4

[Thomas Wisniewski]

Attachment: 1266283-add-support-for-css4-descendant-combinator.diff

Alright, that makes sense. I've modified the patch to just use Next() and manage the pushback state manually, as that was easy enough. But if the other approach is better, I don't mind doing that instead.

Thanks for the quick review!

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  That wasn't really a full review, but anyway...

I'm a little hesitant about storing two different values for the same thing in the internal data structure because there's a risk that we'll get things wrong now, and in future changes (e.g., a future optimization that ends up doing the wrong thing when a selector is specified as >> because it was only tested with space).

What does the spec say about serialization?  Should this new selector be serialized as a space, or as >>?

If the latter, I might be inclined to suggest that to avoid that risk, we split:
  char16_t mOperator;
into:
  char mOperator;
  int8_t mBits;
and make the first selector bit be CSS_SELECTOR_DESCENDANT_COMBINATOR_AS_DOUBLE_CHILD or something like that.


Also, what status is the spec in?  Is it known to be ready for implementation?

Comment 6

[Chris Rebert]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #5)
> What does the spec say about serialization?  Should this new selector be
> serialized as a space, or as >>?

Sounds like >>. Per https://drafts.csswg.org/cssom/#serializing-selectors
> To serialize a selector [...]:
> [...]
> 3. If this is not the last part of the chain of the selector append a single SPACE (U+0020),
> followed by the combinator ">", "+", "~", ">>", "||", as appropriate,
> followed by another single SPACE (U+0020) if the combinator was not whitespace, to s.

Comment 7

[Thomas Wisniewski]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #5)
> Hmmm.  That wasn't really a full review, but anyway...

Oh don't worry, I wasn't jumping that far ahead :)


> split:
>   char16_t mOperator;
> into:
>   char mOperator;
>   int8_t mBits;

I don't have any issue with that. There are also the || and >>> operators, so a setup like this may be for the best. Perhaps it would even be better to just make mOperator itself an enum, if the concern is confusion over split variables and the like. I don't recall seeing this particular mOperator being referenced by any other files than the ones I'm touching, if that helps in making a decision.


>What does the spec say about serialization?  Should this new selector be serialized as a space, or as >>?

As Chris says, the current wording implies that it should be serialized as-is, rather than as a space. I'm not at all sure, but I also believe the favored approach for devtools is to match the user's input rather than normalizing it, so perhaps that's also a useful factor to consider.


> Also, what status is the spec in?  Is it known to be ready for implementation?

All I can really say is that it's currently implemented in the Safari Tech Preview, so it's in WebKit (whether they push it out soon or not). But I'm not in a rush to get patches accepted or anything, so I don't mind letting this simmer.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We should probably figure out whether this is something we want before people start shipping it.

Comment 9

[Thomas Wisniewski]

Agreed. All I can offer is that it looks as though it was resolved to remove >>>, but Chrome found there was too much real-world usage last year to do so, and haven't removed it yet: https://bugs.chromium.org/p/chromium/issues/detail?id=489954

If >>> is going to be eventually removed then the reason for adding >> seems to be gone (the reason being that it's nice to have a >> to sit between the > and >>> combinators, rather than using /deep/ ).

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Thomas Wisniewski from comment #9)
> Agreed. All I can offer is that it looks as though it was resolved to remove
> >>>, but Chrome found there was too much real-world usage last year to do
> so, and haven't removed it yet:
> https://bugs.chromium.org/p/chromium/issues/detail?id=489954

It's still in https://drafts.csswg.org/css-scoping/#deep-combinator .  Where was this resolution?

> If >>> is going to be eventually removed then the reason for adding >> seems
> to be gone (the reason being that it's nice to have a >> to sit between the
> > and >>> combinators, rather than using /deep/ ).

agreed.

Comment 11

[Thomas Wisniewski]

>Where was this resolution?
The Chrome bug linked to these meeting notes: https://www.w3.org/wiki/Webapps/WebComponentsApril2015Meeting

>E. Shadow boundary-piercing combinators
>Apple:	Must not work in closed mode; think named parts is better model
>Mozilla: Would like to postpone piercing to v2
>Microsoft: Would like a named parts model
>Google: Think piercing was a mistake, should be removed
>Resolution: Remove

Comment 12

[Sebastian Zartner [:sebo]]

As the descendant combinator got removed from the spec., I assume this bug can be closed, right?

Sebastian

Comment 13

[Thomas Wisniewski]

Yes, the /deep/ combinator has been removed from the spec, and I believe Chrome also removed it in M63.

Comment 14

[Will Bamberg [:wbamberg]]

No changes to Firefox here, but I've noted in the compat data that this is now deprecated: https://developer.mozilla.org/en-US/docs/Web/CSS/Descendant_selectors#Browser_compatibility.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8755645 [details] [diff] [review]
1266283-add-support-for-css4-descendant-combinator.diff

Cancelling review since the feature was removed from the spec, and since the patch is to the old style system.