Ignore getComputedStyle pseudo-element arguments without colons per CSSOM spec [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt added 1 comment![]( "Open in Gerrit")
Since this is something that works one way today and we're proposing to make it work differently to match the spec, I am concerned about interop and compat risks. I think the change should at least be guarded with a feature flag that we can turn off if needed. I'd also consider going through the Blink launch process with an Intent to Ship.
For interop: What's the behavior in other engines?
For compat: It appears the use counter hasn't landed yet. Do we have some other data indicating the extent to which sites are relying on the current behavior?
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Since this is something that works one way today and we're proposing to make it work differently to match the spec, I am concerned about interop and compat risks. I think the change should at least be guarded with a feature flag that we can turn off if needed. I'd also consider going through the Blink launch process with an Intent to Ship.
For interop: What's the behavior in other engines?
For compat: It appears the use counter hasn't landed yet. Do we have some other data indicating the extent to which sites are relying on the current behavior?
Hi Kevin, thanks for the review!
This is mainly a catch-up to match other engines. The relevant WPT is:
https://wpt.fyi/results/css/cssom/getComputedStyle-pseudo.html?label=experimental&label=master&aligned
The top failure with `!EQ("100px", "50px")` - this change should turn it green, matching Firefox and WebKit which already pass.
There's also an existing bug tracking this: https://issues.chromium.org/issues/41451306 (will add to commit message).
The other test changes are internal tests that needed updating for the new behavior.
This CL is part of a series with the goal of turning all these pseudo-element WPTs green. Originally it was all-in-one here: https://chromium-review.googlesource.com/c/chromium/src/+/7323064/4 but we split it during review.
**On feature flags:** Should I create a single umbrella flag (e.g., `CSSOMPseudoElementSpec` or `CSSOMGetComputedStylePseudoFixes`) to cover all the related CLs, or one flag per CL?
**On Intent to Ship:** Same question - a single I2S for the overall spec compliance work, or one per CL?
Happy to do it either way, just want to know your preference.
Related details
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaSince this is something that works one way today and we're proposing to make it work differently to match the spec, I am concerned about interop and compat risks. I think the change should at least be guarded with a feature flag that we can turn off if needed. I'd also consider going through the Blink launch process with an Intent to Ship.
For interop: What's the behavior in other engines?
For compat: It appears the use counter hasn't landed yet. Do we have some other data indicating the extent to which sites are relying on the current behavior?
Hi Kevin, thanks for the review!
This is mainly a catch-up to match other engines. The relevant WPT is:
https://wpt.fyi/results/css/cssom/getComputedStyle-pseudo.html?label=experimental&label=master&alignedThe top failure with `!EQ("100px", "50px")` - this change should turn it green, matching Firefox and WebKit which already pass.
There's also an existing bug tracking this: https://issues.chromium.org/issues/41451306 (will add to commit message).
The other test changes are internal tests that needed updating for the new behavior.
This CL is part of a series with the goal of turning all these pseudo-element WPTs green. Originally it was all-in-one here: https://chromium-review.googlesource.com/c/chromium/src/+/7323064/4 but we split it during review.
**On feature flags:** Should I create a single umbrella flag (e.g., `CSSOMPseudoElementSpec` or `CSSOMGetComputedStylePseudoFixes`) to cover all the related CLs, or one flag per CL?
**On Intent to Ship:** Same question - a single I2S for the overall spec compliance work, or one per CL?
Happy to do it either way, just want to know your preference.
Thanks for the additional context.
Generally speaking, I would have one feature flag per potentially breaking web-facing change. I think this change deserves its own flag. The other two web-facing changes, as I understand, are fixing things that weren't working at all before - assuming I have that right, I think they're safe to land without flags. (But if there were breakage risk associated with either of them, I would give those separate flags.)
Based on Anders's comments in the all-in-one CL, I would suggest landing the use counter first and letting it run in Stable channel for a month or two so that we have data on the compat risk before proceeding with this CL.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaSince this is something that works one way today and we're proposing to make it work differently to match the spec, I am concerned about interop and compat risks. I think the change should at least be guarded with a feature flag that we can turn off if needed. I'd also consider going through the Blink launch process with an Intent to Ship.
For interop: What's the behavior in other engines?
For compat: It appears the use counter hasn't landed yet. Do we have some other data indicating the extent to which sites are relying on the current behavior?
Kevin BabbittHi Kevin, thanks for the review!
This is mainly a catch-up to match other engines. The relevant WPT is:
https://wpt.fyi/results/css/cssom/getComputedStyle-pseudo.html?label=experimental&label=master&alignedThe top failure with `!EQ("100px", "50px")` - this change should turn it green, matching Firefox and WebKit which already pass.
There's also an existing bug tracking this: https://issues.chromium.org/issues/41451306 (will add to commit message).
The other test changes are internal tests that needed updating for the new behavior.
This CL is part of a series with the goal of turning all these pseudo-element WPTs green. Originally it was all-in-one here: https://chromium-review.googlesource.com/c/chromium/src/+/7323064/4 but we split it during review.
**On feature flags:** Should I create a single umbrella flag (e.g., `CSSOMPseudoElementSpec` or `CSSOMGetComputedStylePseudoFixes`) to cover all the related CLs, or one flag per CL?
**On Intent to Ship:** Same question - a single I2S for the overall spec compliance work, or one per CL?
Happy to do it either way, just want to know your preference.
Thanks for the additional context.
Generally speaking, I would have one feature flag per potentially breaking web-facing change. I think this change deserves its own flag. The other two web-facing changes, as I understand, are fixing things that weren't working at all before - assuming I have that right, I think they're safe to land without flags. (But if there were breakage risk associated with either of them, I would give those separate flags.)
Based on Anders's comments in the all-in-one CL, I would suggest landing the use counter first and letting it run in Stable channel for a month or two so that we have data on the compat risk before proceeding with this CL.
hi kbab...@microsoft.com, thank you, it makes sense and i fully aggree. the counter CL landed. if my understanding is right, it should at some point show up there: https://chromestatus.com/metrics/feature/popularity#GetComputedStylePseudoElementWithoutColon
once the commit is in stable?
https://chromiumdash.appspot.com/commit/dc4c09bb510ec4e8c5c27653577da677f4f01a5a
i'll wait till there is data, or a month after stable, and will continue working on the CL (trying to keep it in a mergable state in the meantime)
links are mainly for my future self!
we can then decide if that requires a I2S ?
Related details
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt added 1 comment![]( "Open in Gerrit")
Another option might be to land this CL now with the feature flag set to status=experimental and plan to flip it to stable once we have more data. That way you wouldn't need to float the patch, and we might get additional signal from folks running with Experimental Web Platform Features enabled on about:flags.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Done. Added a RuntimeEnabledFeature entry with status=experimental and switched the check to RuntimeEnabledFeatures
Related details
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
2 comments
return pseudo_id;Do we need this? Or can we just fall through to the `return pseudo_id` below?
shouldBeEqualToString("computedStyleFor('content', ':before', 'content')", "\"text\"");Can we update to double colons in this file for consistency with the others?
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
Do we need this? Or can we just fall through to the `return pseudo_id` below?
no, done!
shouldBeEqualToString("computedStyleFor('content', ':before', 'content')", "\"text\"");Can we update to double colons in this file for consistency with the others?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 1 comment![]( "Open in Gerrit")
I think Anders is better to review this than me.
Related details
- Anders Hartvoll Ruud
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted Code-Review+1![]( "Open in Gerrit")
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@kbab...@microsoft.com with the current setup and runtime feature flag, just wanted to make sure, is it safe to land from your perspective?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
1 comment
@kbab...@microsoft.com with the current setup and runtime feature flag, just wanted to make sure, is it safe to land from your perspective?
The code looks fine to me.
But something I just thought of, sorry for not realizing this sooner: we should be testing both the codepath with the flag enabled (which the CL currently reflects) and the codepath with the flag disabled (which users will see unless they have Experimental Web Platform Features enabled). The way to do this is to have a virtual test suite for just the handful of tests whose behavior diverges based on the flag.
References:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VirtualTestSuites
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_tests.md#Virtual-test-suites
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Kevin Babbitt@kbab...@microsoft.com with the current setup and runtime feature flag, just wanted to make sure, is it safe to land from your perspective?
The code looks fine to me.
But something I just thought of, sorry for not realizing this sooner: we should be testing both the codepath with the flag enabled (which the CL currently reflects) and the codepath with the flag disabled (which users will see unless they have Experimental Web Platform Features enabled). The way to do this is to have a virtual test suite for just the handful of tests whose behavior diverges based on the flag.
References:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VirtualTestSuites
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_tests.md#Virtual-test-suites
- Anders Hartvoll Ruud
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
1 comment
Related details
- Helmut Januschka
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Ignore getComputedStyle pseudo-element arguments without colons per CSSOM spec
Per CSSOM spec, getComputedStyle(element, pseudoElt) should ignore the
pseudo-element argument if it doesn't start with a colon. This means
getComputedStyle(el, "before") should return the element's style, not
the ::before pseudo-element's style.
This is a behavior change that may affect existing websites. The compat
risk is measured by the UseCounter added in the parent CL.
Updated Blink-internal tests to use the correct CSSOM syntax (":before"
instead of "before").
- M third_party/blink/renderer/core/css/parser/css_selector_parser.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/VirtualTestSuites
- M third_party/blink/web_tests/external/wpt/css/cssom/getComputedStyle-pseudo-expected.txt
- M third_party/blink/web_tests/fast/css/content-property-quote-types.html
- M third_party/blink/web_tests/fast/css/first-letter-property.html
- M third_party/blink/web_tests/fast/css/getComputedStyle/computed-style-properties-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/computed-style-properties.html
- M third_party/blink/web_tests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html
- M third_party/blink/web_tests/images/invalid-image-url-crash.html
- A third_party/blink/web_tests/virtual/cssom-getcomputedstyle-pseudo-no-colon-disabled/README.md
- A third_party/blink/web_tests/virtual/cssom-getcomputedstyle-pseudo-no-colon-disabled/external/wpt/css/cssom/getComputedStyle-pseudo-expected.txt
- A third_party/blink/web_tests/virtual/cssom-getcomputedstyle-pseudo-no-colon-disabled/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element-expected.txt
Code-Review: +1 by Anders Hartvoll Ruud, +1 by Kevin Babbitt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |