Bug 104805 – Do not allow unitless values for CSS unprefixed perspective property

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

[Alexis Menard (darktears)]

Today -webkit-perspective accept valueless values which is not part of the CSS Transforms specification. We should keep the behavior for the old -webkit-perspective property but we should make the parsing stricter for the unprefixed version.

[Eric Willigers]

Microsoft calculates use counts, and no usage of unitless numbers is reported on top sites.
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/perspective/
For comparison, see https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/stroke-width/

[Sam Sneddon [:gsnedders]]

wpt/quirks/unitless-length/excluded-properties-001.html covers this with the "Property perspective does not support quirky length" subtest; we're the only browser to fail this subtest.

[Radar WebKit Bug Importer]

<rdar://problem/77016090>

[Martin Robinson]

Created attachment 443104 [details]
Patch

[Martin Robinson]

Created attachment 443110 [details]
Patch

[Martin Robinson]

Created attachment 443183 [details]
Patch

[Martin Robinson]

Created attachment 443395 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Martin Robinson]

Comment on attachment 443395 [details]
Patch

I think this is ready for review.

[Myles C. Maxfield]

This is a bit scary. Simon should be the one to review it.

[Tim Nguyen (:ntim)]

Comment on attachment 443395 [details]
Patch

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

This looks fine aside from my comments, though I'd like Oriol to look at it before giving r+, since he's done work to make sure such aliased properties get the correct computed style.

> Source/WebCore/css/CSSProperties.json:6568
>          "perspective": {
>              "codegen-properties": {
> -                "aliases": [
> -                    "-webkit-perspective"
> -                ],
> +                "conditional-converter": "Perspective"
> +            },
> +            "specification": {
> +                "category": "css-transforms",
> +                "url": "https://www.w3.org/TR/css-transforms-1/#transform-style-property"
> +            }
> +        },
> +        "-webkit-perspective": {

Please add the related-property field to both of these, this way they can share the computed style (which is behavior you'd have with alias)

perspective should have -webkit-perspective as related property and vice-versa

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2435
> +    auto value = consumePerspective(range, cssParserMode);
> +    if (value)
> +        return value;

if (auto value = consumePerspective(range, cssParserMode))
  return value;

[Tim Nguyen (:ntim)]

Oriol, could you take a look at this patch, especially regarding the gCS() stuff?

[Oriol Brufau]

Comment on attachment 443395 [details]
Patch

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

>> Source/WebCore/css/CSSProperties.json:6568
>> +        "-webkit-perspective": {
> 
> Please add the related-property field to both of these, this way they can share the computed style (which is behavior you'd have with alias)
> 
> perspective should have -webkit-perspective as related property and vice-versa

Yes, longhands sharing a computed style should have related-property so that revert and revert-layer work correctly.
Strange that css/css-cascade/all-prop-revert-layer.html didn't catch it in EWS, locally I get the failure.

However, sharing a computed style is a non-standard hack (except for logical and physical properties), so I would prefer not to add more instances of that unless necessary.

The spec provides 2 standard possibilities: https://www.w3.org/TR/css-cascade-5/#aliasing
The 1st one is an alias, the current behavior, but it's not suitable since the grammars are different. Then the other option is a legacy shorthand.

So you could make -webkit-perspective a shorthand of perspective. Just let the parser for -webkit-perspective accept unitless values, but add the units when expanding to perspective.

Being a shorthand it may not be necessary to handle it in CSSPropertyAnimation.cpp and WillChangeData.cpp, but not sure.

[Martin Robinson]

Created attachment 459228 [details]
Patch

[Martin Robinson]

Thanks for the reviews. I have uploaded a new version of this change that makes `-webkit-perspective` a shorthand for `perspective`. Though it does seem a little counter-intuitive, it seems to work well.

[Oriol Brufau]

Comment on attachment 459228 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:3139
> +        new PerspectiveWrapper(CSSPropertyPerspective),

If CSSPropertyWebkitPerspective doesn't need to be handled here, you can avoid modifying this file.

> Source/WebCore/css/CSSProperties.json:6715
> +                "url": "https://www.w3.org/TR/css-transforms-1/#transform-style-property"

The link was already wrong, but I guess it should be https://www.w3.org/TR/css-transforms-2/#perspective-property

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2469
>          return nullptr;

Nit: You may remove this since there is another "return null" just after the conditional.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2479
> +        return true;

Should check m_range.atEnd(). Otherwise:

CSS.supports("-webkit-perspective", "1px foo"); // true

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2483
> +        if (perspective->value < 0)

Should check m_range.atEnd(). Otherwise:

CSS.supports("-webkit-perspective", "1 foo"); // true

[Martin Robinson]

Created attachment 459232 [details]
Patch

[Martin Robinson]

(In reply to Oriol Brufau from comment #16)
> Comment on attachment 459228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459228&action=review
> 
> > Source/WebCore/animation/CSSPropertyAnimation.cpp:3139
> > +        new PerspectiveWrapper(CSSPropertyPerspective),
> 
> If CSSPropertyWebkitPerspective doesn't need to be handled here, you can
> avoid modifying this file.

Sorry. This was an oversight. It should be fixed now as well as the left over code in WillChangeData.cpp.

> > Source/WebCore/css/CSSProperties.json:6715
> > +                "url": "https://www.w3.org/TR/css-transforms-1/#transform-style-property"
> 
> The link was already wrong, but I guess it should be
> https://www.w3.org/TR/css-transforms-2/#perspective-property

Fixed both links.

> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2469
> >          return nullptr;
> 
> Nit: You may remove this since there is another "return null" just after the
> conditional.

Fixed.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2479
> > +        return true;
> 
> Should check m_range.atEnd(). Otherwise:
> 
> CSS.supports("-webkit-perspective", "1px foo"); // true
> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2483
> > +        if (perspective->value < 0)
> 
> Should check m_range.atEnd(). Otherwise:
> 
> CSS.supports("-webkit-perspective", "1 foo"); // true

Fixed both of these.

[Oriol Brufau]

Comment on attachment 459232 [details]
Patch

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

Informal LGTM.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:6594
> +    case CSSPropertyWebkitPerspective: {

Nit: the curly brackets are not needed.

[Tim Nguyen (:ntim)]

Comment on attachment 459232 [details]
Patch

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

> Source/WebCore/css/CSSProperties.json:6715
> +                "url": " https://www.w3.org/TR/css-transforms-2/#perspective-property"
> +            }
> +        },
> +        "-webkit-perspective": {
> +            "codegen-properties": {
> +                "longhands": [ "perspective" ]
> +            },
> +            "specification": {
> +                "category": "css-transforms",
> +                "url": " https://www.w3.org/TR/css-transforms-2/#perspective-property"

nit: The urls have a bogus space before it

also for consistency with the rest of the file, could you give `"perspective"` its own line in the longhands field?

[Tim Nguyen (:ntim)]

Comment on attachment 459232 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2476
> +    if (RefPtr<CSSValue> value = consumePerspective(m_range, m_context.mode)) {

Does `auto value = ...` or `RefPtr value = ...`  work here?

[Martin Robinson]

Created attachment 459242 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 459242 [details]
Patch

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

> Source/WebCore/css/CSSProperties.json:6718
>              }

Can you add a status field like this one: https://webkit-search.igalia.com/webkit/rev/0d5437dbaa7bdb52f60b64bc7dd5a39adee14e81/Source/WebCore/css/CSSProperties.json#5444-5446 mentioning the quirk?

[Tim Nguyen (:ntim)]

The previous patch seems to suggest you need to rebaseline imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree.html

[Martin Robinson]

Created attachment 459243 [details]
Patch

[Martin Robinson]

(In reply to Tim Nguyen (:ntim) from comment #24)
> The previous patch seems to suggest you need to rebaseline
> imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree.
> html

I examined that test and I couldn't see any reason my change would modify those results. I uploaded a new version to see if the bots would fail on it again.

> Can you add a status field like this one:
> https://webkit-search.igalia.com/webkit/rev/
> 0d5437dbaa7bdb52f60b64bc7dd5a39adee14e81/Source/WebCore/css/CSSProperties.
> json#5444-5446 mentioning the quirk?

Sure. That's a good idea.

[Tim Nguyen (:ntim)]

(In reply to Martin Robinson from comment #26)
> (In reply to Tim Nguyen (:ntim) from comment #24)
> > The previous patch seems to suggest you need to rebaseline
> > imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree.
> > html
> 
> I examined that test and I couldn't see any reason my change would modify
> those results. I uploaded a new version to see if the bots would fail on it
> again.

The length of the computed style changed with your patch. This often happens when removing/adding properties.

The asserts test specifically the length of the computed style to be 0: https://webkit-search.igalia.com/webkit/source/LayoutTests/imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt#3

Unfortunately, since we fail it, the assert message depends on the number of properties exposed.

[Tim Nguyen (:ntim)]

There's also an animation related assert: https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/459243-30692/accessibility/mac/media-role-descriptions-stderr.txt

[Oriol Brufau]

Turning an alias into a legacy shorthand shouldn't actually increase the length of the computed style, since shorthands are not supposed to be indexed. Filed bug 240356.

[Martin Robinson]

Created attachment 459283 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 459283 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        * css/CSSComputedStyleDeclaration.cpp:

I think this changelog is out-of-date (the animation change is not reflected)

> LayoutTests/ChangeLog:15
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Do not allow unitless values for CSS unprefixed perspective property
> +        https://bugs.webkit.org/show_bug.cgi?id=104805
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Do not allow unitless values for CSS unprefixed perspective property
> +        https://bugs.webkit.org/show_bug.cgi?id=104805
> +        <rdar://problem/77016090>
> +
> +        Reviewed by Tim Nguyen.

Double changelog

[Martin Robinson]

Created attachment 459284 [details]
Patch

[Tim Nguyen (:ntim)]

The non platform-specific imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt needs updating for mac as well.

[Martin Robinson]

Created attachment 459292 [details]
Patch

[EWS]

Committed r294224 (250582@main): <https://commits.webkit.org/250582@main>

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