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.

Sign up for GitHub

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

Jump to bottom

Fix nesting #41213

Merged
merged 4 commits into from Jul 27, 2023
Merged

Fix nesting #41213

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed7419e
Carry over fixes from #41050
tabatkins Jul 27, 2023
2604802
Fix how relative and non-relative selectors serialize to be per spec.
tabatkins Jul 27, 2023
b599bb2
Add another test for multi-ampersand.
tabatkins Jul 27, 2023
4ae6aab
Make the simple selector ordering tests not overlap with other tests …
tabatkins Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 32 additions & 14 deletions css/css-nesting/parsing.html
View file
Open in desktop
Expand Up @@ -23,12 +23,12 @@
assert_equals(ss.rules.length, 1, "Outer rule should exist.");
const rule = ss.rules[0];
assert_equals(rule.cssRules.length, 1, "Inner rule should exist.");
const inner_rule = rule.cssRules[0];
assert_equals(inner_rule.selectorText, expected, `Inner rule's selector should be "${expected}".`);
const innerRule = rule.cssRules[0];
assert_equals(innerRule.selectorText, expected, `Inner rule's selector should be "${expected}".`);
}, ruleText);
}

function testInvalidSelector(sel, {parent=".foo"}={}) {
function testInvalidNestingSelector(sel, {parent=".foo"}={}) {
resetStylesheet();
const ruleText = `${parent} { ${sel} { color: green; }}`
test(()=>{
Expand All @@ -39,30 +39,48 @@
}, "INVALID: " + ruleText);
}

// basic usage
testNestedSelector("&");
testNestedSelector("&.bar");
testNestedSelector("& .bar");
testNestedSelector("& > .bar");
testNestedSelector("> & .bar");

// relative selector
testNestedSelector("> .bar", {expected:"& > .bar"});
testNestedSelector("> & .bar", {expected: "& > & .bar"});
testNestedSelector("+ .bar &", {expected:"& + .bar &"});
testNestedSelector(".test > & .bar");
testNestedSelector("+ .bar, .foo, > .baz", {expected:"& + .bar, & .foo, & > .baz"});
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

testNestedSelector(".test > & .bar");
testNestedSelector(".foo, .foo &");
testNestedSelector(":is(.bar, .baz)");
testNestedSelector("&:is(.bar, .baz)");
testNestedSelector(":is(.bar, .baz)", {expected:"&:is(.bar, .baz)"});
testNestedSelector("&:is(.bar, &.baz)");
testNestedSelector(":is(.bar, &.baz)");
testNestedSelector("&:is(.bar, &.baz)");

// Mixing nesting selector with other simple selectors
testNestedSelector("div&");
testNestedSelector(".bar&");
testNestedSelector("[bar]&");
testNestedSelector("#bar&");
testInvalidNestingSelector("&div"); // type selector must be first
testNestedSelector(".class&");
testNestedSelector("&.class");
testNestedSelector("[attr]&");
testNestedSelector("&[attr]");
testNestedSelector("#id&");
testNestedSelector("&#id");
testNestedSelector(":hover&");
testNestedSelector("&:hover");
testNestedSelector(":is(div)&");
testNestedSelector(".bar > &");
testNestedSelector("&:is(div)");

// Multiple nesting selectors
testNestedSelector("& .bar & .baz & .qux");
testNestedSelector("&&");

// Selector list in inner rule
testNestedSelector("& > section, & > article");
testNestedSelector("& + .baz, &.qux", {parent:".foo, .bar"});

testInvalidSelector("&div");
// Selector list in both inner and outer rule.
testNestedSelector("& + .baz, &.qux", {parent:".foo, .bar"});
</script>