Bug 261552 - getComputedStyle(elt).backgroundBlendMode returns only the first property of a list (background-blend-mode)
Summary: getComputedStyle(elt).backgroundBlendMode returns only the first property of ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 17
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://wpt.live/css/compositing/parsi...
Keywords: BrowserCompat, InRadar, WPTImpact
Depends on: 263104
Blocks:
  Show dependency treegraph
 
Reported: 2023-09-14 02:28 PDT by Karl Dubost
Modified: 2023-10-12 19:45 PDT (History)
6 users (show)

See Also:


Attachments
testcase (1.11 KB, text/html)
2023-10-12 01:09 PDT, Karl Dubost
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Karl Dubost 2023-09-14 02:28:52 PDT
see http://wpt.live/css/compositing/parsing/background-blend-mode-computed.html

when "normal, luminosity" is specified it returns only "normal"
while the "normal, luminosity" values are considered supported.

Firefox passes the test.
http://wpt.fyi/css/compositing/parsing/background-blend-mode-computed.html
Comment 1 Radar WebKit Bug Importer 2023-09-21 02:29:13 PDT
<rdar://problem/115832043>
Comment 2 Karl Dubost 2023-10-11 00:38:31 PDT
https://searchfox.org/wubkat/rev/e21786e1635702a6ca7c1e30290f32e8a97fec23/Source/WebCore/css/ComputedStyleExtractor.cpp#4101-4109

    case CSSPropertyBackgroundBlendMode: {
        auto& layers = style.backgroundLayers();
        if (!layers.next())
            return createConvertingToCSSValueID(layers.blendMode());
        CSSValueListBuilder list;
        for (auto* currLayer = &layers; currLayer; currLayer = currLayer->next())
            list.append(createConvertingToCSSValueID(currLayer->blendMode()));
        return CSSValueList::createSpaceSeparated(WTFMove(list));
    }
Comment 3 Karl Dubost 2023-10-11 00:55:50 PDT
I do not have much knowledge on this code, but the `getComputedStyle()` seems to take into account a list of values `CSSValueList::createSpaceSeparated(WTFMove(list));`. 

So maybe this is related to the computation of `auto& layers = style.backgroundLayers();`

Probably Antti is a better person for understanding what is the source of the issue.
Once understood Maybe it will be easy to fix.

oh wait, is it because it should be comma instead of space.

`CSSValueList::createCommaSeparated(WTFMove(list));`.
instead of:
`CSSValueList::createSpaceSeparated(WTFMove(list));`.


```
> target.style.backgroundBlendMode
< "screen, overlay"
> getComputedStyle(target)['background-blend-mode']
< "screen"
```
Comment 4 Karl Dubost 2023-10-11 01:18:05 PDT
That's not it. 

* Not because of layers
* Not because of Comma vs Space
Comment 5 Karl Dubost 2023-10-11 01:33:53 PDT
Maybe layers, only 1?
Comment 6 Simon Fraser (smfr) 2023-10-11 14:10:06 PDT
I think it is because it should be createCommaSeparated(). See the similar code for case CSSPropertyBackgroundImage:
Comment 7 Karl Dubost 2023-10-11 14:54:45 PDT
yes, I quickly tried to replace by createCommaSeparated, but it didn't fix it. 

So I added different WTFLogAlways() at a couple of places.
It never goes in that part part of the code with CSSValueList::createCommaSeparated(WTFMove(list))

So there must be something else too. 

I will try again to make double sure.
Comment 8 Karl Dubost 2023-10-11 17:04:58 PDT
Simon, 
I tried again by just changing to make sure. And it's not only the createCommaSeparated/createSpaceSeparated

diff --git a/Source/WebCore/css/ComputedStyleExtractor.cpp b/Source/WebCore/css/ComputedStyleExtractor.cpp
index b3c66f42e0f0..0b652382f480 100644
--- a/Source/WebCore/css/ComputedStyleExtractor.cpp
+++ b/Source/WebCore/css/ComputedStyleExtractor.cpp
@@ -4099,13 +4099,19 @@ RefPtr<CSSValue> ComputedStyleExtractor::valueForPropertyInStyle(const RenderSty
         return createConvertingToCSSValueID(style.isolation());
 #endif
     case CSSPropertyBackgroundBlendMode: {
+        WTFLogAlways("Inside CSSPropertyBackgroundBlendMode");
         auto& layers = style.backgroundLayers();
-        if (!layers.next())
+        if (!layers.next()) {
+            WTFLogAlways("Early return createConvertingToCSSValueID(layers.blendMode())");
             return createConvertingToCSSValueID(layers.blendMode());
+        }
         CSSValueListBuilder list;
         for (auto* currLayer = &layers; currLayer; currLayer = currLayer->next())
             list.append(createConvertingToCSSValueID(currLayer->blendMode()));
-        return CSSValueList::createSpaceSeparated(WTFMove(list));
+        // we want a comma separated list,
+        // but are we reaching this line of code?
+        WTFLogAlways("final return CSSValueList::createCommaSeparated(WTFMove(list))");
+        return CSSValueList::createCommaSeparated(WTFMove(list));
     }
     case CSSPropertyBackground:
         return getBackgroundShorthandValue();


and loading:
http://wpt.live/css/compositing/parsing/background-blend-mode-computed.html

it never goes to the last return for the list of values.


% run-minibrowser debug       
Starting MiniBrowser with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Users/karlcow/code/OpenSource/WebKitBuild/Debug.
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Inside CSSPropertyBackgroundBlendMode
Early return createConvertingToCSSValueID(layers.blendMode())
Comment 9 Simon Fraser (smfr) 2023-10-11 17:21:09 PDT
This depends on whether the background-image property has > 1 image, since, in webkit, that determines the length of the FillLayers list.
Comment 10 Karl Dubost 2023-10-11 18:14:01 PDT
So in Bug 263051, the createCommaSeparated() list is being fixed.

But that doesn't solve this issue as explained in the comments above. 

There could be a test bug, but it's not totally sure either. 
http://wpt.live/css/compositing/parsing/background-blend-mode-computed.html

Simon is saying:

> there should be a FillLayer per background value; the length of the list is determined by background-image/mask-image


but we should double check the spec first to see how this is supposed to work. 
https://drafts.fxtf.org/compositing-1/#background-blend-mode

> Defines the blending mode of each background layer.
> 
> Each background layer must blend with the element’s background layer 
> that is below it and the element’s background color. Background layers 
> must not blend with the content that is behind the element, instead 
> they must act as if they are rendered into an isolated group.

and


> The background-blend-mode list must be applied in the same order 
> as background-image [CSS3BG]. This means that the first element 
> in the list will apply to the layer that is on top. If a property 
> doesn’t have enough comma-separated values to match the number 
> of layers, the UA must calculate its used value by repeating the 
> list of values until there are enough.


The layering seems to be defined in 
https://drafts.csswg.org/css-backgrounds-3/#layering

It says:

> Each of the images is sized, positioned, and tiled according 
> to the corresponding value in the other background properties. 

Then 

> The lists are matched up from the first value: excess values 
> at the end are not used. 

Then 

> If a property doesn’t have enough comma-separated values 
> to match the number of layers, the UA must calculate its used value 
> by repeating the list of values until there are enough.


I need to read this carefully again. 

The example in this section 

background-image: url(flower.png), url(ball.png), url(grass.png);
background-position: center center, 20% 80%, top left, bottom right;
background-origin: border-box, content-box;
background-repeat: no-repeat;

has exactly the same effect as this set with the extra position dropped and the missing values for background-origin and background-repeat filled in (emphasized for clarity):

background-image: url(flower.png), url(ball.png), url(grass.png);
background-position: center center, 20% 80%, top left;
background-origin: border-box, content-box, border-box;
background-repeat: no-repeat, no-repeat, no-repeat;


And we can see that for background-position where there is 4 values list with an extra "bottom right", it drops it to make it a 3 values list.

So from this I would say the test as currently defined on WPT is wrong and probably the implementation of Gecko too.
Comment 11 Karl Dubost 2023-10-11 19:41:57 PDT
Note tests will be fixed/extended in 
https://github.com/web-platform-tests/wpt/issues/42496
Comment 12 Karl Dubost 2023-10-11 22:47:36 PDT
I opened a bug on Mozilla bugzilla too. 
https://bugzilla.mozilla.org/show_bug.cgi?id=1858591
Comment 13 Karl Dubost 2023-10-12 00:35:39 PDT
I was too hasty here. 

See the discussions on the tests fix!
https://github.com/web-platform-tests/wpt/issues/42496#issuecomment-1759017241

The latest discussions in the CSS WG
https://github.com/w3c/csswg-drafts/issues/7164#issuecomment-1201340918

led fantasai to clarify the way this is working
https://drafts.csswg.org/css-values-4/#linked-properties


Specifically this sentence:

> The computed values of the coordinating list properties are not affected by such truncation or repetition.

which means getComputedStyle() should return the same list than what was specified. 

(even if the used values have been truncated or repeated.)

So there is something which needs to be fixed in WebKit (and Blink).
Comment 14 Karl Dubost 2023-10-12 01:09:31 PDT
Created attachment 468185 [details]
testcase

it has a bigger impact than just backgroundBlendMode.
We might want to rename this bug. 

getComputedStyle(elt).* should return the specified values for coordinating list properties.

.test {
    background-image: url(foo.ping);
    background-blend-mode: normal, luminosity;
    background-position: 0px 0px, 10px 10px;
}

let test = document.querySelector('.test');
window.getComputedStyle(test).backgroundBlendMode;
window.getComputedStyle(test).backgroundPosition;

should return (like Gecko)

normal, luminosity
0px 0px, 10px 10px

currently WebKit and Blink return

normal
0px 0px
Comment 15 Karl Dubost 2023-10-12 19:45:41 PDT
Created Bug 263104