web-platform-tests / interop

web-platform-tests Interop project
https://wpt.fyi/interop
278 stars 28 forks source link

@property tests should check invalid rules are dropped #575

Closed fred-wang closed 10 months ago

fred-wang commented 10 months ago

Test List

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

Rationale

As discussed on https://github.com/w3c/css-houdini-drafts/issues/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 https://github.com/w3c/css-houdini-drafts/issues/1098, the spec could also be modified to align with existing implementations & tests, and with the behavior of @counter-style / @font-face.

foolip commented 10 months ago

@andruud can you review this for Chromium?

fred-wang commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

nt1m commented 10 months ago

https://github.com/w3c/css-houdini-drafts/issues/1098#issuecomment-1717463783 seems like a green light from Blink to do this change. @emilio / @fred-wang Please feel free to land the test change.

andruud commented 10 months ago

https://github.com/w3c/css-houdini-drafts/issues/1098#issuecomment-1717463783 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 commented 10 months ago

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

nt1m commented 10 months ago

For more reference, the PR is: https://github.com/web-platform-tests/wpt/pull/42500