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-nesting-1] Relaxed nesting and var() #9317

Closed
andruud opened this issue Sep 7, 2023 · 14 comments
Closed

[css-nesting-1] Relaxed nesting and var() #9317

andruud opened this issue Sep 7, 2023 · 14 comments
Labels
css-nesting-1 Current Work

Comments

@andruud
Copy link
Member

andruud commented Sep 7, 2023

In #7961, we resolved to allow nested style rules that begin with ident-token, and use parser restarts to solve the ambiguity that arose from that.

However, there's a serious problem that we missed: var() in standard properties.

div {
  width:is(.x1) {
    /* ... */
  }
  width:is(.x2) {
    /* ... */
  }
  /* ... */
  width:is(.xN) {
    /* ... */
  }
}

The idea from #7961 is that when standing at width :, we'll quickly back out of declaration parsing, because the next token (is() does not match width's property grammar. Restarting and trying again as a nested style rule is cheap, because we won't proceed too far into the tokens before discovering that it's not a valid declaration.

However, var() interferes with this plan:

div {
  width:is(.x1) {
    /* ... */
  }
  width:is(.x2) {
    /* ... */
  }
  /* ... */
  width:is(.xN) {
    /* ... */
  }
  var(--x) /* <====== New */
}

Now we have a valid width declaration (parse-time) consisting of everything from the first width to the end of the div-block. The problem is not that var() is present, but that it can be present. Even when it's not there, we have to scan the whole rest of the block to check whether it is or not. If it's not there, we have O(N^2) behavior, because we'll consume the whole rest of the block at width:is(.x1), restart as a style rule, and then repeat at width:is(.x2).

Another related problem (from @tabatkins) is the following:

div {
  width:focus {
    color: var(--x);
  }
}

Per #7961, this is a valid width declaration, which isn't a great result. Especially since you'd get a rule if you do the same thing minus the var().

We could solve this problem one of the following ways:

  1. Giving up on relaxed nesting completely.
  2. Ban {} entirely from declarations. With this we can stop when we see {.
  3. Ban {} from declarations, unless it's the whole value (from @tabatkins). This is probably also acceptable, because we can look at the first non-whitespace token after : and if that's not {, then we can stop when we do see {. Otherwise we stop after the first }.

I would not go any weaker than (3), in particular the previously discussed restriction of "blocks are valid, but only at the end" is not good enough, because authors can realistically put any amount of stuff inside that block (if it's intended as a nested style rule), and also it wouldn't fix the problem illustrated by the last code snippet in this issue.

cc @emilio @mdubet

@emilio
Copy link
Collaborator

emilio commented Sep 7, 2023

Yeah, I agree the last snippet is quite unfortunate.

This is an issue for known property names, but also for --foo, right? Those are technically idents as well.

I thought banning { from declarations whole-sale wasn't acceptable because people put json like things in their custom properties? But I might be mis-remembering.

@andruud
Copy link
Member Author

andruud commented Sep 7, 2023

I don't think we have to do anything for custom properties due to this resolution in #7961:

3. --ident always kicks off declaration (not rule) parsing.

Not sure how this ended up actually being specified, but one possibility we discussed was to simply fail style rule parsing for anything that starts with a dashed ident.

@tabatkins
Copy link
Member

However, there's a serious problem that we missed: var() in standard properties.

"serious problem" is slightly hyperbolic; these problems only occur if you have a certain class of "ambiguous rules" - rules whose selectors start with a type selector that matches a valid property name, followed by a pseudo-class. All other rules early-exit property parsing within the first few tokens, and aren't an issue. These ambiguous rules are, currently and for the forseeable future, essentially unknown in practical CSS. The only overlaps between HTML/SVG/MathML element names and CSS property names are font and marker, both of which aren't really an issue here. (Custom element names can potentially make this more of an issue; an author could create an <align-content> element and try to style it, for example. So it's not something that'll never happen, but it'll still be very rare imo.)

Further, the N^2 behavior is only N^2 in the number of ambiguous rules among a given rule's child rules. If someone writes 1000 child rules in a single parent rule, and every single one is ambiguous, yeah it's not great. But is that realistically a situation that'll happen in practice? I don't think so.

The "if there is a var() we still consume the ambiguous rules as a declaration" one is a little more problematic, yeah. Still gated behind an author writing an ambiguous rule, but if that does happen, then having a var() in the child rules is a very realistic scenario.

So, I don't think this is a strictly necessary fix, as in we-can't-ship-if-this-isn't-fixed; this will not actually affect authors in meaningful ways. But it would definitely be nice to fix.


So, solutions!

The first solution is indeed weaker than your #3, Anders: we've already accepted a future design constraint on ourselves that if we ever included curly blocks in property values, we'll restrict them to the end of the value. This ensures that a browser that doesn't understand the property (and thus fails decl parsing and restarts with rule parsing) will have their parser re-sync with that of a newer browser virtually immediately; they'll consume tokens up to the block, throw it away as an invalid rule, then try to consume the ;, throw it away too, then it's re-synced.

So, we could make that design constraint an official syntax constraint: outside of custom properties (which will continue to allow anything), curly blocks can only appear at the end of declarations. We then make this restriction have the ability to cause a var()-containing property to fail at parse time.

With this, an ambiguous rule will first parse as a decl, getting through the entire {} block. Then it will keep parsing to try and find the ; or the !important or the end of the parent block (since it knows {} is only valid at the end of the property); when it instead runs into the start of the next selector, it'll know this is an invalid property and restart with the rule parser.

This solves both problems reasonably well. The N^2 behavior goes away entirely; instead you just pay parsing costs twice per ambiguous rule, which is what you'll already do for invalid properties. The only remaining downside is that ambiguous rules don't let us early-exit the property parser; we will still end up sucking down the entire rule, which might be big, before we finally hit the end and are allowed to fail and retry with rule parsing. The "ambiguous child rule contains a var(), which forces us to consider it a valid declaration" problem also mostly goes away; it'll only still occur if the ambiguous rule is the final thing in the parent rule.


But I think your #3 possibility is also just fine. We have zero plans to allow curly blocks in properties at all right now; the only suggestions for doing so have been to allow something like inlining a bunch of longhands into a shorthand. So, slightly widening our design prohibition from "must be last in the value" to "must be the entire value" does not preclude literally anything that's ever been suggested, afaik. And then, yeah, you get even better behavior, since a valid property with a curly in it will always start like foo: {, and that is by definition not a valid selector for a block, so that breaks the ambiguity both entirely and immediately.

This avoids the remaining downsides of my above suggestion: you can always early-exit from the declaration parser when you're actually given a rule (at most, you'll just consume the entire selector and then abort when you see the {, which is pretty short); and it works equally well whether there's any additional rules following the ambiguous rule or not.

I would be completely okay with this, it's just a slight expansion from what the WG had already accepted as a future design restriction.

(Notably, this is essentially how Sass avoids the issue - it just assumes that properties can't validly contain curly blocks, and bails on declaration parsing if it encounters one.)


I thought banning { from declarations whole-sale wasn't acceptable because people put json like things in their custom properties? But I might be mis-remembering.

Right, custom properties would still be able to include whatever. Like we explored in the Nesting discussion, custom props are not valid custom element names, so the chance of them showing up as an ambiguous rule is essentially nil.

Not sure how this ended up actually being specified, but one possibility we discussed was to simply fail style rule parsing for anything that starts with a dashed ident.

Yes, that's how it's specified, see https://drafts.csswg.org/css-syntax/#consume-a-qualified-rule. If you are trying to parse a rule and the first two significant tokens you see are a dashed ident followed by a colon, you immediately consume the remnants of a bad declaration and don't return anything. (But most of the time you won't even hit this; very few things can cause a custom property be fail declaration parsing so they'll almost never fall back to being parsed as a rule.)

@andruud
Copy link
Member Author

andruud commented Sep 8, 2023

But I think your #3 possibility is also just fine.

OK, in that case I think we should consider resolving on that.

Proposed: {} is invalid in standard property declarations unless it's the whole value

@mirisuzanne mirisuzanne added this to Thursday Morning in TPAC 2023 agenda Sep 8, 2023
@LeaVerou
Copy link
Member

I see this is slotted for one of the first items on Thursday morning. In case I miss it, please record that I'm in favor of the proposed resolution above.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-nesting-1] Relaxed nesting and var(), and agreed to the following:

  • RESOLVED: if we ever allow {} in properties, it must be the whole value
  • RESOLVED: And a non-conforming top-level {} invalidates non-custom properties even in the presence of a var()
The full IRC log of that discussion <drott> IRC nicks: Daniil Sakhapov's nick is sakhapov, Munira Tursunova is moonira
<emilio> andruud: When implementing relaxed parsing for nesting
<emilio> ... we hit this issue where if there's any var() function we ignore the grammar
<emilio> ... so if you're unlucky with your child selector
<emilio> ... and you can get O(n^2) behavior if you're very unlucky with the child selector name
<TabAtkins> q+
<astearns> ack chris
<emilio> ... there are different ways to fix it but I think the easiest one is disallowing {} in non-custom declarations
<emilio> ack dbaron
<emilio> dbaron: If we say that our future constraint is that if we have braces in standard property values it has to be the whole value, that seems fine except for shorthands
<emilio> ... so I wonder how likely we think that is
<emilio> TabAtkins: we have no plans ever recorded to use {} in a property except for embedding properties in shorthands
<emilio> dbaron: I'm ok with the constraint, I think in reality it might mean never use curly braces in values, but I think that's fine
<emilio> TabAtkins: at least on the top level, yeah
<emilio> TabAtkins: generally in support. The problem here is really uncommon
<emilio> ... I think when it occurs is very undesirable, and it's a low impact change to make it desirable and fast
<TabAtkins> proposed resolution: if we ever allow {} in properties, it must be the whole value
<emilio> s/desirable/reliable
<emilio> TabAtkins: a notable part of this is, this will be recognized at the parsing level
<emilio> fremy: the whole value means top level curly brackets right?
<emilio> TabAtkins: yes
<emilio> RESOLVED: if we ever allow {} in properties, it must be the whole value
<emilio> emilio: can we get a more specific resolution? The current one doesn't make it invalid w/ var()
<emilio> TabAtkins: happy to do that
<TabAtkins> proposed resolution: And a non-conforming top-level {} invalidates the property even in the presence of a var()
<emilio> emilio: does this apply to custom properties?
<emilio> TabAtkins: no
<emilio> emilio: but the same problem does apply right?
<emilio> TabAtkins: yeah but it's harder to screw up there and people do put braces in those
<vmpstr> emilio: is it weird that a parsing rule applies to some properties but not others
<vmpstr> TabAtkins: it exists already
<vmpstr> emilio: oh so you can't put a child ...
<TabAtkins> to trigger this for a custom prop, you need to write a rule that looks like `--foo:hover {...}`
<vmpstr> TabAtkins: if you try to write the rule ^ it is already impossible to parse as a rule
<vmpstr> TabAtkins: you have to respell it as --foo
<emilio> emilio: Ok
<TabAtkins> s/--foo/:is(--foo)
<vmpstr> emilio: we need to exclude some properties from that resolution?
<emilio> TabAtkins: yes
<emilio> proposed: And a non-conforming top-level {} invalidates non-custom properties even in the presence of a var()
<emilio> miriam: objections?
<emilio> RESOLVED: And a non-conforming top-level {} invalidates non-custom properties even in the presence of a var()

@devongovett
Copy link
Contributor

Am I understanding correctly that

div {
  button:focus {
    color: var(--x);
  }
}

would be parsed as a rule but

div {
  --button:focus {
    color: var(--x);
  }
}

would be parsed as a declaration?

That second one is parseable as a rule outside of nesting today. Maybe not a problem if element names can't start with --. Looks like custom selectors also require a leading :.

@tabatkins
Copy link
Member

That is correct. And yes, today --button:focus {...} will parse as a rule at the top level, but we ended up having to disallow it in nested context and there's not much reason to allow it at top-level either; better to have a consistent rule ("spell it :is(--foo)") for a rare case (/non-existent, since no valid HTML element can be named starting with a --) rather than split it into two different cases.

aarongable pushed a commit to chromium/chromium that referenced this issue Sep 28, 2023
To resolve some unfortunate ambiguity related to relaxed nesting
syntax, the CSSWG has decided to disallow {} from the value of
any standard property declaration unless it's the whole value [1].
Crucially, this applies even when var() is present in the value.

This CL detects these invalid braces in CSSVariableParser
by checking whether or not the top-level {} (if present) is the
only top-level component value [2].

[1] w3c/csswg-drafts#9317
[2] https://drafts.csswg.org/css-syntax/#component-value

Bug: 1427259
Change-Id: I00207a09416a9181d984ae65addd08983f5fd5c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4843631
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1202475}
aarongable pushed a commit to chromium/chromium that referenced this issue Sep 29, 2023
Since braces can no longer appear in standard property values
(except as the whole value), we can early-out of tokenization
if unexpected braces are encountered.

There are two optimization in this CL:

 1. ConsumeValue is now split into "ConsumeRestrictedPropertyValue"
    and "ConsumeUnrestrictedPropertyValue", with "restrictions"
    referring to how '{}' may and may not be used. Standard properties
    use the restricted version, and custom properties (and
    descriptors) use the unrestricted version (same as before).
    This fixes the O(N^2) problem described in [1], because we no
    longer tokenize past the '{' after (what was intended as) the
    style rule prelude.
 2. Then we have the case where '{ ... }' is the whole value.
    In this case, we can't just stop at the first '{', but we instead
    have to stop after the first block (because nothing is supposed
    to follow that block). This prevents unnecessarily tokenizing
    stuff after the {} if we're anyway going to restart.

The included performance tests:

 - NestingIdentKnownProperty.html: 242ms without this CL,
                                   5ms with this CL.
 - NestingIdentLeadingBraces.html: 118ms without this CL,
                                   61ms with this CL.

I also ran the style perf test on a version of this CL which
enables it unconditionally (i.e. drops the runtime flag).
The result is a little strange. Some minor slowdown is to be
expected. I have no good explanation for the result on the Extension
subtest, however.

Parse (µs)             Before     After    Perf      95% CI (BCa)
=================== ========= ========= ======= =================
ECommerce                 928       933   -0.6%  [ -1.9%,  +0.0%]
Encyclopedia             5099      5103   -0.1%  [ -1.2%,  +0.8%]
Extension                1098       987  +11.3%  [ +9.8%, +13.3%]
News                     5521      5416   +1.9%  [ -0.5%, +10.8%]
Search                   3268      3285   -0.5%  [ -0.9%,  -0.2%]
Social1                  9187      9234   -0.5%  [ -1.4%,  +0.8%]
Social2                   445       448   -0.7%  [ -1.7%,  +0.0%]
Sports                  34127     34558   -1.2%  [ -2.6%,  -0.6%]
Video                   21935     22252   -1.4%  [ -3.1%,  -0.6%]
Geometric mean                            +0.8%  [ +0.3%,  +1.8%]

Bug: 1427259

[1] w3c/csswg-drafts#9317

Change-Id: I2914e8415cc845615b17ffe12df8517cd83bd25d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4897802
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1203160}
@matthew-dean
Copy link

matthew-dean commented Oct 7, 2023

Oh no...

The idea from #7961 is that when standing at width :, we'll quickly back out of declaration parsing, because the next token (is() does not match width's property grammar. Restarting and trying again as a nested style rule is cheap, because we won't proceed too far into the tokens before discovering that it's not a valid declaration.

Does this imply that now CSS pre-processors will now have to recognize / understand every property's grammar / micro-syntax in order to understand how to parse if something is a declaration or not? This would potentially break Less and Sass if that's the case. The reason being that both Less and Sass have interpolation of property names, so the name of the property (or start of a qualified rule) is not known.

This sort of lookahead parsing seems like a radical departure of the CSS parsing definition... which may be okay but as I'm actively re-writing a pre-processor, are these new parsing rules cemented / defined?

@matthew-dean

This comment was marked as off-topic.

@rthrejheytjyrtj545
Copy link

@matthew-dean This is more convenient for authors; pay attention to the design principles of the platform. If author tools want to use their specific fantasy syntax, CSS defines a way to make it as compatible as possible.

tabatkins added a commit that referenced this issue Oct 9, 2023
@andruud
Copy link
Member Author

andruud commented Oct 19, 2023

Fixed by 77e9601.

@andruud andruud closed this as completed Oct 19, 2023
@tabatkins
Copy link
Member

Does this imply that now CSS pre-processors will now have to recognize / understand every property's grammar / micro-syntax in order to understand how to parse if something is a declaration or not?

No. In fact, the latest changes to handling {} in property values means the decl-vs-rule distinction is entirely clear from the generic syntax. Knowledge of properties and their grammars just lets you parse more efficiently (you can early-exit faster in most cases), but it won't change how you parse. (This previously wasn't true in some corner cases.)

@matthew-dean
Copy link

@tabatkins Thanks for the clarification! The ambiguity in early specs around "blocks" (including curly blocks) being a possible legal value for (non-custom) properties has long been a concern of mine. I'm very happy this is getting more restricted in the spec!

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 1, 2024
… about blocks and custom / non-custom properties. r=firefox-style-system-reviewers,zrhoffman

From w3c/csswg-drafts#9317

Do some gymnastics to avoid rewinding unnecessarily, since this is
super-hot code.

Differential Revision: https://phabricator.services.mozilla.com/D207797
github-actions bot pushed a commit to servo/stylo that referenced this issue May 2, 2024
… about blocks and custom / non-custom properties. r=firefox-style-system-reviewers,zrhoffman

From w3c/csswg-drafts#9317

Do some gymnastics to avoid rewinding unnecessarily, since this is
super-hot code.

Differential Revision: https://phabricator.services.mozilla.com/D207797
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue May 2, 2024
… about blocks and custom / non-custom properties. r=firefox-style-system-reviewers,zrhoffman

From w3c/csswg-drafts#9317

Do some gymnastics to avoid rewinding unnecessarily, since this is
super-hot code.

Differential Revision: https://phabricator.services.mozilla.com/D207797
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jun 1, 2024
… about blocks and custom / non-custom properties. r=firefox-style-system-reviewers,zrhoffman

From w3c/csswg-drafts#9317

Do some gymnastics to avoid rewinding unnecessarily, since this is
super-hot code.

Differential Revision: https://phabricator.services.mozilla.com/D207797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-nesting-1 Current Work
Projects
TPAC 2023 agenda
Thursday Morning
Development

No branches or pull requests

10 participants