[Gap Decorations]: Disentangle resolved -width values from -style [chromium/src : main]
Sam Davis Omekara (Gerrit)
Message from Sam Davis Omekara![]( "Open in Gerrit")
Set Ready For Review
Related details
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
This test documents all computed styles on a <div> element.@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 4 comments![]( "Open in Gerrit")
As a heads up it looks like there are still some test failures with this change
return "0px" != width_value->CssText() &&
"none" != style_value->CssText() &&
"hidden" != style_value->CssText();nit: although it was done this way previously, I'd swap the ordering of all the != such that "0px" etc are on the right side of the statements
bool is_legacy_column_rule_behavior =Should this name include something to do with the fact that this is related to visibility
This test documents all computed styles on a <div> element.@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Hmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Related details
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 4 comments![]( "Open in Gerrit")
As a heads up it looks like there are still some test failures with this change
Those test failures have to do with this comment thread: https://chromium-review.googlesource.com/c/chromium/src/+/6909751/comment/0af17042_7332c242/. So was waiting on resolution there to address them.
return "0px" != width_value->CssText() &&
"none" != style_value->CssText() &&
"hidden" != style_value->CssText();nit: although it was done this way previously, I'd swap the ordering of all the != such that "0px" etc are on the right side of the statements
Done
bool is_legacy_column_rule_behavior =Should this name include something to do with the fact that this is related to visibility
Hmm, I see your point but the more I think about, I realize we're not affecting the visibility. Even if column-rule-width is 5px if the -style value is none/hidden it will still not be visible, but the computed&&resolved value will be 5px and not 0px.
This test documents all computed styles on a <div> element.Alison Maher@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Hmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Is your question if the change is safe? It's currently behind a feature flag which is marked experimental. The other thing is that the follow up change tries to capture scenarios where this might be seen/user-facing. Should I change the order and land that first, we gather data before landing this?
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
2 comments
bool is_legacy_column_rule_behavior =Sam Davis OmekaraShould this name include something to do with the fact that this is related to visibility
Hmm, I see your point but the more I think about, I realize we're not affecting the visibility. Even if column-rule-width is 5px if the -style value is none/hidden it will still not be visible, but the computed&&resolved value will be 5px and not 0px.
Yeah I guess what you have is fine then, since it is explained what legacy behavior this is referring to in the comment beforehand
This test documents all computed styles on a <div> element.Alison Maher@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Sam Davis OmekaraHmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Is your question if the change is safe? It's currently behind a feature flag which is marked experimental. The other thing is that the follow up change tries to capture scenarios where this might be seen/user-facing. Should I change the order and land that first, we gather data before landing this?
More so if the resolution was safe, not the CL itself. But I keep forgetting that the only effect it could have is if the author is actually querying for the value? If there was an effect anytime this behavior changed, then it being the default value would make this very common, but if it only matters if the author actually queries for the value, then I think gathering the data and seeing what we get back makes sense. No need to swap the ordering.
And I think this is fine, but may be annoying for anyone adding to the main one, they will likely hit errors when running through the bots and have to update this too, but I can't really think of a better way myself
Related details
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
This test documents all computed styles on a <div> element.Alison Maher@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Sam Davis OmekaraHmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Alison MaherIs your question if the change is safe? It's currently behind a feature flag which is marked experimental. The other thing is that the follow up change tries to capture scenarios where this might be seen/user-facing. Should I change the order and land that first, we gather data before landing this?
More so if the resolution was safe, not the CL itself. But I keep forgetting that the only effect it could have is if the author is actually querying for the value? If there was an effect anytime this behavior changed, then it being the default value would make this very common, but if it only matters if the author actually queries for the value, then I think gathering the data and seeing what we get back makes sense. No need to swap the ordering.
And I think this is fine, but may be annoying for anyone adding to the main one, they will likely hit errors when running through the bots and have to update this too, but I can't really think of a better way myself
One more thing other than querying the value would be using "inherit". When we use inherit, we take the computed value from parent[1]. This is another scenario where the effect of this change will be seen. The following tests exemplifies this:
- third_party/blink/web_tests/external/wpt/css/css-ui/outline-009.html
- third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html
On the static test file, I think another option would be to mark these as failing for the virtual test suite with a comment as to why these are marked failing.
[1]: https://developer.mozilla.org/en-US/docs/Web/CSS/inherit
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
This test documents all computed styles on a <div> element.Alison Maher@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Hmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Re: effect of change - It only affects the value reported via gCS, not what gets rendered, which I think makes it safer. I do think we should add use counters to measure the impact though.
Re: large expectation files - Do we need to the "dump all values" tests in the virtual suite at all? It seems to me like the behavior differences are covered by css-multicol/parsing/column-rule-width-computed.html and css-ui/parsing/outline-width-computed.html.
Related details
- Alison Maher
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
This test documents all computed styles on a <div> element.Alison Maher@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Kevin BabbittHmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Re: effect of change - It only affects the value reported via gCS, not what gets rendered, which I think makes it safer. I do think we should add use counters to measure the impact though.
Re: large expectation files - Do we need to the "dump all values" tests in the virtual suite at all? It seems to me like the behavior differences are covered by css-multicol/parsing/column-rule-width-computed.html and css-ui/parsing/outline-width-computed.html.
- Use counter change is prepped as the followup change to this one.
- Right, I agree. I've taken those tests out of the Virtual test suite.
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted Code-Review+1![]( "Open in Gerrit")
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Davis Omekara (Gerrit)
Sam Davis Omekara added 1 comment![]( "Open in Gerrit")
- Kevin Babbitt
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
Related details
- Kevin Babbitt
- Sam Davis Omekara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
- Sam Davis Omekara
Sam Davis Omekara (Gerrit)
Sam Davis Omekara voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sam Davis Omekara
| Commit-Queue | +2 |
1 comment
This test documents all computed styles on a <div> element.@alm...@microsoft.com @kbab...@microsoft.com: The tricky part about this change is that it modifies the default values of the two properties. If I want to keep the previous behavior, I would need to generate a static test txt file expectation, but that could become outdated whenever new properties are added. I'm not sure how best to approach this. Another option is having the TestExpectations to expect this virtual test to fail instead of relying on a static txt file.
Kevin BabbittHmm if it modifies the default value, I would presume the effect of this change will be fairly large (i.e. if we were to gather data on it). Is this something we think is safe based on that fact?
Sam Davis OmekaraRe: effect of change - It only affects the value reported via gCS, not what gets rendered, which I think makes it safer. I do think we should add use counters to measure the impact though.
Re: large expectation files - Do we need to the "dump all values" tests in the virtual suite at all? It seems to me like the behavior differences are covered by css-multicol/parsing/column-rule-width-computed.html and css-ui/parsing/outline-width-computed.html.
- Use counter change is prepped as the followup change to this one.
- Right, I agree. I've taken those tests out of the Virtual test suite.
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/54881.
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
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Gap Decorations]: Disentangle resolved -width values from -style
This CL decouples the resolved value of outline-width and
column-rule-width from their corresponding *-style properties. The
second part of the CSSWG resolution[1], stipulates that the resolved
value for these properties should now be independent of the *-style.
Most file changes in this CL involve updating tests to reflect this new
behavior. To simplify reverting in the case of a web compat issue, a
virtual test suite with the flag disabled and relevant tests as base has
been set up.
A subsequent CL will be put up to set up use counters to evaluate
potential web compat risks associated with this change
[1]:
https://github.com/w3c/csswg-drafts/issues/11494#issuecomment-2675800489
- M third_party/blink/renderer/core/fragment_directive/css_selector_fragment_anchor_test.cc
- M third_party/blink/renderer/core/style/computed_style.h
- 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/css-gaps/parsing/gap-decorations-col-rule-width.html
- M third_party/blink/web_tests/external/wpt/css/css-multicol/animation/column-rule-width-interpolation.html
- M third_party/blink/web_tests/external/wpt/css/css-multicol/parsing/column-rule-computed.html
- M third_party/blink/web_tests/external/wpt/css/css-multicol/parsing/column-rule-width-computed.html
- A third_party/blink/web_tests/external/wpt/css/css-ui/outline-009-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-ui/outline-009.html
- M third_party/blink/web_tests/external/wpt/css/css-ui/parsing/canonical-order-outline-sub-properties-001-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-ui/parsing/canonical-order-outline-sub-properties-001.html
- M third_party/blink/web_tests/external/wpt/css/css-ui/parsing/outline-width-computed.html
- M third_party/blink/web_tests/external/wpt/css/cssom/cssom-getPropertyValue-common-checks-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/computed-style-listing-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/computed-style-without-renderer-listing-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/getComputedStyle-column-rule-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/getComputedStyle-column-rule.html
- M third_party/blink/web_tests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand-expected.txt
- M third_party/blink/web_tests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand.html
- M third_party/blink/web_tests/fast/replaced/resources/focus-test.js
- M third_party/blink/web_tests/svg/css/getComputedStyle-listing-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/external/wpt/css/css-multicol/parsing/column-rule-computed-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/external/wpt/css/css-multicol/parsing/column-rule-width-computed-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/external/wpt/css/css-ui/parsing/canonical-order-outline-sub-properties-001-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/external/wpt/css/css-ui/parsing/outline-width-computed-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/external/wpt/css/cssom/cssom-getPropertyValue-common-checks-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/fast/css/getComputedStyle/getComputedStyle-column-rule-expected.txt
- A third_party/blink/web_tests/virtual/computed-width-values-depend-on-style-values/fast/css/getComputedStyle/getComputedStyle-outline-shorthand-expected.txt
Code-Review: +1 by Kevin Babbitt, +1 by Philip Rogers, +1 by Alison Maher
| 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")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/54881
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |