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.
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/
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.
<rdar://problem/77016090>
Created attachment 443104 [details] Patch
Created attachment 443110 [details] Patch
Created attachment 443183 [details] Patch
Created attachment 443395 [details] Patch
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
Comment on attachment 443395 [details] Patch I think this is ready for review.
This is a bit scary. Simon should be the one to review it.
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;
Oriol, could you take a look at this patch, especially regarding the gCS() stuff?
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.
Created attachment 459228 [details] Patch
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.
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
Created attachment 459232 [details] Patch
(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.
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.
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?
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?
Created attachment 459242 [details] Patch
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?
The previous patch seems to suggest you need to rebaseline imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree.html
Created attachment 459243 [details] Patch
(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.
(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.
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
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.
Created attachment 459283 [details] Patch
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 > +2022-05-13 Martin Robinson <mrobinson@webkit.org> > + > + Do not allow unitless values for CSS unprefixed perspective property > + https://bugs.webkit.org/show_bug.cgi?id=104805 > + > + Reviewed by NOBODY (OOPS!). > + > + > +2022-05-12 Martin Robinson <mrobinson@webkit.org> > + > + 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
Created attachment 459284 [details] Patch
The non platform-specific imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree-expected.txt needs updating for mac as well.
Created attachment 459292 [details] Patch
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].