[cssom][css-nesting] Error behavior for insertRule with declaration blocks.

[andruud]

In #10234 we resolved to give insertRule the ability to parse and insert declaration blocks (as CSSNestedDeclarations rules).

As of recent spec edits, insertRule is now set up to parse a declaration block (after the regular rule parsing), and throw an error when that declaration block is empty. https://drafts.csswg.org/cssom/#insert-a-css-rule, Step 4.

Is this behavior indeed what we want? @emilio points out that it's possibly weird that potentially-unknown-property:foo throws, but potentially-unknown-property: foo; color:red doesn't.

On the other hand, the following would lead to no CSSNestedDeclarations rule when parsed normally:

.a {
  .b { ... }
  potentially-unknown-property: foo;
}

Whereas this would:

.a {
  .b { ... }
  potentially-unknown-property: foo;
  color:red;
}

So probably the specified behavior isn't too bad, but I'd like to discuss to either get a stamp for this change, or to see if we can do better somehow.

[fantasai]

I think if we are not throwing on invalid declarations (at all), then we should not throw on empty. But if we are throwing on invalid declarations, then we should also throw on empty.

[andruud]

Throwing on invalid declarations sounds bad for forwards compat, we probably shouldn't try to do that.

I think if we are not throwing on invalid declarations (at all), then we should not throw on empty.

All right, but we still need to define the behavior when the when the parsed declarations is an empty list. I assume we'll just ignore it, i.e. not create/insert a CSSNestedDeclarations rule?

[tabatkins]

Yeah, while I don't like the OM's lack of errors, we should be consistent here - no error thrown, just nothing happens.

All right, but we still need to define the behavior when the when the parsed declarations is an empty list. I assume we'll just ignore it, i.e. not create/insert a CSSNestedDeclarations rule?

Yeah, consistent behavior between "no decls given at all" and "only invalid decls given" - both result in no decls, so shouldn't create a rule.

[mdubet]

Throwing on invalid declarations is clearly a giant forward compatibility can of worms 😬
Not having a feedback for the CSSOM user that this function call hasn't done anything is slightly weird, but better than the other choices. If the user really want to know if something happened, checking the length of cssRules before and after would give the answer.

[emilio]

Yeah, while I don't like the OM's lack of errors, we should be consistent here - no error thrown, just nothing happens.

But insertRule throws on invalid rules right now tho, right?

[mdubet]

Yeah, while I don't like the OM's lack of errors, we should be consistent here - no error thrown, just nothing happens.

But insertRule throws on invalid rules right now tho, right?

Yes https://drafts.csswg.org/cssom-1/#insert-a-css-rule

[tabatkins]

Right, ok, so since we already throw on invalid rules, we should throw here; I just want consistency. (Sorry, didn't go track down the previous version of the algo to double check.)

In that case an empty list should throw, yes, whether it comes from an empty string or "a list of invalid decls"

[css-meeting-bot]

The CSS Working Group just discussed [cssom][css-nesting] Error behavior for insertRule with declaration blocks., and agreed to the following:

• RESOLUTION: throw on insertRule only when there are no valid declarations. Do not throw otherwise
• RESOLVED: throw on insertRule only when there are no valid declarations. Do not throw otherwise

The full IRC log of that discussion

<noamr> emilio: we resolved to be able to call insertRule for various declarations, but we didn't resolve on error handling
<noamr> emilio: 2 options: (1) do nothing, effectively making this API not throw at all, right now we throw. (2) throw, but only when we get empty declaration blocks
<noamr> emilio: 2 is a bit weird if you only specify invalid declarations, but would be consistent with how it's parsing. Seems like there's a variety of opinions on the thread or other options
<noamr> astearns: other people who have commented
<noamr> Mattieud: we thought that maybe we'd add another function, insertDeclaration, to avoid this issue.
<emilio> q+
<noamr> MattieuD: This was one of the options we resolved on last time, still option for the discussion.
<astearns> ack emilio
<noamr> emilio: I thought we wanted insertRule work and would do the right thing
<noamr> TabAtkins: the resolution was to prefer insertRule, but fall back if necessary due to web compat
<noamr> emilio: were you against throwing on declaration blocks?
<noamr> TabAtkins: I want to be consistent. Need to look at existing implementation. If we throw now we should keep throwing
<noamr> emilio: insertRule current throws
<noamr> TabAtkins: then insertRule should throw if the declaration is consisting only of invalid rules
<noamr> emilio: that's the current spec
<noamr> matthieud: The spec says to throw syntax errors in multiple places
<noamr> astearns: matthieud are you ok with matching the throwing behavior here, or prefer a separate method?
<noamr> matthieud: current is OK, it's a bit weird and surprising when mixing valid/invalid declarations, but that's fine
<astearns> ack fantasai
<noamr> fantasai: throwing on "only invalid rules" vs throwing on "mixed" is weird and inconsistent. we should throw on "some invalid stuff"
<emilio> q+
<astearns> ack emilio
<noamr> TabAtkins: we can't throw on "some invalid" because of forward compatibility, as people won't be able to use it for multiple browsers in the future as it would always throw
<noamr> fantasai: it would be weird if something was throwing for not having any valid rules and then suddenly not throw when one valid rule added
<TabAtkins> we currently throw on "one invalid at-rule", I'm not sure that throwing on "one invalid decl" is inconsistent
<noamr> emilio: that's exactly the CSS parser behavior
<florian> q+
<matthieud> matthieud: the CSS parser and the CSSOM have different behavior "on purpose" : the CSS parser happily skip invalid rules/at-rules/declarations - but the CSSOM throws on invalid rule
<noamr> emilio: It's not that bad, because that's how the CSS parser behavior works
<noamr> emilio: I don't recall the behavior when there are multiple rules
<fantasai> s/any valid rules/any valid declarations/
<fantasai> s/one valid rule/one valid declaration/
<noamr> Matthieud: you can't insert multiple rules with insertRule
<astearns> ack florian
<noamr> florian: from my POV if we throw on everything that's invalid or empty and don't throw on mix valid/invalid, than that's consistent
<noamr> florian: you need at least 1 valid declaration to be valid
<emilio> q+
<noamr> fantasai: if we care about the idea of forward compat, if we use 3 new syntax and one old syntax, and then you remove the old one it starts throwing, it doesn't make sense
<noamr> fantasai: if we want to throw invalid declarations we should always do that
<noamr> florian: it works the same as with inserting an empty string
<astearns> ack emilio
<noamr> emilio: I don't disagree with fantasai but I don't think it's possible because it would make the API never throw with any input
<TabAtkins> `insertRule("@invalid {...}")` throws - this is perhaps a bad idea for forwards compat *as well*, but it's the current long-standing behavior
<noamr> emilio: that's a pretty significant breaking change
<noamr> emilio: the current spec is perhaps the most sensible solution
<astearns> ack fantasai
<noamr> florian: I argue that it makes conceptual sense, not necessarily that it's convenient for devs
<noamr> fantasai: I agree we shouldn't change the behavior for @ rules, that's fine since it's one rule at a time
<noamr> fantasai: I'm arguing that for invalid declarations only, or a mix of valid and invalid declarations
<emilio> q+
<matthieud> q+
<noamr> fantasai: as a consequence we need to decide whether we throw on all-invalid and empty string
<astearns> ack emilio
<fantasai> s/that for/for consistency on/
<noamr> fantasai: we need to either throw or not throw on all-invalid/non-invalid/empty-string
<noamr> fantasai: the 3 of them should behave in the same way
<fantasai> s/non-invalid/some-invalid/
<noamr> emilio: not-throwing on all invalid is not feasible, because you don't know if you were given invalid declarations or an invalid at rule. it gets weird
<florian> q+
<noamr> florian: right, it means changing the existing situation where we throw on invalid rule
<noamr> s
<noamr> s/florian/Matthieud
<fantasai> s/as a consequence we need to decide/and I think we should be consistent about/
<noamr> emilio: then we should other do what's on the spec, or throw on some invalid, but it's a bit odd how the rest of CSS works
<astearns> ack matthieud
<TabAtkins> At the moment there's no way, given the parsing algos, to tell the difference between "empty string" and "all invalid decls"; invalid decls are dropped during parsing.
<noamr> MatthieuD: this would be a breaking API change
<astearns> ack florian
<TabAtkins> If we try to just split on "empty string" vs "not empty", then a string containing just an invalid rule will pass (whereas currently it throws).
<noamr> florian: maybe I'm failing to understand something, but the way I see it, you want to throw as a reaction to anything invalid being passed, while I think we should throw on the absence of something valid
<TabAtkins> We'd had to instead do some heuristic on the stuff in the string to see if it *might* look like there's a decl in there.
<noamr> florian: throwing is not a signal of "something was wrong" but rather "there was not a single thing that was right"
<fantasai> Could check for initial "@" for "invalid at-rule". Probably that's what most people would rely on.
<noamr> florian: at least it's useful in some cases where you insert some valid rules and some invalid
<TabAtkins> Can't look for just an @; style rules are allowed too
<noamr> fantasai: it's ok conceptually, do we want to be resilient on wrong declarations, if we don't throw on most implementations and silently handle invalid declarations, and then some implementation arbitrarily succeeds
<fantasai> TabAtkins, ahh right
<noamr> florian: isn't that useful, when you want to get told when "one thing works" vs "nothing works"
<TabAtkins> Grammar is hard (this is why I wanted insertDeclarations(), fwiw)
<matthieud> I don't think trying to find an heuristic like looking for an @ is a good idea (and it already doesnt work for nested style rule)
<emilio> q+
<noamr> fantasai: why do I care if I dropped 2/3 vs 3/3?
<noamr> florian: it depends why you inserted more than one
<noamr> florian: maybe you added a prefixed and a non-prefixed, it depends on why you use the API
<noamr> florian: this works for fallback properties, where you want "at least one of these to work"
<noamr> florian: you called insertRule, if it succeeded you probably meant it because you have enough fallbacks, but if nothing goes through something went wrong
<noamr> fantasai: this might not be more or less wrong
<astearns> ack emilio
<noamr> emilio: it consistent with how the API behaves right now. If the rule list grows by 1 it doesn't throw, otherwise it throws
<noamr> emilio: it's weird because it's inserting something that's not a rule into a rule list
<noamr> astearns: a lot of comments on IRC, not entirely sure we're ready for a resolution. take back to the issue?
<noamr> fantasai: I'm not going to object, people will be confused, but I don't know if there is a better option
<noamr> fantasai: given the restrictions, I don't see a better solution
<noamr> astearns: concerns about "only throwing when there is nothing valid?"
<noamr> proposed resolution: throw for insertRule if there are no valid declarations, not throw otherwise
<noamr> RESOLUTION: throw on insertRule only when there are no valid declarations. Do not throw otherwise
<noamr> RESOLVED: throw on insertRule only when there are no valid declarations. Do not throw otherwise

[tabatkins]

Resolution is essentially "close no change", fwiw. So I'll go ahead and close. ^_^