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

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

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

moz-wptsync-bot
Copy link
Collaborator

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.

  1. /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

…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
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Firefox project.

@foolip
Copy link
Member

foolip commented Oct 18, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants