Bug 238125

Summary: [css-cascade] revert-layer ignores properties sharing a computed style
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, darin, ews-watchlist, koivisto, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/33772
Bug Depends on: 237487, 238126, 238350    
Bug Blocks: 236199    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Oriol Brufau
Reported 2022-03-20 08:24:32 PDT
WebKit has some properties that share a field in RenderStyle. When setting one of such properties in one cascade layer, and then setting the other property to 'revert-layer' in the next layer, we should get the former value. Example: <!DOCTYPE html> <style> @layer { body { box-shadow: 0 0 0 1000px red; } } @layer { body { -webkit-box-shadow: 0 0 0 1000px green; } } @layer { body { box-shadow: revert-layer; } } </style> It looks red, it should be green. Firefox and Chromium are green. 'revert' is also affected, but I don't think the affected properties are used in UA styles, so in practice it doesn't matter. This can be fixed with an approach similar to what I did in bug 236199 for logical properties. It's not that hard because these properties that share a computed style come in pairs. The only exception is for -webkit-border-image and the border-image-* longhands, which I plan to address in bug 237487.
Attachments
Patch (26.80 KB, patch)
2022-04-22 13:10 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (36.23 KB, patch)
2022-04-24 22:29 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (35.83 KB, patch)
2022-04-25 06:02 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (35.75 KB, patch)
2022-04-26 07:16 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-03-21 11:01:36 PDT
<rdar://problem/90577975>
Oriol Brufau
Comment 2 2022-04-20 11:07:44 PDT
After the refactoring from bug 238260 and its dependencies, I think it makes more sense to do this one before bug 236199.
Oriol Brufau
Comment 3 2022-04-22 13:10:25 PDT
Created attachment 458167 [details] Patch
EWS Watchlist
Comment 4 2022-04-22 13:12:20 PDT
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
Oriol Brufau
Comment 5 2022-04-24 22:29:22 PDT
Created attachment 458241 [details] Patch
Oriol Brufau
Comment 6 2022-04-25 06:02:31 PDT
Created attachment 458264 [details] Patch
Oriol Brufau
Comment 7 2022-04-25 06:23:32 PDT
PTAL. Maybe the test goes a bit overboard, but it exposes various bugs in WK and Blink so I think it's good.
Antti Koivisto
Comment 8 2022-04-26 01:35:53 PDT
Comment on attachment 458264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458264&action=review > Source/WebCore/style/PropertyCascade.cpp:157 > +const PropertyCascade::Property* PropertyCascade::lastDeferredPropertyForIdOrRelated(CSSPropertyID propertyID) const The name is hard to parse Maybe lastDeferredPropertyResolvingRelated() or something? > Source/WebCore/style/PropertyCascade.cpp:161 > + auto relatedID = getRelatedPropertyId(propertyID); > + auto indexForPropertyID = deferredPropertyIndex(propertyID); > + auto indexForRelatedID = deferredPropertyIndex(relatedID); This is bit hard to understand. getRelatedPropertyId() returns CSSProperyInvalid if there is no related property which would cause assert in deferredPropertyIndex(). This seems to rely on every deferred property having a related property. Is that just a coincidence? I thought those are separate concepts. Should CSSProperyInvalid case be handled?
Antti Koivisto
Comment 9 2022-04-26 02:28:46 PDT
Comment on attachment 458264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458264&action=review > LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-revert-layer.html:14 > +#nothing { > + accent-color: #123; > + align-content: baseline; > + align-items: baseline; > + align-self: baseline; > + align-tracks: baseline; Does this list need to be kept up to date when new properties are added? > LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-revert-layer.html:428 > +for (let property of cs) { > + if (!property.startsWith("-")) { > + initialValues[property] = cs.getPropertyValue(property); > + } > +} Do the results of this test need to be updated every time a new CSS property is added? That would be pretty annoying. If so, could that be avoided?
Oriol Brufau
Comment 10 2022-04-26 07:16:18 PDT
Created attachment 458359 [details] Patch
Oriol Brufau
Comment 11 2022-04-26 07:23:38 PDT
Comment on attachment 458264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458264&action=review >> Source/WebCore/style/PropertyCascade.cpp:157 >> +const PropertyCascade::Property* PropertyCascade::lastDeferredPropertyForIdOrRelated(CSSPropertyID propertyID) const > > The name is hard to parse > > Maybe lastDeferredPropertyResolvingRelated() or something? Done. >> Source/WebCore/style/PropertyCascade.cpp:161 >> + auto indexForRelatedID = deferredPropertyIndex(relatedID); > > This is bit hard to understand. getRelatedPropertyId() returns CSSProperyInvalid if there is no related property which would cause assert in deferredPropertyIndex(). This seems to rely on every deferred property having a related property. Is that just a coincidence? I thought those are separate concepts. > > Should CSSProperyInvalid case be handled? Yes, currently all deferred properties have a getRelatedPropertyId(). In bug 236199 I plan to make logical/physical properties deferred, but getRelatedPropertyId() will continue returning CSSProperyInvalid. They will be handled with CSSProperty::resolveDirectionAwareProperty() or CSSProperty::unresolvePhysicalProperty(). Anyways, I have added this check: if (relatedID == CSSPropertyInvalid) { ASSERT_NOT_REACHED(); return hasDeferredProperty(propertyID) ? &deferredProperty(propertyID) : nullptr; } >> LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-revert-layer.html:14 >> + align-tracks: baseline; > > Does this list need to be kept up to date when new properties are added? That was the idea. I have changed it to only test the properties which are specified in this stylesheet. >> LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-revert-layer.html:428 >> +} > > Do the results of this test need to be updated every time a new CSS property is added? That would be pretty annoying. If so, could that be avoided? Done.
EWS
Comment 12 2022-04-26 17:20:25 PDT
Committed r293485 (250019@main): <https://commits.webkit.org/250019@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458359 [details].
Note You need to log in before you can comment on or make changes to this bug.