Implement CSS4 descendant combinator >>

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
--
enhancement
a year ago
7 months ago

People

(Reporter: Chris Rebert, Unassigned)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: [parity-webkit], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Updated

a year ago
Blocks: 693083
Whiteboard: [parity-webkit]
(Reporter)

Updated

a year ago
Keywords: dev-doc-needed

Comment 1

a year ago
Created attachment 8755560 [details] [diff] [review]
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.
Attachment #8755560 - Flags: review?(cam)

Comment 2

a year ago
Created attachment 8755561 [details] [diff] [review]
2.diff
Attachment #8755561 - Flags: review?(cam)
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

a year ago
Created attachment 8755645 [details] [diff] [review]
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!
Attachment #8755560 - Attachment is obsolete: true
Attachment #8755561 - Attachment is obsolete: true
Attachment #8755560 - Flags: review?(cam)
Attachment #8755561 - Flags: review?(cam)
Attachment #8755645 - Flags: review?(dbaron)
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?
(Reporter)

Comment 6

a year ago
(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

a year ago
(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.
We should probably figure out whether this is something we want before people start shipping it.
Flags: needinfo?(dbaron)

Comment 9

9 months ago
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/ ).
(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.
Flags: needinfo?(dbaron)

Comment 11

9 months ago
>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
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.