LibWeb/CSS: Implement background-blend-mode CSS property #3157
Conversation
|
Hello! One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the |
vukanovics
force-pushed
the
background-blend-mode
branch
from
ad5eac6 to
80212fe
Compare
|
Looking further into the specification, this paragraph also is currently incorrectly implemented: "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." |
There was a problem hiding this comment.
Welcome! This is nice work. :^)
I think I've answered the two questions you had. As for "Background layers must not blend with the content that is behind the element", you can try fixing that if you like, but putting a // FIXME comment about it somewhere appropriate is fine too.
As for the commit, this feels like at least 2 separate things: Adding blend-mode support to the painting code, and then adding the CSS background-blend-mode property. If you can split that into two commits it'd be good.
Also, it would be nice to have a test for this! There are some WPT tests linked from the spec which you can import with the script described here.
| "darken", | ||
| "multiply", | ||
| "color-burn", | ||
| "lighten", | ||
| "screen", | ||
| "color-dodge", | ||
| "overlay", | ||
| "soft-light", | ||
| "hard-light", | ||
| "difference", | ||
| "exclusion", | ||
| "hue", | ||
| "saturation", | ||
| "color", | ||
| "luminosity" |
There was a problem hiding this comment.
As you asked: This whole file should be alphabetically sorted.
| case PropertyID::BackgroundBlendMode: | ||
| case PropertyID::BackgroundAttachment: |
There was a problem hiding this comment.
This should be alphabetical too.
| @@ -79,6 +79,24 @@ | |||
| "local", | |||
| "scroll" | |||
| ], | |||
| "blend-mode": [ | |||
There was a problem hiding this comment.
This should go after background-box.
| auto const num_blend_modes = blend_modes.is_value_list() ? blend_modes.as_value_list().size() : 1; | ||
|
|
||
| if (layer_index >= num_blend_modes) { | ||
| layer.blend_mode = CSS::BlendMode::Normal; | ||
| } else { |
There was a problem hiding this comment.
Re your 2nd question: These lines aren't necessary. value_for_layer() already deals with the background layers and repeating the list if there aren't enough, so you can just use blend_mode_value directly.
If that's not working, let me know and I'll try and figure out what's wrong.
| switch (blend_mode_value->to_keyword()) { | ||
| case CSS::Keyword::Normal: | ||
| layer.blend_mode = CSS::BlendMode::Normal; | ||
| break; |
There was a problem hiding this comment.
Adding an enum to Enums.json automatically generates a function that converts this for you. The whole switch can be replaced with:
layer.blend_mode = CSS::keyword_to_blend_mode(blend_mode_value->to_keyword()).value_or(CSS::BlendMode::Normal);It returns Optional, so the value_or() is equivalent to having the default: branch in the switch.
Hi friends!
This patch implements the background-blend-mode CSS property:
Addresses a part of #3140:
There two immediate problems this patch has:
Unfortunately, this code doesn't handle the lack of a normal blend mode layer under non-normal layers properly.
I'm looking into this, but I'd appreciate help :)