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

[css-syntax] Dashed-ident rules and error recovery #9336

Closed
andruud opened this issue Sep 11, 2023 · 6 comments
Closed

[css-syntax] Dashed-ident rules and error recovery #9336

andruud opened this issue Sep 11, 2023 · 6 comments

Comments

@andruud
Copy link
Member

andruud commented Sep 11, 2023

After a recent edit in relation to relaxed nesting, css-syntax now says this:

<{-token>
If the first two non- values of rule’s prelude are an whose value starts with "--" followed by a , consume the remnants of a bad declaration from input, with nested, and return nothing.

Unless I'm mistaken, this means that encountering this situation top-level means we'll treat the entire rest of the stylesheet as a "bad declaration".

--foo:hover {
  color: red;
}
/* Lots of other innocent and valid rules here */

That is probably a bit extreme, and makes the change a bit hard to ship, since it could affects existing sites (drastically).

Perhaps we can limit the behavior to when nested is true? Or something else? @tabatkins

@tabatkins
Copy link
Member

Yup, this looks like just a mistake; I didn't mean for this hungry behavior to apply at the top-level.

The intention of the hungry behavior is that it's already very difficult for custom properties to actually ever be invalid, so an "ambiguous rule" like --foo:hover {...} would virtually never fail the "looks like a declaration" test and actually parse as a rule. To avoid that being a super rare case, I just catch it in the "consume a rule" algo and force it to continue to act like a custom property.

But since custom props (or any props at all) can't exist on the top level of a stylesheet, there's no reason to worry about it. That is, --really-a-prop:hover and-more-weird-values is just unreasonable to interpret as a property at the top level of a stylesheet.

So yeah, I propose that we do indeed guard against this. Specifically, in https://drafts.csswg.org/css-syntax/#consume-a-qualified-rule, in the {-token branch, if we see something that looks like a custom property, then we behave differently based on whether it's nested or not: if nested, we do the current behavior (force it to be a custom property and throw it away), if not nested, we just consume the {} block and then throw it away.

This maintains the constant I want to maintain about where --foo:hover {...} rules are allowed (nowhere), and avoids eating an entire stylesheet.

Agenda+ to confirm this with the WG.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-syntax] Dashed-ident rules and error recovery, and agreed to the following:

  • ACTION: TabAtkins to look into using a more generic term than 'nested' for the flag for mixed declaration+rule contexts
  • RESOLVED: at top level, if you see a rule that looks like a custom property, we consume as a rule and throw it away as invalid
The full IRC log of that discussion <TabAtkins> scribe+ TabAtkins
<fantasai> TabAtkins: We resolved the fundamental grammar ambiguity between properties and declarations in nesting by deciding we would start by parsing as a declaration, and if it was invalid, go backand parse as a rule
<fantasai> TabAtkins: recently tweaked to make more efficient
<fantasai> TabAtkins: works pretty fine, but a wrinkle about custom properties
<fantasai> TabAtkins: custom properties are very hard to make invalid
<fantasai> TabAtkins: anything you write in a custom property will always be valid
<fantasai> TabAtkins: so if you write --foo: [anything] that's a valid custom property
<fantasai> TabAtkins: in order to avoid a behavior change for authors
<fantasai> TabAtkins: if it looks like custom property, we treat it as a custom property during rule parsing
<fantasai> TabAtkins: if you have 'display:hover', can quickly tell it's not a valid display
<fantasai> TabAtkins: issue is that I didn't condition that check on being in a nested context
<fantasai> TabAtkins: so as written, if you have a top-level style rule starting with --foo:whatever, it will try to consume a custom property declaration
<fantasai> TabAtkins: it will eat everything up to the next top-level semicolon
<fantasai> TabAtkins: lose the rest of the stylesheet from that point forward
<fantasai> TabAtkins: I didn't think about this case, so proposal is to condition that check for "does this look like a custom property" to check whether in a nested context or not
<fantasai> TabAtkins: if not nested context, we let it parse as a rule
<fantasai> TabAtkins: but will do rule-parsing and end at next curly brace
<fantasai> TabAtkins: this means that behavior-wise, this brings us back to how this case would have acted before my recent change to allow nesting to work
<fantasai> TabAtkins: i.e. if you write a style rule --foo:whatever, it will only eat one style rule
<fantasai> TabAtkins: I've checked this with Emilio during TPAC
<astearns> q+
<astearns> ack fantasai
<TabAtkins> fantasai: so what this means is that parsing behavior for style rules with a custom ident is different in top level vs nested
<TabAtkins> fantasai: so if we have custom idents at beginnign of selector, will we be int rouble?
<fantasai> TabAtkins: In both cases, if you try to write a rule with --foo:something, you would end up with something invalid
<fantasai> TabAtkins: if it wasn't a valid custom property, you won't get a rule
<fantasai> TabAtkins: The difference in parsing behavior is, if it's nested it'll consume as a declaration
<fantasai> TabAtkins: whereas at top level it'll consume as a rule
<fantasai> TabAtkins: in both cases, can't be a rule
<fantasai> TabAtkins: we took as an implicit restriction on ourselves that if you have a style rule that wants to start with a dashed type selector, you have to spell it differently e.g. wrap in :is()
<fantasai> TabAtkins: considered that to be fine because that's not a valid custom element name in HTML or XML (I think?)
<fantasai> TabAtkins: very difficult to have a type selector that looks like that, can work around
<fantasai> TabAtkins: error recovery is different in both cases, but either way the rule is invalid and won't work
<fantasai> TabAtkins: and I think that's enough consistency
<fantasai> ok, sgtm
<fantasai> astearns: My qeustion is, defining it as working normally in nested context vs defining it as doing different thing in top-level
<fantasai> astearns: would that be exactly the same? I'm worried nested context are about CSS nesting, not a regular declaration block...
<fantasai> TabAtkins: the switch used in Syntax is "is it mixed with declarations"
<fantasai> astearns: ok, and nested context is that type of context
<fantasai> TabAtkins: yeah
<fantasai> TabAtkins: flag is called "nested" but in practice used for mixed cases
<fantasai> astearns: might want a different term
<fantasai> TabAtkins: I'll look into it
<fantasai> ACTION: TabAtkins to look into using a more generic term than 'nested' for the flag for mixed declaration+rule contexts
<fantasai> TabAtkins: Proposal is, at top level, if you see a rule that looks like a custom property, we consume as a rule and throw it away as invalid
<fantasai> RESOLVED: at top level, if you see a rule that looks like a custom property, we consume as a rule and throw it away as invalid

@cdoublev
Copy link
Collaborator

cdoublev commented Jan 4, 2024

I do not see in which nested context --foo:hover { color: blue; } not-semicolon would not be consumed as a (valid) declaration.

So I do not understand why checking nesting is needed (in consume a qualified rule, cf. previous comment) instead of consuming {} indifferently whether the consumed rule is nested or not.


(I'm trying to guess from here, if that helps explain to me what I am missing.)

From the note titled What's this check for? below the consume a qualified rule algo (and also in the previous comment):

This ensures that --foo:hover { color: blue; } is consistently invalid everywhere [...]

Does it mean that this is consistently an invalid rule everywhere?

Authors can write a custom property that takes a {}-block in its value, even combined with other thing; if that custom property is then invalid (due to an invalidly-written var() function, for example), [...]

Do you mean that --foo: var(<not-custom-property-name>) should fail to be consumed as a declaration?

2.2 Error Handling says that the grammar must be checked after each construct is parsed, but it is non-normative and it seems inefficient to retry parsing a declaration as a rule because its value was invalid according to the grammar.

Maybe I misunderstood #8834 (comment), ie. what it means to check that a declaration or rule is valid in the context (which I think is not required anymore, btw).

@tabatkins
Copy link
Member

--foo:hover { color: blue; } var(1); will fail to parse as a valid declaration, and instead attempt to be parsed as a qualified rule.

So I do not understand why checking nesting is needed

In a nested context, a --foo: ... is probably a declaration, so consuming the whole thing to next the same-level semicolon is likely to be the correct behavior.

At the top level, properties aren't allowed; if you've written --foo:... it's much more likely you are trying to write a rule. You don't see many stray color: blue; declarations at the top level of a stylesheet, after all. So we should consume just a rule-worth of tokens; that's more likely to match what the author intended. Plus, since declarations aren't valid at the top level, it's very unlikely that a top-level semicolon will appear at all, so a stray --foo:... would likely consume the entire rest of the stylesheet if it was treated as a declaration.

Do you mean that --foo: var(<not-custom-property-name>) should fail to be consumed as a declaration?

Correct. That's step 9 of "consume a declaration"

it seems inefficient to retry parsing a declaration as a rule because its value was invalid according to the grammar.

Sure, it would be inefficient. But you don't actually have to try, you just have to act identically to an impl that did try. The vast majority of the time you can just quit immediately, or at least very quickly. See the impl notes at the end of "consume a block's contents", which offers guidance about this exact topic.

@tabatkins
Copy link
Member

ACTION: TabAtkins to look into using a more generic term than 'nested' for the flag for mixed declaration+rule contexts

Closing the loop on this action:

Most of the "consume" algorithms use the "nested" flag. In almost all cases it really does mean "am I nested in a {} block", because it's used to determine whether a } stops the parsing immediately (because it's the closing brace of a parent block) or not.

The only exception is in "consume a qualified rule": in addition to the above-described usage, it's also used to decide how to handle a custom property. In theory this is referencing the "mixed declarations and rules, or just rules" decision, but in practice it's to make a custom property showing up at the top level of a stylesheet work better (so it doesn't consume the entire rest of the stylesheet), so it's actually caring about nesting too.

So, I'm concluding that "nested" is still an appropriate name for this flag, and this action needs no further change.

@cdoublev
Copy link
Collaborator

cdoublev commented Jan 5, 2024

Enlightening, thanks.

mdubet added a commit to mdubet/WebKit that referenced this issue Jul 8, 2024
https://bugs.webkit.org/show_bug.cgi?id=276302
rdar://131274469

Reviewed by NOBODY (OOPS!).

w3c/csswg-drafts#9336 (comment)

* LayoutTests/imported/w3c/web-platform-tests/css/css-syntax/custom-property-rule-ambiguity-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-syntax/custom-property-rule-ambiguity.html:
* Source/WebCore/css/parser/CSSParserImpl.cpp:
(WebCore::CSSParserImpl::consumeQualifiedRule):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants