Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nesting #41213

Merged
merged 4 commits into from Jul 27, 2023
Merged

Fix nesting #41213

merged 4 commits into from Jul 27, 2023

Conversation

tabatkins
Copy link
Contributor

Fixes some bugs uncovered in #41172 comments.

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabatkins tabatkins enabled auto-merge (squash) July 27, 2023 23:32
@tabatkins tabatkins merged commit 4026105 into web-platform-tests:master Jul 27, 2023
17 checks passed
emilio added a commit that referenced this pull request Jul 28, 2023
This one is the only one that mixes relative selectors with non-relative ones. Probably missed by #41213
testNestedSelector(".foo, .foo &", {expected:"& .foo, .foo &"});

// implicit relative (and not)
testNestedSelector(".foo");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabatkins Why do we not expect & .foo here? I see nothing in the recent edit that does anything special for relative selectors with a descendant combinator? https://drafts.csswg.org/css-nesting-1/#cssom

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andruud I'm confused, my understanding is that these are not really relative selectors.

I guess they do match the <relative-selector> grammar without a combinator, tho... But you can't really serialize & unconditionally for all those, because then .foo & would become & .foo &, which is not really equivalent / what you want, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilio: .foo is a relative selector, but .foo & is not a relative selector, see the first two bullet points here: https://drafts.csswg.org/css-nesting-1/#syntax

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Well both are relative selectors per syntax, and none have an extra combinator, but yeah, you're right.

I don't think we want to serialize the implied combinator, since not having it doesn't make the selector invalid in any other context (unlike for relative selectors that actually start with a combinator), but I wouldn't mind either way I guess, if you have a strong reason to prefer serializing the & there...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serializing it seems more consistent, so I'd prefer that, but I can live with anything as long as WPTs agree with the spec. :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, given no response from @tabatkins: since the spec describes the behavior, the call notes doesn't say otherwise, and @emilio says he can live with it, then I think we'll just change the WPT to expect that relative .foo is serialized as & .foo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's slightly unfortunate, fwiw, but I can live with it yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry for missing this comment!

I don't have much of an opinion on which way we go. You're right that, per a strict reading, the inner selector of .foo { .bar {...}} is a relative selector - nested style rules use <relative-selector-list> for their grammar, implying that their selectors are all "relative selectors", and only exempt one case from that interpretation (when the selector contains an explicit &). Then the CSSOM section does say that you must absolutize all relative selectors, inserting the implied &.

We added that serialization requirement to make sure that the selectors were valid when transferred to other contexts, and that reasoning doesn't have anything to say about a selector like ".foo" - both ".foo" and "& .foo" would be valid when moved around (and approximately equal in behavior, except that the latter can't select the root element).

Whether we fix this by adjusting the WPT, or by fixing the spec to only insert the & when it's needed to prevent the selector from starting with a combinator (and thus being invalid in non-nested contexts), I don't care too much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor preference for changing the spec, but can live with either really.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the WPT to follow the spec here #42560 if we want to go in this direction

mdubet pushed a commit to mdubet/wpt that referenced this pull request Oct 16, 2023
The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests#41213 (comment)
mdubet pushed a commit to mdubet/wpt that referenced this pull request Oct 16, 2023
The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests#41213 (comment)
mdubet pushed a commit to mdubet/wpt that referenced this pull request Nov 9, 2023
The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests#41213 (comment)
mdubet added a commit that referenced this pull request Nov 9, 2023
The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

#41213 (comment)

Co-authored-by: Matthieu Dubet <mdubet@apple.com>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 22, 2023
…selector with &, a=testonly

Automatic update from web-platform-tests
[CSS Nesting] Always serialize relative selector with & (#42560)

The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests/wpt#41213 (comment)

Co-authored-by: Matthieu Dubet <mdubet@apple.com>
--

wpt-commits: 2e87edd0f508a08d823d4f3992ea41b0a2ef5611
wpt-pr: 42560
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 22, 2023
…selector with &, a=testonly

Automatic update from web-platform-tests
[CSS Nesting] Always serialize relative selector with & (#42560)

The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests/wpt#41213 (comment)

Co-authored-by: Matthieu Dubet <mdubet@apple.com>
--

wpt-commits: 2e87edd0f508a08d823d4f3992ea41b0a2ef5611
wpt-pr: 42560
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Nov 24, 2023
…selector with &, a=testonly

Automatic update from web-platform-tests
[CSS Nesting] Always serialize relative selector with & (#42560)

The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests/wpt#41213 (comment)

Co-authored-by: Matthieu Dubet <mdubet@apple.com>
--

wpt-commits: 2e87edd0f508a08d823d4f3992ea41b0a2ef5611
wpt-pr: 42560
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Nov 24, 2023
…selector with &, a=testonly

Automatic update from web-platform-tests
[CSS Nesting] Always serialize relative selector with & (#42560)

The current spec mandates to always serialize the implicit &,
even when it's not necessary (such as .foo)

web-platform-tests/wpt#41213 (comment)

Co-authored-by: Matthieu Dubet <mdubet@apple.com>
--

wpt-commits: 2e87edd0f508a08d823d4f3992ea41b0a2ef5611
wpt-pr: 42560
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
* Carry over fixes from web-platform-tests#41050

* Fix how relative and non-relative selectors serialize to be per spec.

* Add another test for multi-ampersand.

* Make the simple selector ordering tests not overlap with other tests in the file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants