Bug 111121 – getComputedStyle not working with more than one background image.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Uday Kiran]

getComputedStyle of background: url("dummy://test/1.png"), url("dummy://test/2.png"); is incorrect.
Tested on Chrome 24.

[Uday Kiran]

Created attachment 190824 [details]
testcase

[Emil A Eklund]

What do you expect the computed style to return and what are you seeing instead?

[Uday Kiran]

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

[Matt Woodrow]

Created attachment 445122 [details]
Patch

[Matt Woodrow]

Created attachment 445148 [details]
Patch

[Matt Woodrow]

Created attachment 445149 [details]
Patch

[Matt Woodrow]

Created attachment 445160 [details]
Patch

[Cameron McCormack (:heycam)]

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.

[Matt Woodrow]

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.

[Matt Woodrow]

Created attachment 445165 [details]
Patch

[Cameron McCormack (:heycam)]

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.

[Matt Woodrow]

(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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85790272>