getComputedStyle of background: url("dummy://test/1.png"), url("dummy://test/2.png"); is incorrect. Tested on Chrome 24.
Created attachment 190824 [details] testcase
What do you expect the computed style to return and what are you seeing instead?
Actual Result: rgba(0, 0, 0, 0) url(dummy://test/1.png), url(dummy://test/2.png) repeat, repeat scroll, scroll 0% 0%, 0% 0% / auto, auto padding-box, padding-box border-box, border-box I think expected result should be rgba(0, 0, 0, 0) url(dummy://test/1.png) repeat scroll 0% 0% / auto padding-box border-box, url(dummy://test/2.png) repeat scroll 0% 0% / auto padding-box border-box
Created attachment 445122 [details] Patch
Created attachment 445148 [details] Patch
Created attachment 445149 [details] Patch
Created attachment 445160 [details] Patch
Comment on attachment 445160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445160&action=review Patch is good but I'd like to see the next version. Some comments and questions below. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4451 > +size_t ComputedStyleExtractor::getLayerCount(CSSPropertyID property) > +{ You have the assertion about `property` in getBackgroundPropertyShorthandValue, but I'd repeat it in here too. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4452 > + auto* styledElement = this->styledElement(); Can drop the "this->". (Not sure why some other functions in this file do that.) > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4469 > +Ref<CSSValueList> ComputedStyleExtractor::getBackgroundPropertyShorthandValue(CSSPropertyID property, const StylePropertyShorthand& propertiesBeforeSlashSeperator, const StylePropertyShorthand& propertiesAfterSlashSeperator, CSSPropertyID lastLayerProperty) Maybe "getFillLayerPropertyShorthandValue" (after the naming of the FillLayer type)? I don't really think of "mask" as a being background-ish. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4475 > + auto bgColor = lastLayerProperty != CSSPropertyInvalid ? propertyValue(CSSPropertyBackgroundColor, DoNotUpdateLayout) : nullptr; Feels a bit weird to pass in a property for lastLayerProperty but use CSSPropertyBackgroundColor here. Maybe call bgColor "lastValue" and pass in lastLayerProperty to propertyValue()? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4488 > + // Background color is a singleton, and should only be prepended to the last layer. WebKit style is to avoid comments that describe what the code is doing, if it's reasonably clear what the code is doing by reading it. (And to make code clear enough that a comment isn't required, if possible.) So I would probably drop this comment. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4494 > + beforeList->append(layerCount == 1 ? value : *downcast<CSSValueList>(value).item(i)); When layerCount == 1, is value a CSSValueList or a single value? If it's a list, we should append item(i). > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4497 > + Do we need to append the slash in here? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4506 > + // If there was only a single layer, then don't bother wrapping in the > + // layers list. And I'd drop this one too. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4520 > + static const CSSPropertyID propertiesBeforeSlashSeperator[4] = { CSSPropertyBackgroundImage, CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment, CSSPropertyBackgroundPosition }; > static const CSSPropertyID propertiesAfterSlashSeperator[3] = { CSSPropertyBackgroundSize, CSSPropertyBackgroundOrigin, CSSPropertyBackgroundClip }; Do you know what other browsers do in terms of the ordering of the longhand components? The general rule should be to follow the grammar order in the spec, unless there is some other explicit rules about serialization, and this ordering here doesn't match what's in the spec. Also we can drop the explicit array lengths.
Comment on attachment 445160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445160&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4469 >> +Ref<CSSValueList> ComputedStyleExtractor::getBackgroundPropertyShorthandValue(CSSPropertyID property, const StylePropertyShorthand& propertiesBeforeSlashSeperator, const StylePropertyShorthand& propertiesAfterSlashSeperator, CSSPropertyID lastLayerProperty) > > Maybe "getFillLayerPropertyShorthandValue" (after the naming of the FillLayer type)? I don't really think of "mask" as a being background-ish. Yeah, that's probably a good idea. Some of the other mask code is using 'background' naming (in the parser etc), though that's probably just an artefact of how it was developed, rather than an explicit style choice. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4475 >> + auto bgColor = lastLayerProperty != CSSPropertyInvalid ? propertyValue(CSSPropertyBackgroundColor, DoNotUpdateLayout) : nullptr; > > Feels a bit weird to pass in a property for lastLayerProperty but use CSSPropertyBackgroundColor here. Maybe call bgColor "lastValue" and pass in lastLayerProperty to propertyValue()? Whoops, that indeed was the intent, just forgot to change it. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4494 >> + beforeList->append(layerCount == 1 ? value : *downcast<CSSValueList>(value).item(i)); > > When layerCount == 1, is value a CSSValueList or a single value? If it's a list, we should append item(i). When layerCount == 1 then it's just a single value (which could itself be a list, if the property itself is a list), if layerCount > 1, then it's a list with a value per-layer. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4497 >> + > > Do we need to append the slash in here? It's a slash-separated list, so just by having the two sublist, we get the '/' into the serialised output. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4520 >> static const CSSPropertyID propertiesAfterSlashSeperator[3] = { CSSPropertyBackgroundSize, CSSPropertyBackgroundOrigin, CSSPropertyBackgroundClip }; > > Do you know what other browsers do in terms of the ordering of the longhand components? The general rule should be to follow the grammar order in the spec, unless there is some other explicit rules about serialization, and this ordering here doesn't match what's in the spec. > > Also we can drop the explicit array lengths. This matches the order we were outputting previously, and matches what blink does. Gecko doesn't appear to make any attempt to produce a computed style for the background shorthand. I figured it'd be easier to just change the (obviously broken) behaviour for multi-layer backgrounds to match blink, and worry about the property ordering as a separate issue.
Created attachment 445165 [details] Patch
Comment on attachment 445165 [details] Patch r=me though it would be nice if some subtests checked for the correct interleaving of the longhand components when there is more than one layer present.
(In reply to Cameron McCormack (:heycam) from comment #11) > Comment on attachment 445165 [details] > Patch > > r=me though it would be nice if some subtests checked for the correct > interleaving of the longhand components when there is more than one layer > present. The new tests that I added should be checking multiple layers, and that all the components are in the expected order.
Committed r286200 (244582@main): <https://commits.webkit.org/244582@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445165 [details].
<rdar://problem/85790272>