Bug 217623 – CSSStyleDeclaration breaks JS spec (properties not showing up in Object.getOwnPropertyNames)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Sneddon [:gsnedders]]

> "-webkit-border-before" in document.body.style
< true
> document.body.style.hasOwnProperty("-webkit-border-before")
< true
> Object.keys(document.body.style).indexOf("-webkit-border-before")
< -1
> Object.getOwnPropertyNames(document.body.style).indexOf("-webkit-border-before")
< -1
> Object.getOwnPropertyNames(document.body.style.__proto__).indexOf("-webkit-border-before")
< -1
> Object.getOwnPropertyNames(document.body.style.__proto__.__proto__).indexOf("-webkit-border-before")
< -1

Note that the JS spec says for [[OwnPropertyKeys]] ( ), as an invariant:

> The returned List must contain at least the keys of all non-configurable own properties that have previously been observed.

[Sam Weinig]

This is intertwined with this silly comment I made a few years ago (see https://trac.webkit.org/changeset/219622/webkit) in CSSStyleDeclaration.idl

`
    // Use named getters/setters for all support CSS property names. The spec says
    // these should be normal attributes, but there are too many combinations with
    // prefixes for this to be practical. If we remove legacy aliases, we should
    // reconsider this decision.
    getter (DOMString or double) (DOMString name);
    [CEReactions, CallNamedSetterOnlyForSupportedProperties] setter undefined (DOMString name, [LegacyNullToEmptyString] DOMString value);
`

The easiest (and perhaps best) way to fix this is generate a supplemental IDL file from CSSProperties.json and pass that to the bindings generator using the various rules from https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface.

[Sam Weinig]

Created attachment 411289 [details]
Patch

[Sam Weinig]

Created attachment 411292 [details]
Patch

[Sam Weinig]

Created attachment 411293 [details]
Patch

[Sam Sneddon [:gsnedders]]

"See also"-ing the other bugs that should hopefully be fixed by Sam's rewrite of the bindings code.

Also, Sam, can you review https://github.com/web-platform-tests/wpt/pull/26086 and https://github.com/web-platform-tests/wpt/pull/26082 as part of this? Should probably write a separate test for the Object.keys() issue described above.

[Sam Weinig]

Created attachment 411340 [details]
Patch

[Sam Weinig]

Created attachment 411349 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #7)
> Created attachment 411349 [details]
> Patch

Now we are getting somewhere. 1 progression, 7 regressions. Some notes on the failures:

- Got to decide if we want to keep the weird css/pixel/pos prefixed variants.
- We currently support things like "element.style.epubCaptionSide", where epub has a lowercase "e". This seems to be non-standard, with the standard suggesting this should be "element.style.EpubCaptionSide".
- We have at least one test of the iteration order, which is not something is defined for IDL members, so I am not sure we need to maintain that.

Need to look into the rest more closely.

[Sam Weinig]

Created attachment 411363 [details]
CSSStyleDeclaration+PropertyNames.idl

If anyone is interested, here is the generated IDL.

[Sam Weinig]

Created attachment 411395 [details]
Patch

[Sam Sneddon [:gsnedders]]

(In reply to Sam Weinig from comment #8)
> - Got to decide if we want to keep the weird css/pixel/pos prefixed variants.

If it's easy to keep them I think I'd rather do that as a later change (or an earlier change!)?

> - We currently support things like "element.style.epubCaptionSide", where
> epub has a lowercase "e". This seems to be non-standard, with the standard
> suggesting this should be "element.style.EpubCaptionSide".

While Chrome used to expose epubCaptionSide, they seem to only expose "-epub-caption-side" and now? So maybe we can drop epubCaptionSide?

> - We have at least one test of the iteration order, which is not something
> is defined for IDL members, so I am not sure we need to maintain that.

Us and Chrome seem pretty similar in enumeration order today, whereas Firefox seems a bit all over the place. Not sure it matters too much?

[Sam Weinig]

Created attachment 411449 [details]
Patch

[Sam Weinig]

(In reply to Sam Sneddon [:gsnedders] from comment #11)
> (In reply to Sam Weinig from comment #8)
> > - Got to decide if we want to keep the weird css/pixel/pos prefixed variants.
> 
> If it's easy to keep them I think I'd rather do that as a later change (or
> an earlier change!)?

I'm going to definitely keep them for this change but I think we should consider dropping them (it's better to drop them after the refactor as a separate step, since if we have to bring them back, we don't have to figure out how to make them work in the new system later on).

> 
> > - We currently support things like "element.style.epubCaptionSide", where
> > epub has a lowercase "e". This seems to be non-standard, with the standard
> > suggesting this should be "element.style.EpubCaptionSide".
> 
> While Chrome used to expose epubCaptionSide, they seem to only expose
> "-epub-caption-side" and now? So maybe we can drop epubCaptionSide?

Do they expose element.style.EpubCaptionSide (with a capital E)?

[Sam Weinig]

Created attachment 411453 [details]
Patch

[Sam Weinig]

Created attachment 411454 [details]
Patch

[Sam Sneddon [:gsnedders]]

(In reply to Sam Weinig from comment #13)
> (In reply to Sam Sneddon [:gsnedders] from comment #11)
> > (In reply to Sam Weinig from comment #8)
> > > - We currently support things like "element.style.epubCaptionSide", where
> > > epub has a lowercase "e". This seems to be non-standard, with the standard
> > > suggesting this should be "element.style.EpubCaptionSide".
> > 
> > While Chrome used to expose epubCaptionSide, they seem to only expose
> > "-epub-caption-side" and now? So maybe we can drop epubCaptionSide?
> 
> Do they expose element.style.EpubCaptionSide (with a capital E)?

https://wpt.fyi/results/css/css-conditional/js/CSS-supports-CSSStyleDeclaration.html?label=pr_head&max-count=1&pr=26086 shows they only support -epub-caption-side and not EpubCaptionSide. (Plz review the PR!)

[Sam Weinig]

(In reply to Sam Sneddon [:gsnedders] from comment #16)
> (In reply to Sam Weinig from comment #13)
> > (In reply to Sam Sneddon [:gsnedders] from comment #11)
> > > (In reply to Sam Weinig from comment #8)
> > > > - We currently support things like "element.style.epubCaptionSide", where
> > > > epub has a lowercase "e". This seems to be non-standard, with the standard
> > > > suggesting this should be "element.style.EpubCaptionSide".
> > > 
> > > While Chrome used to expose epubCaptionSide, they seem to only expose
> > > "-epub-caption-side" and now? So maybe we can drop epubCaptionSide?
> > 
> > Do they expose element.style.EpubCaptionSide (with a capital E)?
> 
> https://wpt.fyi/results/css/css-conditional/js/CSS-supports-
> CSSStyleDeclaration.html?label=pr_head&max-count=1&pr=26086 shows they only
> support -epub-caption-side and not EpubCaptionSide. (Plz review the PR!)

Hey Sam, I'm not exactly sure what you mean by review the PR. I don't think I am reviewer on that project. Can anyone review things?

[Sam Weinig]

Created attachment 411475 [details]
Patch

[Sam Weinig]

Split out bindings mangling changes into 217776.

[Sam Weinig]

Created attachment 411481 [details]
Patch

[Darin Adler]

Comment on attachment 411481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411481&action=review

> Source/WebCore/DerivedSources.make:1422
> +    CSSStyleDeclaration+PropertyNames.idl \

Maybe we should use $(CSS_PROPERTY_NAME_FILES) here to make this rule a little less long.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3967
> +    return $param if $param =~ /^CSSOM/;

Maybe we should switch to this approach for some of the other prefixes later?

> Source/WebCore/css/CSSStyleDeclaration.cpp:309
> +    String stringValue = WTF::switchOn(value, [&] (const auto& input) {

auto?

> Source/WebCore/css/CSSStyleDeclaration.h:75
> +    ExceptionOr<void> setPropertyValueInternal(CSSPropertyID, String);

How did you decide on String vs. const String& here?

> Source/WebCore/css/CSSStyleDeclaration.h:77
> +    ExceptionOr<void> setPropertyValueInternalForPosOrPixelPrefixed(CSSPropertyID, Variant<double, String>);

How did you decide on Variant vs. const Variant& here?

> LayoutTests/ChangeLog:13
> +        Remove tests for iteration order, which is not standardized, and not consistent among
> +        browsers.

Makes me a little sad; since someone could rely on the order by accident, wouldn’t there be some value in standardizing it, working towards being consistent among browsers some day, and testing that it matches what we propose for the standard?

[Sam Sneddon [:gsnedders]]

(In reply to Darin Adler from comment #21)

> Makes me a little sad; since someone could rely on the order by accident,
> wouldn’t there be some value in standardizing it, working towards being
> consistent among browsers some day, and testing that it matches what we
> propose for the standard?

This is partly covered by https://github.com/heycam/webidl/issues/432 and partly by CSSOM not defining what order the attributes are generated in.

[Darin Adler]

When I say "removing tests makes me a little sad", it’s more than just annoyed that we don’t have order specified, and the risk that posts for interoperability. It’s also that I like tests to help us keep our behavior stable, which does not require that the behavior be specified or standardized. So that part of removing the tests makes me sad too. Standards are very important, but they don’t necessarily constrain what we should test!

[Darin Adler]

posts -> poses

[Sam Weinig]

(In reply to Darin Adler from comment #21)
> Comment on attachment 411481 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411481&action=review
> 
> > Source/WebCore/DerivedSources.make:1422
> > +    CSSStyleDeclaration+PropertyNames.idl \
> 
> Maybe we should use $(CSS_PROPERTY_NAME_FILES) here to make this rule a
> little less long.

Good idea.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3967
> > +    return $param if $param =~ /^CSSOM/;
> 
> Maybe we should switch to this approach for some of the other prefixes later?

Yeah. We need to make this more straightforward.

> 
> > Source/WebCore/css/CSSStyleDeclaration.cpp:309
> > +    String stringValue = WTF::switchOn(value, [&] (const auto& input) {
> 
> auto?

Done.

> 
> > Source/WebCore/css/CSSStyleDeclaration.h:75
> > +    ExceptionOr<void> setPropertyValueInternal(CSSPropertyID, String);
> 
> How did you decide on String vs. const String& here?
> 
> > Source/WebCore/css/CSSStyleDeclaration.h:77
> > +    ExceptionOr<void> setPropertyValueInternalForPosOrPixelPrefixed(CSSPropertyID, Variant<double, String>);
> 
> How did you decide on Variant vs. const Variant& here?

For both of these I don't have a good rule of thumb here. Currently the bindings always WTFMove() into implementation arguments, so it seemed like the right thing to do. But I would love to have a better rule / intuition on this.

[Sam Weinig]

(In reply to Darin Adler from comment #23)
> When I say "removing tests makes me a little sad", it’s more than just
> annoyed that we don’t have order specified, and the risk that posts for
> interoperability. It’s also that I like tests to help us keep our behavior
> stable, which does not require that the behavior be specified or
> standardized. So that part of removing the tests makes me sad too. Standards
> are very important, but they don’t necessarily constrain what we should test!

I've been trying to come up with a way to test iteration order stability for WebKit that isn't a pain. The problem with many of the existing tests has been that each port needed separate results since some properties were enabled and some were not. I think maybe doing based on some of the Internal only interfaces that we expose for WebKitTestRunner might be a good way, as we could make sure to exercise all the variables. 

In this case, port specific properties were actually not the issue (though if I printed them all out it would be). But rather, the tests relied on false assumptions like nothing in the iteration order would sort below the string "0" (that was domListEnumeration.html) or that they should be sorted alphabetically, both because the old implementation explicitly provided a sorted list of properties. Now the implementation is using the same behavior we have for all other IDLs, which is things iterate in the order they are declared in the IDL.

[Sam Weinig]

Created attachment 411509 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #25)
> (In reply to Darin Adler from comment #21)
> > Comment on attachment 411481 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411481&action=review
> > 
> > > Source/WebCore/DerivedSources.make:1422
> > > +    CSSStyleDeclaration+PropertyNames.idl \
> > 
> > Maybe we should use $(CSS_PROPERTY_NAME_FILES) here to make this rule a
> > little less long.
> 
> Good idea.
> 

I couldn't quite work out how to make this work, my make-fu continues to be less than stellar. Will try to clean it up along with some others that are duplicated though.

[EWS]

Committed r268564: <https://trac.webkit.org/changeset/268564>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411509 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70359261>

[Darin Adler]

(In reply to Sam Weinig from comment #26)
> things iterate in the order they are declared in the IDL

So our new form of test that should be not too hard to maintain would be a based on a list of property names in the order they should appear *allowing missing properties*. If you agree that a test is still valuable.

[Darin Adler]

I can imagine a type of test where we list things in the order we expect to see them and use a trailing "?" to indicate items that may be omitted without failing the list. Then carefully design the test output so that it is unlikely to require different baselines even if there are some failures as long as they are the same failures.

[Alexey Proskuryakov]

As mentioned on Slack, this change resulted in a substantial build time regression (1 minute for production builds).

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #33)
> As mentioned on Slack, this change resulted in a substantial build time
> regression (1 minute for production builds).

The regression has been partially addressed by https://trac.webkit.org/changeset/268957/webkit and https://trac.webkit.org/changeset/268962/webkit.

I am waiting for credentials to sync so I can get more data.

[Alexey Proskuryakov]

This also made WebCore 9% larger.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 218525

[Emilio Cobos Álvarez (:emilio)]

Should this reland now that the blocking bug is fixed?

[Emilio Cobos Álvarez (:emilio)]

Ah lol, blocking bug was fixed by revert :-)

[Sam Weinig]

Filed https://bugs.webkit.org/show_bug.cgi?id=222517, to re-land this change behind an #ifdef to allow iteration on it in the tree without negatively affecting compile time or binary size.

Once landed, we can iterate on reducing the regression and once it is gone, enable it for real.

[Sam Weinig]

(In reply to Sam Weinig from comment #39)
> Filed https://bugs.webkit.org/show_bug.cgi?id=222517, to re-land this change
> behind an #ifdef to allow iteration on it in the tree without negatively
> affecting compile time or binary size.
> 
> Once landed, we can iterate on reducing the regression and once it is gone,
> enable it for real.

Will use https://bugs.webkit.org/show_bug.cgi?id=222518 to track finding a solution that does not regress things.

[Sam Weinig]

This is now fixed with <https://commits.webkit.org/236379@main>.