Fix getPropertyValue() and setProperty() behavior for 'all' [chromium/src : main]
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
}Is this the only place where we should handle this?
Did you consider doing this in SetLonghandProperty()?
Related details
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Is this the only place where we should handle this?
Did you consider doing this in SetLonghandProperty()?
I did consider putting this in SetLonghandProperty(). However, this function is called from both the declarative path (cssText) and the imperative path (setProperty).
If it were placed in SetLonghandProperty(), RemovePropertiesAffectedByAll() would run during the declarative path as well (like in patchset 8), which breaks the InlineAll test in CascadeExpansionTest. In the declarative path, all three entries in "left:1px; all:unset; right:1px" need to be preserved for cascade expansion to work correctly.
That's why patchset 7 called RemovePropertiesAffectedByAll() in CSSParser::ParseValue (imperative-only path) to remove existing longhands when setting all.
But after thinking about your comment more, I think rather than deleting longhands with RemovePropertiesAffectedByAll() when setProperty() is called, it would be better to move the all entry to the end of the property vector and determine priority between all and longhands at read time. This way both declarative and imperative paths maintain consistent behavior. StylePropertySerializer[1] already has a similar pattern, so I think I can leverage that.
I'll try this approach and ping you again. Thanks!
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Message from Suyeon Ji![]( "Open in Gerrit")
- Rune Lillesveen
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Hi, I've uploaded a new patchset.
Now, when SetLonghandProperty() is called, instead of removing properties, it moves the target property to the end of the set. This applies whether we are setting all itself or a longhand affected by all. Similar to StylePropertySerializer, this allows us to satisfy the spec by comparing indices. Could you take another look?
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinho Bang (Gerrit)
Jinho Bang voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
Fix getPropertyValue() and setProperty() behavior for 'all'
According to the CSS spec[1], the 'all' property is a shorthand.
Shorthands, especially 'all', can reset all CSS properties (except 'direction', 'unicode-bidi', custom properties) to a given value.
However, Chromium currently does not follow the spec.
First, setProperty('all') does not reset all properties. So when
querying properties affected by 'all', the correct value is not
returned.
Second, for a shorthand, getPropertyValue()[2] should return the
serialized value of its longhands, but currently 'all' does not
do this.
This patch fixes the behavior to match the specification.
[1]: https://drafts.csswg.org/css-cascade/#all-shorthand
[2]: https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue
- M third_party/blink/renderer/core/css/css_property_value_set.cc
- D third_party/blink/web_tests/external/wpt/css/cssom/cssstyledeclaration-all-shorthand-expected.txt
Code-Review: +1 by Rune Lillesveen, +1 by Jinho Bang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinho Bang (Gerrit)
Jinho Bang has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinho Bang (Gerrit)
Jinho Bang added 1 comment![]( "Open in Gerrit")
if (CSSProperty::Get(property).IsAffectedByAll()) {I didn't realize this, but IsAffectedByAll() will return true in most cases. The intended behavior is probably to only consider this when the `all` property actually exists in the property set, but as it stands, this logic will run in most cases. I'm not sure but I think we need something like a bit flag to ensure this only applies when the `all` property is present.
Related details
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job linux-r350-perf/blink_perf.css failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10fd0768090000
Related details
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 2 comments![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); ++i) {There is still an issue with the Get operation, as it requires iteration.
Given `left: 10px; all: revert; right: 20px`
vector: [all: revert, right: 20px]
the expected value for getPropertyValue("all") should be an empty string.
To satisfy this, we need to determine if any properties affected by all were set after it during serialization. While comparing the index of all with the vector's size() might seem simple, it’s difficult to filter out properties not affected by all. Therefore, iterating through the loop is necessary.
if constexpr (std::is_same_v<T, CSSPropertyID>) {Since all is a shorthand that resets almost all properties, calling setProperty("all", all_value) means that any subsequent getPropertyValue(property) for an affected property should return that all_value. Consequently,this logic must include a process to check the stored value of all.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
ASSERT_EQ(all.size() + 1, e.size());I initially thought this was a regression, but after further review, I found that duplicates are already being handled during cascade expansion. Since it's safe for them to be removed beforehand, I’ve updated the test accordingly.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji has uploaded the change for review![]( "Open in Gerrit")
Suyeon Ji removed AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Menard, Alexis, blink-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org and blink-revie...@chromium.org from reviewers of this change.
Commit message
[For review] Fix getPropertyValue() and setProperty() behavior for 'all'
According to the CSS spec[1], the 'all' property is a shorthand.
Shorthands, especially 'all', can reset all CSS properties (except 'direction', 'unicode-bidi', custom properties) to a given value.
However, Chromium currently does not follow the spec.
First, setProperty('all') does not reset all properties. So when
querying properties affected by 'all', the correct value is not
returned.
Second, for a shorthand, getPropertyValue()[2] should return the
serialized value of its longhands, but currently 'all' does not
do this.
This patch fixes the behavior to match the specification.
[1]: https://drafts.csswg.org/css-cascade/#all-shorthand
[2]: https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue
Change diff
Change information
- M third_party/blink/renderer/core/css/css_property_value_set.cc
- M third_party/blink/renderer/core/css/resolver/cascade_expansion_test.cc
- D third_party/blink/web_tests/external/wpt/css/cssom/cssstyledeclaration-all-shorthand-expected.txt
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); ++i) {There is still an issue with the Get operation, as it requires iteration.
Given left: 10px; all: revert; right: 20px
vector: [all: revert, right: 20px]
the expected value for getPropertyValue("all") should be an empty string.
To satisfy this, we need to determine if any properties affected by all were set after it during serialization. While comparing the index of all with the vector's size() might seem simple, it’s difficult to filter out properties not affected by all. Therefore, iterating through the loop is necessary.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |