w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.5k stars 660 forks source link

[css-nesting] Another problem with mixing declarations and rules - error recovery #8349

Open plinss opened 1 year ago

plinss commented 1 year ago

I'm not sure if this has been discussed before or not, but I just realized there's another issue with both option 3 and the lookahead approaches to nesting. Error recovery.

There are different behaviors for error recovery with declarations and rules, an invalid declaration consumes everything up until the next unenclosed ;, where invalid rules consume everything up until the next unenclosed ; or block {}.

Consider the following example:

div {
    background: green;
    .nested {
        color: blue;
    }
    background: red;

What color is the background of the div? In a browser that doesn't support nesting, it'll be green, in a browser that does, it'll be red. This is a problem.

Having a & prefix doesn't fix this. Lookahead doesn't fix this. AFAICT the only thing that does is a full at-rule prefix, e.g. @nest (even a bare @ doesn't fix it) because this causes the browser to shift back into rule recovery mode when it was expecting a delcaration.

I see a few paths forward here:

1) We forbid declarations after rules. This is still problematic as browsers that support nesting will ignore all declarations after the nested rules, while older browsers will only ignore the first one. 2) We require a ; after nested rules. I think this is fragile. 3) We just YOLO it and tell people "don't do that". If people really want to put declarations after nested rules they need to add a ; between the last rule and the first following declaration if they want compatibility with older browsers. 4) We require a full at-rule prefix. Probably the least popular, but the most safe option.

I'm open to other suggestions...

romainmenke commented 1 year ago

What color is the background of the div? In a browser that doesn't support nesting, it'll be green, in a browser that does, it'll be red. This is a problem.

I think this is only an issue if CSS authors expect nested CSS to have aspects of graceful degradation / progressive enhancement. But I don't think they do.

/* no one writes nested CSS with this mental model : */

.foo {
  /* base styles */

  &:hover {
    /* enhancement */
  }
}

CSS authors are aware that any use of nesting will result in a broken and unusable result in older browsers without native support.

@layer is no different than this. Only in very rare cases will a website render ok when large chunks of CSS are missing because they are wrapped in an at rule an older browser doesn't support.

Nested CSS might break down in weirder ways than some might expect but I don't think how it breaks is particularly important.

It is also why I spend so much of my time crafting tools to transpile features like these for older browsers. Authors who need to support older browsers and want to use nested CSS can use tools like these (as they do today).

plinss commented 1 year ago

I think this is only an issue if CSS authors expect nested CSS to have aspects of graceful degradation / progressive enhancement. But I don't think they do.

I disagree. CSS has had forward compatible parsing and graceful degradation since day one. It's a core feature of the language.

Of course authors should not expect nested rules to work in older browsers, but they will not (and should not) expect them to break other aspects of the stylesheet. Nothing should get ignored except the nested rules.

I do accept that due to the nature of nesting, most authors will either use a preprocessor to generate the stylesheets they server to all clients (which makes our entire nesting feature irrelevant), serve different stylesheets to newer clients than older ones (which is fragile, ask any browser vendor how much trouble UA sniffing has caused), or will simply use nesting and not care/test on older browsers. Some authors may not care about older browsers, but we need to.

We can decide to just live with this (see option 3 above), but if so, we need to make that decision with our eyes open and we also need to give the proper advice to authors on what to expect and what best practices to use in their stylesheets.

romainmenke commented 1 year ago

CSS has had forward compatible parsing and graceful degradation since day one. It's a core feature of the language.

Yes, but this can only go so far. Which is why I referenced @layer specifically.

@layer in older browsers is just ignored, but there is no benefit to that. The page will be broken anyway. There is no way to use @layer in a non-trivial way and have some form of compatibility with older browsers. For CSS authors and end users there is no difference between throwing out the entire stylesheet or only @layer. There will even be many cases where throwing out the entire stylesheet would have been better.

The same would be true for @nest. The bit of CSS that is invalid and thrown out would be slightly different than nested CSS without @nest but the end result is a broken page all the same.

but they will not (and should not) expect them to break other aspects of the stylesheet.

They should and do expect the rendered page to be fully broken. There is just no way around this. The vast majority of rules and declarations will be nested and thus thrown out as invalid. The way nested CSS is written today is just not compatible with older browsers.

Exactly how it breaks at a tokenizer/parser level is something we care about, but CSS authors and end users do not.

I am only talking about nesting here, for many other features forwards compat is absolutely critical

We can decide to just live with this (see option 3 above),

This seems fine to me.

Loirooriol commented 1 year ago

I fail to see how "@layer is no different than this":

div {background: green}
@layer {}
p {background: red}

On browsers that don't support @layer, both div {background: green} and p {background: red} work.

background: green;
.nested {}
background: red;

On browsers that don't support nesting, background: green; works but background: red; doesn't work.

romainmenke commented 1 year ago

absolutely true, at a tokenizer/parser level.

My comments are specifically about CSS author expectations.

The way nested CSS is written today doesn't look anything like these artificial examples. It is more like this :

.component {
  /* few or no declarations, "component" is mostly just a wrapper, "@scope" doesn't exist yet */
  color: cyan;

  .element-a {
    color: blue;

    &:hover {
      color: pink;
    }

    @media screen {
      color: green; 
    }

    /* maybe some more deeply nested elements ... */
  }

  /* more elements ... */

  /* no declarations at all */
}

All the important bits are thrown out. That a single declaration would also be throw out or not is not relevant to CSS authors because of this.


No one is using @layer like this :

div {background: green}
@layer {}
p {background: red}

Much more common to see this :

@import url('bootstrap.css') layer(bootstrap);

/* overrides on top of bootstrap here */

No one expects their overrides on top of bootstrap to result in a working page when the import is ignored. The entire foundation is gone.

In this aspect nesting and @layer are similar. Using the feature changes the way you write CSS and the resulting stylesheets will not work in older browsers.

tabatkins commented 1 year ago

They should and do expect the rendered page to be fully broken. There is just no way around this. The vast majority of rules and declarations will be nested and thus thrown out as invalid. The way nested CSS is written today is just not compatible with older browsers.

Yes, Romain is making the right point. While an @layer rule in an old browser won't mess up the following rule, it will absolutely make your stylesheet useless since it'll cause the entire contents of the @layer to get thrown away. That's a huge problem! There's simply no way to get around it, tho, without at best doing some really awkward workarounds in the design that would significantly hamper the feature going forward.

Nesting is in exactly the same boat. Your nested rules will simply get thrown away in older browsers. There's no way to work around this; you have to either buy in to Nesting (accepting that your stylesheet will be broken in older browsers) or avoid it entirely. And if you're buying in, the error-recovery change doesn't matter; it'll work correctly in the browsers that your stylesheet works in, and it'll be extremely broken in older browsers no matter what.

This is just how adding newer syntax features works. It's happened in the past, and it'll happen in the future; Nesting is perfectly normal in this regard. It's awkward during the transition period but stops being a problem as usage numbers advance. We certainly prefer designing features that can gracefully fallback instead, but if sometimes isn't an option.

All that said, this particular issue only occurs if you put properties after rules, which is a bad practice anyway. Put all your properties at the top, before any rules, and you'll be fine. Putting them later gives you absolutely nothing except possible confusion.

Loirooriol commented 1 year ago

Of course new functionality won't work on old browsers. This issue is about the old functionality that follows it.

While an @layer rule in an old browser won't mess up the following rule, it will absolutely make your stylesheet useless

That's not true in general. The @layer may just contain one rule with some minor styles that aren't really important to render the page well. While ignoring the entire stylesheet could have a heavy negative impact. It depends on the case.

the error-recovery change doesn't matter; it'll work correctly in the browsers that your stylesheet works in, and it'll be extremely broken in older browsers no matter what

Again, not necessarily, e.g. here I would be fine with losing the cosmetic shadow on hover, losing display: grid may have a much worse impact:

.grid {
  &:hover { box-shadow: 0 0 5px }
  display: grid; /* won't work on old browsers */
  grid-template-columns: repeat(auto-fill, 100px); /* won't work without display: grid */
}

It's happened in the past, and it'll happen in the future

I can't recall a single new feature that had this effect. It's not the case for property values, property names, selectors nor at-rules. When these things are not recognized, they don't affect the next declaration or rule. The closest thing that comes to mind is selector lists, where a single invalid branch invalidates the others, but it still only affects that rule, and this behavior has caused lots of frustration.

occurs if you put properties after rules, which is a bad practice anyway

1st time I have seen this described as bad practice. https://drafts.csswg.org/css-nesting/#mixing just says "all three can be arbitrarily mixed". If it's bad practice then the spec should warn authors.

plinss commented 1 year ago

Yes, Romain is making the right point. While an @layer rule in an old browser won't mess up the following rule, it will absolutely make your stylesheet useless since it'll cause the entire contents of the @layer to get thrown away. That's a huge problem! There's simply no way to get around it, tho, without at best doing some really awkward workarounds in the design that would significantly hamper the feature going forward.

I'm not advocating for some workaround that will make nested rules work in an older browser, and of course I'm aware that a stylesheet that's naively written for a browser that supports nesting isn't going to give a good result in an older browser. That's not my point.

Nesting is in exactly the same boat. Your nested rules will simply get thrown away in older browsers. There's no way to work around this; you have to either buy in to Nesting (accepting that your stylesheet will be broken in older browsers) or avoid it entirely. And if you're buying in, the error-recovery change doesn't matter; it'll work correctly in the browsers that your stylesheet works in, and it'll be extremely broken in older browsers no matter what.

My point is that while stylesheets will break in older browsers, they should break in a predicable way so that anyone who wants to provide fallback behavior for older browsers can. This is a fundamental aspect of CSS as you're well aware. Saying 'it's partly broken, so who cares how broken" is not generally the right answer.

The behavior of swallowing the first declaration after a nested rule will be surprising to most authors.

This is just how adding newer syntax features works. It's happened in the past, and it'll happen in the future; Nesting is perfectly normal in this regard. It's awkward during the transition period but stops being a problem as usage numbers advance. We certainly prefer designing features that can gracefully fallback instead, but if sometimes isn't an option.

The transition period is important. If we don't provide a functional mechanism to support both older and newer browsers, it will hurt the uptake of the feature until the vast majority of browsers support it.

Of course due to the nature of nesting an 'all-or-nothing' approach probably makes the most sense for most authors. To this end, we need to make sure that nesting is feature detectable so that authors can include both smaller stylesheets that use nesting, and larger ones that don't for older clients. (I expect preprocessors to generate both from a single source.)

That doesn't mean that we should ignore fallback behavior, CSS isn't versioned.

All that said, this particular issue only occurs if you put properties after rules, which is a bad practice anyway. Put all your properties at the top, before any rules, and you'll be fine. Putting them later gives you absolutely nothing except possible confusion.

Agreed, but if this is the approach that we take (as listed as option 3 above), at a minimum we need to document putting properties first as best practice and explain to author what will happen if they don't. We just spent a large part of a meeting discussing how mixed rules and properties work, and no one said "don't do that".

We also need to take this aspect into consideration when we consider alternatives.

tabatkins commented 1 year ago

That's not true in general. The @layer may just contain one rule with some minor styles that aren't really important to render the page well. While ignoring the entire stylesheet could have a heavy negative impact. It depends on the case.

Yes, one can imagine a scenario in which @layer is used, for some reason, purely for a small cosmetic effect. That is not a realistic usage scenario, however, so I didn't address it. We generally expect people to use @layer by putting the entire stylesheet into layers, or at least most of it.

I can't recall a single new feature that had this effect. It's not the case for property values, property names, selectors nor at-rules. When these things are not recognized, they don't affect the next declaration or rule. The closest thing that comes to mind is selector lists, where a single invalid branch invalidates the others, but it still only affects that rule, and this behavior has caused lots of frustration.

I didn't say we'd added new parsing constructs that invalidates the next property, I said we've added features that are all-or-nothing - either you only care about browsers that support them, or you don't use the feature at all, because it's used in a wide-ranging and integral way that isn't realistically fall-back-able.

1st time I have seen this described as bad practice. drafts.csswg.org/css-nesting/#mixing just says "all three can be arbitrarily mixed". If it's bad practice then the spec should warn authors.

An older version of the spec did more explicitly warn against it. Having a warning put back in is probably appropriate, just for reasons of readability. (Added now.)

My point is that while stylesheets will break in older browsers, they should break in a predicable way so that anyone who wants to provide fallback behavior for older browsers can

In the (likely exceedingly rare) cases someone does want to write a stylesheet using nesting while providing some sort of content for those without, the rule is: put your properties first.

We just spent a large part of a meeting discussing how mixed rules and properties work, and no one said "don't do that".

Right, we've said if often enough that it didn't particularly bear repeating, plus "don't do it" isn't a valid response to "what should the behavior be when someone does it".

plinss commented 1 year ago

Another viable approach is to put a ; after every (or at least the last) nested rule if you want to mix properties and rules.

And to be clear, I'm not raising this as a showstopper, I'm just saying that it needs to be considered, and the WG as a whole needs to decide to just live with it or not. If we do decide to live with it (which I'm ok with, but not thrilled about), we do need to make authors aware of the issue and provide guidance. That's why I offered that as an option in the first place. If we wind up in a place where we reconsider the @nest approach, this should be taken into account then as at-rules do have proper error recovery in older browsers, so it makes the behavior simpler and more flexible for authors.

tabatkins commented 1 year ago

Another viable approach is to put a ; after every (or at least the last) nested rule if you want to mix properties and rules.

Yup, that works, it's just not necessary if you do the more readable thing in the first place. ^_^ I'd prefer not mentioning it in the spec, since there's a more preferable option in the first place.

plinss commented 1 year ago

It might be worth mentioning anyway. We need to be careful with the lookahead algorithm, if it's possible for an invalid rule to be considered as a declaration then it could trigger declaration error recovery, which will also swallow all the following nested rules. Putting a ; after every nested rule prevents that too.

tabatkins commented 1 year ago

if it's possible for an invalid rule to be considered as a declaration then it could trigger declaration error recovery, which will also swallow all the following nested rules.

Right, that's part of why I prefer the current spec's behavior wrt #8251 right now; by tilting toward parsing anything unknown as a rule, you get safer error recovery.

The proposed infinite lookahead algo (https://github.com/w3c/csswg-drafts/issues/7961#issuecomment-1396112293) also leans in this direction. Of the three ways to trigger property parsing, the first ("starts with a dashed-ident") is "unsafe" but not a realistic confused-for-selector scenario; the second ("starts with prop:...") is safe bc it uses lookahead to know if it's gonna need to parse as a rule or property; the third ("starts with prop: ... (note the space)") is "unsafe" but again can only show up if you really screw up writing your selector.

plinss commented 1 year ago

I agree that we should bias towards rule recovery, where we'll get into trouble is if we ever add something like:

new-property:something { something } color: red;

and the part of the declaration after the {} could potentially be interpreted as a valid property and will start taking effect during error recovery. We can choose to never do that, but we need to document that and the why of it for our future selves and whoever comes after.

Where we need to be really careful are the cases where we use lookahead to tell if something is a rule or property, and guess wrong because it's invalid either way. Falling into declaration error recovery by mistake could be really bad.

Right, we've said if often enough that it didn't particularly bear repeating, plus "don't do it" isn't a valid response to "what should the behavior be when someone does it".

Agreed, but one of the options here is to make properties after rules invalid, turning 'best practice' into 'mandatory practice', which would make all the spec'd behavior of mixed rules and declarations moot. (I'm not in favor of this approach, fwiw, because older browsers will only skip the first declaration after a rule, not all of them, so it's still inconsistent behavior.)

tabatkins commented 1 year ago

where we'll get into trouble is if we ever add something like:

Yup, that exact pattern is documented in my lookahead parsing comment as a design restriction we'll have to abide by - non-custom properties will still be able to start with a {}-block (allowing ideas like explicit shorthands or whatever), but wouldn't be able to contain a top-level {}-block after the start.

Falling into declaration error recovery by mistake could be really bad.

Agreed, thus my analysis in my previous comment. So long as CSS abides by the design restriction, one of the possible "parse like a declaration" results is safe, and the other two, while still "unsafe" (that is, they'll eat following rules at the same level, until they see a ;), require typo'ing your stylesheet.

plinss commented 1 year ago

Yup, that exact pattern is documented in https://github.com/w3c/csswg-drafts/issues/7961#issuecomment-1396136134 we'll have to abide by - non-custom properties will still be able to start with a {}-block (allowing ideas like explicit shorthands or whatever), but wouldn't be able to contain a top-level {}-block after the start.

Note that the problem I was trying to highlight is actually the reverse. If a new, non-custom property has a {} and then something after the block, the part after the block won't get skipped if the property gets treated as an invalid rule. Theoretically, the end bit could wind up looking like a valid property on its own, and error recovery would wind up injecting a new property that it shouldn't. So this is a risk of using rule recovery where we should use declaration recovery... Having something before the block isn't the risk here, it's allowing things after the block. (Things before the block may pose another risk, like confusing the lookahead algo.)

tabatkins commented 1 year ago

Theoretically, the end bit could wind up looking like a valid property on its own,

Yeah, in theory that's possible, if exceedingly unlikely; we haven't put colons into any property yet, and that's the minimum that would be needed. We could potentially pre-commit ourselves to not do that (only use {}-blocks at the top level of a property as the whole value), but I think in practice that'll be the case anyway.

tabatkins commented 1 year ago

Just to quickly lay out the error-recovery things again, to make it clear without having to dive thru an issue:

  1. In older browsers that don't understand Nesting, they'll obviously drop any nested rules on the floor. If you have declarations after the rule, the first declaration following the rule will also be dropped.
  2. In newer browsers, error-recovery is just fine so long as we (the CSSWG) stick to two design restrictions: no top-level semicolons in rule preludes, and if we put top-level {} blocks in decl values, we only do so at the end of the value (so the semicolon is the next thing the parser will see). These two restrictions ensure that if you try to write a valid declaration or rule, but fail (either make a mistake, or write something that the user's browser doesn't support yet), the parser will stop at the same point as if you had written it validly.
  3. We can't stop custom properties from violating the design restriction from above, so when something looks vaguely like a custom property, it's guaranteed to be parsed as one, and never as a rule.

For (1), putting decls after your rules is already not a great practice for readability; if authors put all their decls first (like the spec recommends) then there's no additional problem. (And in any case, all the rules being dropped is already gonna result in a broken stylesheet, so further breakage is fairly insignificant.)

For (2), these restrictions were more or less already guaranteed; neither are syntax space we've ever even thought about exploring.

For (3), this is only theoretically a problem for non-HTML host languages, where it might be possible to have a dashed-ident as an element name. In HTML, tho, this is illegal - custom element names are required to start with an ASCII alpha character in the first place. So --foo:hover ... will never be a reasonable selector, and there's no chance of it being confused with a --foo custom property. (I'm not totally sure if this is even legal in general XML.)