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

@property tests should check invalid rules are dropped #575

Closed
fred-wang opened this issue Oct 10, 2023 · 11 comments
Closed

@property tests should check invalid rules are dropped #575

fred-wang opened this issue Oct 10, 2023 · 11 comments
Labels
focus area: Custom Properties test-change-proposal Proposal to add or remove tests for an interop area

Comments

@fred-wang
Copy link

Test List

css/css-properties-values-api/at-property-cssom.html
css/css-properties-values-api/at-property.html

Rationale

As discussed on w3c/css-houdini-drafts#1098 the spec describes when @property rules are invalid. The tests mentioned above incorrectly assumes such invalid @property rules are "exposed via CSSOM and don't lead to any registered custom property" while they should just be dropped. https://phabricator.services.mozilla.com/D190444 fixes Firefox's implementation to align with the spec and adjust web platform tests accordingly.

Alternatively, and as mentioned on w3c/css-houdini-drafts#1098, the spec could also be modified to align with existing implementations & tests, and with the behavior of @counter-style / @font-face.

@foolip
Copy link
Member

foolip commented Oct 10, 2023

@andruud can you review this for Chromium?

@fred-wang
Copy link
Author

cc @nt1m @anttijk since they made reviews/commits in WebKit related to these tests.

cc @emilio who approved https://phabricator.services.mozilla.com/D190444

@emilio
Copy link

emilio commented Oct 11, 2023

I'm fine following the spec and fixing the tests here. Per the discussion in the issue the spec authors seem fine with it too. This is not likely to have much compat impact, and we have a patch already (above).

@nt1m
Copy link
Member

nt1m commented Oct 11, 2023

I admit I'm not a fan of the idea of making a last minute change to interop with little benefit for developers/users. I'd prefer having the spec align with implementations unless there's really a compelling reason to do this change.

@fred-wang
Copy link
Author

fred-wang commented Oct 11, 2023

As a side note, even if we decide to change the spec instead, the mentioned tests still have some small mistakes that could be fixed. For example some tests for invalid @property don't use quotes for the syntax descriptor, so they would be invalid for that reason rather than the thing they mean to test. Or the @property --valid-whitespace rule is actually invalid because syntax and initial-value don't match.

The patch for Firefox suggests aligning with the spec would be small effort (basically moving the validation check around), but not sure that's true for Chromium/WebKit...

@emilio
Copy link

emilio commented Oct 11, 2023

I think it's relatively low effort implementation-wise, and the current implementation behavior isn't particularly useful, since the property rule is effectively immutable.

@nt1m
Copy link
Member

nt1m commented Oct 11, 2023

It's probably an OK change to do, looks good from WebKit.

@nt1m
Copy link
Member

nt1m commented Oct 11, 2023

w3c/css-houdini-drafts#1098 (comment) seems like a green light from Blink to do this change. @emilio / @fred-wang Please feel free to land the test change.

@andruud
Copy link

andruud commented Oct 11, 2023

w3c/css-houdini-drafts#1098 (comment) seems like a green light from Blink to do this change. @emilio / @fred-wang Please feel free to land the test change.

Indeed, go ahead.

@nt1m
Copy link
Member

nt1m commented Oct 12, 2023

I'm going to close this since the Gecko commit landed on autoland.

@nt1m nt1m closed this as completed Oct 12, 2023
@nt1m
Copy link
Member

nt1m commented Oct 12, 2023

For more reference, the PR is: web-platform-tests/wpt#42500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Custom Properties test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

5 participants