1857724 - [css-properties-values-api] Invalid @property declarations should be dropped

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zach Hoffman [:zrhoffman]]

The current implementation of @property parsing skips descriptors if they are invalid but registers the property regardless. This is incorrect, invalid @property rules should not result in a property registration

Comment 1

[Frédéric Wang (:fredw)]

Can you elaborate a bit on that?

The spec says "Unknown descriptors are invalid and ignored, but do not invalidate the @property rule." and this is covered by one test from /css/css-properties-values-api/determine-registration.html

As of syntax, inherits and initial-value descriptors , I read they won't be registered here:
https://searchfox.org/mozilla-central/source/servo/components/style/stylist.rs#2949

And https://searchfox.org/mozilla-central/source/servo/components/style/properties_and_values/rule.rs#261 seems to follow the spec to me (with syntax being absent if not parsed).

I tried with simple tests but could not reproduce.

Comment 2

[Frédéric Wang (:fredw)]

OK I checked https://github.com/w3c/css-houdini-drafts/issues/1098 and found Emilio's https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11822

And per discussion on the github issue, it seems people are leaning towards making "Unknown descriptors" invalidate @property rule too.

Comment 3

[Zach Hoffman [:zrhoffman]]

OK I checked https://github.com/w3c/css-houdini-drafts/issues/1098 and found Emilio's https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11822

Great! This will break a bunch some WPTs, but those WPTs should be adjusted (and probably an interop issue filed <https://github.com/web-platform-tests/interop/issues/new/choose>).(In reply to Frédéric Wang (:fredw) from comment #2)

And per discussion on the github issue, it seems people are leaning towards making "Unknown descriptors" invalidate @property rule too.

AIUI, the <https://github.com/w3c/css-houdini-drafts/issues/1098> discussion concludes that the spec is correct as-is. So when the spec says

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

our implementation seems correct, wrt that. Unless there is some discussion that I am missing?

Comment 4

[Frédéric Wang (:fredw)]

Attachment: Bug 1857724 - [css-properties-values-api] Invalid @property declarations should be dropped. r=#layout,#style

Comment 5

[Frédéric Wang (:fredw)]

(In reply to Zach Hoffman [:zrhoffman] from comment #3)

OK I checked https://github.com/w3c/css-houdini-drafts/issues/1098 and found Emilio's https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11822

Great! This will break a bunch some WPTs, but those WPTs should be adjusted (and probably an interop issue filed <https://github.com/web-platform-tests/interop/issues/new/choose>).(In reply to Frédéric Wang (:fredw) from comment #2)

I updated the WPT tests in https://phabricator.services.mozilla.com/D190444 and added a few more (it seems we fail 3 of them though) and opened https://github.com/web-platform-tests/interop/issues/575 ; I'm still not sure whether we want to fix the implementations/tests or the spec instead, but let's see.

And per discussion on the github issue, it seems people are leaning towards making "Unknown descriptors" invalidate @property rule too.

AIUI, the <https://github.com/w3c/css-houdini-drafts/issues/1098> discussion concludes that the spec is correct as-is. So when the spec says

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

our implementation seems correct, wrt that. Unless there is some discussion that I am missing?

OK, I misread andruud's comment about taht. I think the discussion does suggest any change for that case, just that Emilio was using this example as how "invalid @property rule" should be interpreted.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfd3a0958f03
[css-properties-values-api] Invalid @property declarations should be dropped. r=emilio

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42500 for changes under testing/web-platform/tests

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/cfd3a0958f03

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot