Implementation of the background-position-x/y 2 values syntax [chromium/src : main]
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
CSSValue* ConsumePositionLonghand(CSSParserTokenStream& stream,Javier FernandezConsumePositionLonghand is used for several -webkit- properties as well. Is it intentional that you change the behavior of those?
If so, they should also be covered by tests.
Rune LillesveenMy main goal was to solve a few interop issues in the background-position shorthand and its background-position-x/y longhand. I thought that since the change is back-compatible, we could reuse the same function. However, it's true that the webkit- prefixed properties are legacy and non-standard so it could be a better idea to implement this change in a new function, specifically applied to background-position-x/x.
I'm happy to add the tests if you think otherwise, though.
Javier FernandezAs a data point, does Safari/WebKit support the two-value syntax for these?:
-webkit-mask-position-x
-webkit-mask-position-y
-webkit-perspective-origin-x
-webkit-perspective-origin-y
-webkit-transform-origin-x
-webkit-transform-origin-y
Rune LillesveenWebkit supports the 2 value syntax in all the -webkit prefixed properties mentioned above.
Firefox supports the 2-value syntax in -webkit-mask-position-x. It implements the other 2 properties unprefixed, and it indeed supports the 2-value syntax on them.
Javier FernandezI see that the `-webkit-*-x` / `-webkit-*-y` variants are not mentioned in the compat spec. How about adding some tests for them in wpt_internal/ ?
Javier FernandezSure, I'll do that. As a mater of fact, I've found a case that crashes when using the 2-syntax on the -webkit-perspective-origin-x, so definitively more tests are needed there.
The webkit-perspective and webkit-transform require deeper changes to implement this new 2-value syntax. I'm not sure it's worth the effort, given that they are not standard and even the MDN documentation differs from the WebKit implementation.
For now I implemented the new syntax for the background-position and webkit-mask-position, which are implemented in the 3 main engines.
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Javier Fernandez
| Commit-Queue | +1 |
2 comments
Hi @futhark,
The patch is ready for for review.
CSSValue* ConsumePositionLonghand(CSSParserTokenStream& stream,Javier FernandezConsumePositionLonghand is used for several -webkit- properties as well. Is it intentional that you change the behavior of those?
If so, they should also be covered by tests.
Rune LillesveenMy main goal was to solve a few interop issues in the background-position shorthand and its background-position-x/y longhand. I thought that since the change is back-compatible, we could reuse the same function. However, it's true that the webkit- prefixed properties are legacy and non-standard so it could be a better idea to implement this change in a new function, specifically applied to background-position-x/x.
I'm happy to add the tests if you think otherwise, though.
Javier FernandezAs a data point, does Safari/WebKit support the two-value syntax for these?:
-webkit-mask-position-x
-webkit-mask-position-y
-webkit-perspective-origin-x
-webkit-perspective-origin-y
-webkit-transform-origin-x
-webkit-transform-origin-y
Rune LillesveenWebkit supports the 2 value syntax in all the -webkit prefixed properties mentioned above.
Firefox supports the 2-value syntax in -webkit-mask-position-x. It implements the other 2 properties unprefixed, and it indeed supports the 2-value syntax on them.
Javier FernandezI see that the `-webkit-*-x` / `-webkit-*-y` variants are not mentioned in the compat spec. How about adding some tests for them in wpt_internal/ ?
Javier FernandezSure, I'll do that. As a mater of fact, I've found a case that crashes when using the 2-syntax on the -webkit-perspective-origin-x, so definitively more tests are needed there.
The webkit-perspective and webkit-transform require deeper changes to implement this new 2-value syntax. I'm not sure it's worth the effort, given that they are not standard and even the MDN documentation differs from the WebKit implementation.
For now I implemented the new syntax for the background-position and webkit-mask-position, which are implemented in the 3 main engines.
Done
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
<!DOCTYPE html>We prefer wpt_internal over fast/ for non-standard properties. Could you use testharness.js instead of the old testRunner?
Related details
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
We prefer wpt_internal over fast/ for non-standard properties. Could you use testharness.js instead of the old testRunner?
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted Code-Review+1![]( "Open in Gerrit")
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/55034.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
I'm sorry @futhark but I reverted an unpexpected change in a comment of the background-position-y-valid.html test.
I don't understand why such change cleared out your review, though.
Could you please review it again ? Thanks and sorry again.
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted Code-Review+1![]( "Open in Gerrit")
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Implementation of the background-position-x/y 2 values syntax
Chrome implements the background-position-x/y CSS properties since long
ago, as part of the transition of the "background-position" property to
become a shorthand (as specified in the CSS Background L4). However,
Chrome was the only browser not implementing the 2-value syntax.
The L4 is still not ready for implementation and the 2-value syntax of
the background-position-x property is still being discussed in the CSS
WG and with several open issues related to the use of logical values
(eg, x-start). However, the use of physical values is not under
discussion and has been implemented by the 3 main browsers both for the
background-position (property in L3 and now a shorthand in L4) and the
background-position-x/y properties.
This CL implements the 2-value syntax for the background-position-x/y
considering only the already accepted physical values, given that it's
unlikely that the ongoing discussions may introduce changes that could
affect this implementation. Additionally, Safari and FF already
implements this syntax and there are several WPT to ensure the
interoperability if the implementation.
Sent an intent-to-prototype [1]
[1] https://groups.google.com/a/chromium.org/d/msgid/blink-dev/68d10a08.710a0220.5367c.009d.GAE%40google.com?utm_medium=email&utm_source=footer
- M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
- M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/external/wpt/css/css-backgrounds/parsing/background-position-x-computed-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-backgrounds/parsing/background-position-x-valid-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-backgrounds/parsing/background-position-y-computed-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-backgrounds/parsing/background-position-y-valid-expected.txt
- A third_party/blink/web_tests/wpt_internal/css/css-masking/parsing/webkit-mask-position-x-invalid.html
- A third_party/blink/web_tests/wpt_internal/css/css-masking/parsing/webkit-mask-position-x-valid-expected.txt
- A third_party/blink/web_tests/wpt_internal/css/css-masking/parsing/webkit-mask-position-x-valid.html
- A third_party/blink/web_tests/wpt_internal/css/css-masking/parsing/webkit-mask-position-y-invalid.html
- A third_party/blink/web_tests/wpt_internal/css/css-masking/parsing/webkit-mask-position-y-valid-expected.txt
- A third_party/blink/web_tests/wpt_internal/css/css-masking/parsing/webkit-mask-position-y-valid.html
Code-Review: +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
一丝 (Gerrit)
一丝 added 1 comment![]( "Open in Gerrit")
The CSSWG has agreed to add `mask-position-x` and `mask-position-y`, but no one has written the new specification yet. Firefox supports the unprefix two-value syntax.
https://github.com/w3c/fxtf-drafts/issues/103#issuecomment-360211489
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |