[css-properties-values-api] Rule validity doesn't match implementations #1098

emilio · 2023-06-26T17:44:12Z

Implementations and tests blatantly disagree with the spec even in simple cases.

https://drafts.css-houdini.org/css-properties-values-api-1/#the-syntax-descriptor says:

The syntax descriptor is required for the @property rule to be valid; if it’s missing, the @property rule is invalid.

https://drafts.css-houdini.org/css-properties-values-api-1/#inherits-descriptor:

The inherits descriptor is required for the @property rule to be valid; if it’s missing, the @property rule is invalid.

https://drafts.css-houdini.org/css-properties-values-api-1/#initial-value-descriptor:

If the value of the syntax descriptor is the universal syntax definition, then the initial-value descriptor is optional. If omitted, the initial value of the property is the guaranteed-invalid value.

Otherwise, if the value of the syntax descriptor is not the universal syntax definition, the following conditions must be met for the @property rule to be valid:

The initial-value descriptor must be present.

The initial-value descriptor’s value must parse successfully according to the grammar specified by the syntax definition.

The initial-value must be computationally independent.

If the above conditions are not met, the @property rule is invalid.

Yet both Blink (and WebKit probably by copying whatever Blink implemented) disagree with it in trivial cases. E.g. this logs 0 per spec, but 1 in WebKit and Blink.

This is annoying because this affects a lot of interop 2023 tests. We have a implementation of this and as I was reviewing this to potentially enable it I found that all the tests are just broken.

Am I missing something? "rule is invalid" only has one meaning in my book, which is that the whole rule should be dropped, just like an @font-face and the

Unknown descriptors are invalid and ignored, but do not invalidate the @property rule.

Makes me think that the spec agrees with my interpretation on what "validity" means here.

I think this is ultimately a spec bug, as in w3c/csswg-drafts#1682 it was decided to be consistent for @counter-style and @font-face.

cc @tabatkins @lilles @andruud

The text was updated successfully, but these errors were encountered:

andruud · 2023-06-26T18:34:05Z

This was briefly discussed at some point. If I remember correctly, the behavior we intended was that "invalid" in most cases here means "does not lead to a property registration". But I can not find the discussion now. :(

cdoublev · 2023-08-26T03:27:03Z

Also noticed by @romainmenke in w3c/csswg-drafts#9175 because an "invalid" @property still makes a following @import invalid.

Basically, it seems that people do not want a rule to be removed because its block contents is "invalid", right?

tabatkins · 2023-08-28T18:58:53Z

I don't recall having that discussion either (but that doesn't mean it didn't occur - my memory is notoriously faulty). Happy to either fix the spec to match impls or leave it and let impls get fixed, tho. I don't think I care too much how it goes, but making the actual rule invalid in simple grammatical failures like this is the more common behavior, and is what is generally meant by "X is invalid" across our specs.

Per blame, this text does come directly from @andruud (a5cd596) and was early in their spec-writing career, so it was likely just some loose wording.

andruud · 2023-09-13T11:40:38Z

was early in their spec-writing career

Yeah, maybe I was just confused at the time.

"rule is invalid" only has one meaning in my book, which is that the whole rule should be dropped

OK, then let's change WPTs to reflect that and I'll adjust our implementation.

fred-wang · 2023-10-10T08:00:28Z

Hi people, I uploaded a patch to align Firefox's behavior with the spec and the WPTs are adjusted accordingly there. I also opened web-platform-tests/interop#575

It's possible that we also want to follow what @emilio mentioned and align the spec with @counter-style and @font-face, but I'm not sure what was the conclusion here.

fred-wang · 2023-10-10T09:43:44Z

One thing I forgot to mention is about the interpretation of invalid inherits values. It seems they should be dropped, so the descriptor would be missing and the full @property rule becomes invalid (similar to what the spec describes for the syntax descriptor). But the spec is not explicit about that and existing tests assume it would take value false.

cdoublev · 2023-10-11T10:03:42Z

align the spec with @counter-style and @font-face, but I'm not sure what was the conclusion here

It does not seem usefull to preserve the rule in the CSSOM if it is invalid and cannot become later valid, since its attributes are read-only, as I noted it in w3c/csswg-drafts#9417.

But if later people ask for updating these attributes via CSSOM, I think there would be a compat issue.

But the spec is not explicit about that and existing tests assume it would take value false.

And people seem to want true as the initial value (cf. #994), which would have to be defined in the descriptor definition table. Currently, it is n/a (see prose).

I think descriptors declarations with invalid value must be ignored would be better defined in CSS Syntax, and any explicit mention of this parsing rule in other specs should be removed.

…dropped. Currently Firefox properly performs validation of an @Property rule, as defined in [1]. However when such a rule is invalid, it only does not register the custom property instead of dropping the whole rule. Other implementations also follow that aproach and existing web platform tests disagree with the specification [2]. This patch aligns Firefox's behavior with the specification, by moving @Property validation during parsing and dropping invalid rules. Tests are updated as follows: 1. /css/css-properties-values-api/at-property-cssom.html Existing tests that don't have the three descriptors (syntax, inherit, initial-value) are invalid and now the test verifies no corresponding rules are exposed via CSSOM. `--no-initial-value` is renamed `--no-initial-color-value` and its legacy tests are kept for a new @Property `--no-initial-universal-value` which uses the universal syntax (so initial value is optional). Some dummy descriptors are added for --tab\ttab so that it remains valid. Similarly, we ensure --valid-whitespace's syntax (space-separated) and initial-value (comma-separated) agree. 2. /css/css-properties-values-api/at-property.html Existing `test_descriptor()` tests are trying an @Property with a single specified descriptor and so are always invalid. To work around that, we tweak `test_descriptor()` so that it can build a valid descriptor instead. The `syntax` and `inherits` fallback to universal and true respectively while the `initial-value` descriptor is built from the `syntax`. An extra parameters is introduced in case the caller wants to provide these values directly. Finally, when the expected value is null the function instead verifies that the rule is really dropped. 2.1. Some existing syntax tests are registering rules with unquoted syntax value 'red', 'rgb(255, 0, 0)', 'color', 'foo | bar' and expect to obtain a rule with an empty syntax string, suggesting some kind of invalidity handling (cf similar tests). We interpret the first two as "specifying a color value", quotes are added and the first one actually becomes a valid custom-ident. The last two already have a similar quoted version, so we just interpret them as "missing quotes". 2.2. Given the previous 'red' custom-ident, we add tests for invalid custom-ident as defined in [3]. 2.3. Some existing `syntax` tests are checking that we must have "pipe between components" and no "leading bar" and are again expecting a rule with an empty syntax string. We fix the apparent mistake of missing quotes and provide initial values that could potentially be interpreted as correct by implementations accepting these invalid syntaxes. 2.4. One `initial-value` test is checking "var(--x)" but that is not computationally independent so tweak the test to check that makes the @Property rule invalid. Also add a similar '3em' test mentioned in the spec. 2.5. Some `inherits` tests verify that invalid rules are interpreted as false. It seems they should instead be treated as if it does not exist and so should make the @Property rule invalid. [1] https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule [2] w3c/css-houdini-drafts#1098 [3] https://drafts.csswg.org/css-values-4/#custom-idents Differential Revision: https://phabricator.services.mozilla.com/D190444 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1857724 gecko-commit: cfd3a0958f03cb28be6b62a57eb5873358189e1e gecko-reviewers: emilio

…ons should be dropped. r=emilio Currently Firefox properly performs validation of an @Property rule, as defined in [1]. However when such a rule is invalid, it only does not register the custom property instead of dropping the whole rule. Other implementations also follow that aproach and existing web platform tests disagree with the specification [2]. This patch aligns Firefox's behavior with the specification, by moving @Property validation during parsing and dropping invalid rules. Tests are updated as follows: 1. /css/css-properties-values-api/at-property-cssom.html Existing tests that don't have the three descriptors (syntax, inherit, initial-value) are invalid and now the test verifies no corresponding rules are exposed via CSSOM. `--no-initial-value` is renamed `--no-initial-color-value` and its legacy tests are kept for a new @Property `--no-initial-universal-value` which uses the universal syntax (so initial value is optional). Some dummy descriptors are added for --tab\ttab so that it remains valid. Similarly, we ensure --valid-whitespace's syntax (space-separated) and initial-value (comma-separated) agree. 2. /css/css-properties-values-api/at-property.html Existing `test_descriptor()` tests are trying an @Property with a single specified descriptor and so are always invalid. To work around that, we tweak `test_descriptor()` so that it can build a valid descriptor instead. The `syntax` and `inherits` fallback to universal and true respectively while the `initial-value` descriptor is built from the `syntax`. An extra parameters is introduced in case the caller wants to provide these values directly. Finally, when the expected value is null the function instead verifies that the rule is really dropped. 2.1. Some existing syntax tests are registering rules with unquoted syntax value 'red', 'rgb(255, 0, 0)', 'color', 'foo | bar' and expect to obtain a rule with an empty syntax string, suggesting some kind of invalidity handling (cf similar tests). We interpret the first two as "specifying a color value", quotes are added and the first one actually becomes a valid custom-ident. The last two already have a similar quoted version, so we just interpret them as "missing quotes". 2.2. Given the previous 'red' custom-ident, we add tests for invalid custom-ident as defined in [3]. 2.3. Some existing `syntax` tests are checking that we must have "pipe between components" and no "leading bar" and are again expecting a rule with an empty syntax string. We fix the apparent mistake of missing quotes and provide initial values that could potentially be interpreted as correct by implementations accepting these invalid syntaxes. 2.4. One `initial-value` test is checking "var(--x)" but that is not computationally independent so tweak the test to check that makes the @Property rule invalid. Also add a similar '3em' test mentioned in the spec. 2.5. Some `inherits` tests verify that invalid rules are interpreted as false. It seems they should instead be treated as if it does not exist and so should make the @Property rule invalid. [1] https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule [2] w3c/css-houdini-drafts#1098 [3] https://drafts.csswg.org/css-values-4/#custom-idents Differential Revision: https://phabricator.services.mozilla.com/D190444

…dropped. Currently Firefox properly performs validation of an @Property rule, as defined in [1]. However when such a rule is invalid, it only does not register the custom property instead of dropping the whole rule. Other implementations also follow that aproach and existing web platform tests disagree with the specification [2]. This patch aligns Firefox's behavior with the specification, by moving @Property validation during parsing and dropping invalid rules. Tests are updated as follows: 1. /css/css-properties-values-api/at-property-cssom.html Existing tests that don't have the three descriptors (syntax, inherit, initial-value) are invalid and now the test verifies no corresponding rules are exposed via CSSOM. `--no-initial-value` is renamed `--no-initial-color-value` and its legacy tests are kept for a new @Property `--no-initial-universal-value` which uses the universal syntax (so initial value is optional). Some dummy descriptors are added for --tab\ttab so that it remains valid. Similarly, we ensure --valid-whitespace's syntax (space-separated) and initial-value (comma-separated) agree. 2. /css/css-properties-values-api/at-property.html Existing `test_descriptor()` tests are trying an @Property with a single specified descriptor and so are always invalid. To work around that, we tweak `test_descriptor()` so that it can build a valid descriptor instead. The `syntax` and `inherits` fallback to universal and true respectively while the `initial-value` descriptor is built from the `syntax`. An extra parameters is introduced in case the caller wants to provide these values directly. Finally, when the expected value is null the function instead verifies that the rule is really dropped. 2.1. Some existing syntax tests are registering rules with unquoted syntax value 'red', 'rgb(255, 0, 0)', 'color', 'foo | bar' and expect to obtain a rule with an empty syntax string, suggesting some kind of invalidity handling (cf similar tests). We interpret the first two as "specifying a color value", quotes are added and the first one actually becomes a valid custom-ident. The last two already have a similar quoted version, so we just interpret them as "missing quotes". 2.2. Given the previous 'red' custom-ident, we add tests for invalid custom-ident as defined in [3]. 2.3. Some existing `syntax` tests are checking that we must have "pipe between components" and no "leading bar" and are again expecting a rule with an empty syntax string. We fix the apparent mistake of missing quotes and provide initial values that could potentially be interpreted as correct by implementations accepting these invalid syntaxes. 2.4. One `initial-value` test is checking "var(--x)" but that is not computationally independent so tweak the test to check that makes the @Property rule invalid. Also add a similar '3em' test mentioned in the spec. 2.5. Some `inherits` tests verify that invalid rules are interpreted as false. It seems they should instead be treated as if it does not exist and so should make the @Property rule invalid. [1] https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule [2] w3c/css-houdini-drafts#1098 [3] https://drafts.csswg.org/css-values-4/#custom-idents Differential Revision: https://phabricator.services.mozilla.com/D190444 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1857724 gecko-commit: cfd3a0958f03cb28be6b62a57eb5873358189e1e gecko-reviewers: emilio

…ns should be dropped. r=emilio Currently Firefox properly performs validation of an property rule, as defined in [1]. However when such a rule is invalid, it only does not register the custom property instead of dropping the whole rule. Other implementations also follow that aproach and existing web platform tests disagree with the specification [2]. This patch aligns Firefox's behavior with the specification, by moving property validation during parsing and dropping invalid rules. Tests are updated as follows: 1. /css/css-properties-values-api/at-property-cssom.html Existing tests that don't have the three descriptors (syntax, inherit, initial-value) are invalid and now the test verifies no corresponding rules are exposed via CSSOM. `--no-initial-value` is renamed `--no-initial-color-value` and its legacy tests are kept for a new property `--no-initial-universal-value` which uses the universal syntax (so initial value is optional). Some dummy descriptors are added for --tab\ttab so that it remains valid. Similarly, we ensure --valid-whitespace's syntax (space-separated) and initial-value (comma-separated) agree. 2. /css/css-properties-values-api/at-property.html Existing `test_descriptor()` tests are trying an property with a single specified descriptor and so are always invalid. To work around that, we tweak `test_descriptor()` so that it can build a valid descriptor instead. The `syntax` and `inherits` fallback to universal and true respectively while the `initial-value` descriptor is built from the `syntax`. An extra parameters is introduced in case the caller wants to provide these values directly. Finally, when the expected value is null the function instead verifies that the rule is really dropped. 2.1. Some existing syntax tests are registering rules with unquoted syntax value 'red', 'rgb(255, 0, 0)', 'color', 'foo | bar' and expect to obtain a rule with an empty syntax string, suggesting some kind of invalidity handling (cf similar tests). We interpret the first two as "specifying a color value", quotes are added and the first one actually becomes a valid custom-ident. The last two already have a similar quoted version, so we just interpret them as "missing quotes". 2.2. Given the previous 'red' custom-ident, we add tests for invalid custom-ident as defined in [3]. 2.3. Some existing `syntax` tests are checking that we must have "pipe between components" and no "leading bar" and are again expecting a rule with an empty syntax string. We fix the apparent mistake of missing quotes and provide initial values that could potentially be interpreted as correct by implementations accepting these invalid syntaxes. 2.4. One `initial-value` test is checking "var(--x)" but that is not computationally independent so tweak the test to check that makes the property rule invalid. Also add a similar '3em' test mentioned in the spec. 2.5. Some `inherits` tests verify that invalid rules are interpreted as false. It seems they should instead be treated as if it does not exist and so should make the property rule invalid. [1] https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule [2] w3c/css-houdini-drafts#1098 [3] https://drafts.csswg.org/css-values-4/#custom-idents Differential Revision: https://phabricator.services.mozilla.com/D190444 UltraBlame original commit: cfd3a0958f03cb28be6b62a57eb5873358189e1e

foolip · 2023-10-18T08:20:58Z

FYI, I've filed web-platform-tests/interop#587 about the test changes affecting Interop 2023. We have no CI enforcement that Interop 2023 tests aren't changed, but we do review any changes. Filing test change proposals before the changes are made is appreciated.

emilio · 2023-10-18T11:04:38Z

As per the above, it was discussed on another interop issue. Let's change this since implementations should start matching the spec shortly.

…ons should be dropped. r=emilio Currently Firefox properly performs validation of an @Property rule, as defined in [1]. However when such a rule is invalid, it only does not register the custom property instead of dropping the whole rule. Other implementations also follow that aproach and existing web platform tests disagree with the specification [2]. This patch aligns Firefox's behavior with the specification, by moving @Property validation during parsing and dropping invalid rules. Tests are updated as follows: 1. /css/css-properties-values-api/at-property-cssom.html Existing tests that don't have the three descriptors (syntax, inherit, initial-value) are invalid and now the test verifies no corresponding rules are exposed via CSSOM. `--no-initial-value` is renamed `--no-initial-color-value` and its legacy tests are kept for a new @Property `--no-initial-universal-value` which uses the universal syntax (so initial value is optional). Some dummy descriptors are added for --tab\ttab so that it remains valid. Similarly, we ensure --valid-whitespace's syntax (space-separated) and initial-value (comma-separated) agree. 2. /css/css-properties-values-api/at-property.html Existing `test_descriptor()` tests are trying an @Property with a single specified descriptor and so are always invalid. To work around that, we tweak `test_descriptor()` so that it can build a valid descriptor instead. The `syntax` and `inherits` fallback to universal and true respectively while the `initial-value` descriptor is built from the `syntax`. An extra parameters is introduced in case the caller wants to provide these values directly. Finally, when the expected value is null the function instead verifies that the rule is really dropped. 2.1. Some existing syntax tests are registering rules with unquoted syntax value 'red', 'rgb(255, 0, 0)', 'color', 'foo | bar' and expect to obtain a rule with an empty syntax string, suggesting some kind of invalidity handling (cf similar tests). We interpret the first two as "specifying a color value", quotes are added and the first one actually becomes a valid custom-ident. The last two already have a similar quoted version, so we just interpret them as "missing quotes". 2.2. Given the previous 'red' custom-ident, we add tests for invalid custom-ident as defined in [3]. 2.3. Some existing `syntax` tests are checking that we must have "pipe between components" and no "leading bar" and are again expecting a rule with an empty syntax string. We fix the apparent mistake of missing quotes and provide initial values that could potentially be interpreted as correct by implementations accepting these invalid syntaxes. 2.4. One `initial-value` test is checking "var(--x)" but that is not computationally independent so tweak the test to check that makes the @Property rule invalid. Also add a similar '3em' test mentioned in the spec. 2.5. Some `inherits` tests verify that invalid rules are interpreted as false. It seems they should instead be treated as if it does not exist and so should make the @Property rule invalid. [1] https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule [2] w3c/css-houdini-drafts#1098 [3] https://drafts.csswg.org/css-values-4/#custom-idents Differential Revision: https://phabricator.services.mozilla.com/D190444

andruud added the css-properties-values-api-1 label Sep 13, 2023

cdoublev mentioned this issue Sep 27, 2023

[css-fonts-4] @font-palette-values lacking declaration for font-family w3c/csswg-drafts#9417

Closed

fred-wang mentioned this issue Oct 10, 2023

@property tests should check invalid rules are dropped web-platform-tests/interop#575

Closed

moz-wptsync-bot mentioned this issue Oct 12, 2023

[Gecko Bug 1857724] [css-properties-values-api] Invalid @property declarations should be dropped. web-platform-tests/wpt#42500

Merged

foolip mentioned this issue Oct 18, 2023

Review recent changes to Custom Properties tests web-platform-tests/interop#587

Closed

emilio closed this as completed Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[css-properties-values-api] Rule validity doesn't match implementations #1098

[css-properties-values-api] Rule validity doesn't match implementations #1098

emilio commented Jun 26, 2023

andruud commented Jun 26, 2023

cdoublev commented Aug 26, 2023 •

edited

Loading

tabatkins commented Aug 28, 2023

andruud commented Sep 13, 2023

fred-wang commented Oct 10, 2023 •

edited

Loading

fred-wang commented Oct 10, 2023

cdoublev commented Oct 11, 2023 •

edited

Loading

foolip commented Oct 18, 2023

emilio commented Oct 18, 2023

[css-properties-values-api] Rule validity doesn't match implementations #1098

[css-properties-values-api] Rule validity doesn't match implementations #1098

Comments

emilio commented Jun 26, 2023

andruud commented Jun 26, 2023

cdoublev commented Aug 26, 2023 • edited Loading

tabatkins commented Aug 28, 2023

andruud commented Sep 13, 2023

fred-wang commented Oct 10, 2023 • edited Loading

fred-wang commented Oct 10, 2023

cdoublev commented Oct 11, 2023 • edited Loading

foolip commented Oct 18, 2023

emilio commented Oct 18, 2023

cdoublev commented Aug 26, 2023 •

edited

Loading

fred-wang commented Oct 10, 2023 •

edited

Loading

cdoublev commented Oct 11, 2023 •

edited

Loading