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.
<rdar://problem/90577975>
After the refactoring from bug 238260 and its dependencies, I think it makes more sense to do this one before bug 236199.
Created attachment 458167 [details] Patch
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
Created attachment 458241 [details] Patch
Created attachment 458264 [details] Patch
PTAL. Maybe the test goes a bit overboard, but it exposes various bugs in WK and Blink so I think it's good.
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?
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?
Created attachment 458359 [details] Patch
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.
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].