[Webkit-unassigned] [Bug 111121] getComputedStyle not working with more than one background image.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111121

--- Comment #9 from Matt Woodrow <m_woodrow at apple.com> ---
Comment on attachment 445160
  --> https://bugs.webkit.org/attachment.cgi?id=445160
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211126/e467ee00/attachment-0001.htm>