Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibWeb/CSS: Implement background-blend-mode #3940

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skyz1
Copy link
Contributor

@skyz1 skyz1 commented Mar 14, 2025

This PR implements the background-blend-mode CSS property and fixes a regex in the WPT import script that caused the import of the corresponding test to fail.

Some of the blending modes in LibGFX do not exactly reflect the behavior of the other browsers. However, this issue is outside the scope of this PR, which focuses on applying the appropriate blending modes to the backgrounds.

This updates the regex for CSS URLs in the WPT import script to
correctly match URLs surrounded by single quotes.
@skyz1 skyz1 requested a review from AtkinsSJ as a code owner March 14, 2025 13:51
This implements the 'background-blend-mode' CSS property.
@skyz1 skyz1 force-pushed the background-blend-mode branch from b94908a to 3287183 Compare March 14, 2025 15:00
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, but a couple of things aren't needed because of how background data is stored.

@@ -327,6 +327,14 @@
"background-attachment"
]
},
"background-blend-mode": {
"animation-type": "none",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be "discrete"

@@ -452,6 +454,7 @@ class ComputedValues {
CSS::UserSelect user_select() const { return m_noninherited.user_select; }
CSS::Isolation isolation() const { return m_noninherited.isolation; }
CSS::Containment const& contain() const { return m_noninherited.contain; }
CSS::MixBlendMode background_blend_mode() const { return m_noninherited.background_blend_mode; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the blend mode is stored as part of the BackgroundLayerData, we shouldn't store it on its own in ComputedValues as well.

Comment on lines +1629 to +1633
MixBlendMode ComputedProperties::background_blend_mode() const
{
auto const& value = property(PropertyID::BackgroundBlendMode);
return keyword_to_mix_blend_mode(value.to_keyword()).release_value();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this is not needed as nobody will ask for a background-blend-mode on its own.

@@ -24,6 +24,7 @@ struct ResolvedBackgroundLayerData {
CSSPixelRect image_rect;
CSS::Repeat repeat_x;
CSS::Repeat repeat_y;
CSS::MixBlendMode mix_blend_mode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Just call this blend_mode. Looks like <blend-mode> is the type name used in the spec currently.

@@ -100,6 +100,7 @@ struct DrawRepeatedImmutableBitmap {
};

struct Save { };
struct SaveLayer { };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, adding the SaveLayer command would be in its own commit before this one.

Copy link

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

@github-actions github-actions bot added the conflicts Pull request has merge conflicts that need resolution label Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Pull request has merge conflicts that need resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants