Closed Bug 841601 Opened 11 years ago Closed 11 years ago

Add support for background-blend-mode

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: cabanier, Assigned: olaru)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(4 files, 26 obsolete files)

8.89 KB, patch
Details | Diff | Splinter Review
7.35 KB, patch
Details | Diff | Splinter Review
242.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
41.66 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.56 Safari/537.17

Steps to reproduce:

There is currently no support for background-blend-mode on mozilla: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#background-blend-mode
This patch will add support for CSS parsing and storing of the keyword.
Subsequent patches will implement the actual rendering of blended background images
Component: Untriaged → DOM
Product: Firefox → Core
Attached patch First try (obsolete) — Splinter Review
Attached patch Fixed strange diff (obsolete) — Splinter Review
Attachment #714166 - Attachment is obsolete: true
Attachment #714185 - Flags: feedback?(dbaron)
I'm glad to see you posting Gecko patches.  But a few things:

 (1) patches to add new CSS properties should always have appropriate additions to layout/style/test/property_database.js, and you should make the mochitests in layout/style/test pass with the additions.  ("TEST_PATH=layout/style make mochitest-plain" in the objdir.)  If I don't see those changes, I won't even look at the rest of the patch, since it's likely to have errors if it hasn't been run through appropriate tests.  (I'm happy to answer specific questions about what would cause a particular failure.)

 (2) Please post diffs using the diff settings in https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration

 (3) I don't think we want to implement this with a prefix; it should be implemented without a prefix, with the default value of that pref set with an #ifdef RELEASE_BUILD (on for nightly and aurora, off for beta and release) in modules/libpref/src/init/all.js

 (4) Given roc's response to the proposal of this property, e.g., in http://lists.w3.org/Archives/Public/public-fx/2013JanMar/0028.html , it's not clear to me that we want to implement this property.  I think it's probably worth getting to a decision on that before putting more energy into implementing it.
Status: UNCONFIRMED → NEW
Component: DOM → Layout: View Rendering
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Add support for -moz-background-blend-mode → Add support for background-blend-mode
Comment on attachment 714185 [details] [diff] [review]
Fixed strange diff

see comment 3
Attachment #714185 - Flags: feedback?(dbaron) → feedback-
(In reply to David Baron [:dbaron] from comment #3)
Thanks for the review
1) I will do so. I know tests are needed and I will probably port over the ones I wrote for webkit.
Yes, I would like to know what failures you have in mind.
2) I followed this: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
but must have missed a config. I will update
3) I implemented it with a prefix in webkit. I'm a little worried about differences in how  and when stacking contexts are generated between the browsers which will result in rendering differences.
4) Unfortunately that email didn't spark a large conversation. I believe this is a light-weight feature to some people will really like to use for decorative purposes. Its implementation is much easier than full-on blending of elements and but still fairly powerful. I also implemented it in WebKit.
(In reply to Rik Cabanier from comment #5)
> Yes, I would like to know what failures you have in mind.

Nothing in mind, though the patch you posted pretty clearly doesn't compile.

> 3) I implemented it with a prefix in webkit. I'm a little worried about
> differences in how  and when stacking contexts are generated between the
> browsers which will result in rendering differences.

I think that's an argument against having a property, or at the very least an argument countering your argument that it's a "light-weight feature".

> 4) Unfortunately that email didn't spark a large conversation. I believe
> this is a light-weight feature to some people will really like to use for
> decorative purposes. Its implementation is much easier than full-on blending
> of elements and but still fairly powerful. I also implemented it in WebKit.

Not sparking a large conversation doesn't make an issue go away.
(In reply to David Baron [:dbaron] from comment #6)
> Nothing in mind, though the patch you posted
> pretty clearly doesn't compile.
It was compiling on my machine. I guess something went wrong when creating the patch...

> 3) I implemented it with a prefix in
> webkit. I'm a little worried about
> differences in how  and when stacking
> contexts are generated between the
> browsers which will result in rendering
> differences.

> I think that's an argument against having a property, or at
> the very least an argument countering your argument that it's a
> "light-weight feature".

Since I haven't implemented the rendering portion for FF yet, I can't say if there are differences. CSS transforms were implemented by many people, yet still landed despite rendering differences.

> 4) Unfortunately that email didn't spark a large
> conversation. I believe
> this is a light-weight feature to some people will
> really like to use for
> decorative purposes. Its implementation is much
> easier than full-on blending
> of elements and but still fairly powerful. I
> also implemented it in WebKit.

> Not sparking a large conversation doesn't
> make an issue go away.

I know, but an imminent implementation sometimes sparks a discussion :-)
Attachment #714185 - Attachment is obsolete: true
Fixed patch per David's recommendation
Attachment #717522 - Attachment is obsolete: true
Attachment #717522 - Flags: review?(dbaron)
Attachment #717523 - Flags: review?(dbaron)
Attachment #717522 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] from comment #6)
> (In reply to Rik Cabanier from comment #5)
> > Yes, I would like to know what failures you have in mind.
> 
> Nothing in mind, though the patch you posted pretty clearly doesn't compile.

I wonder why you thought it didn't compile. I updated the patch with your suggestions.

> 
> > 3) I implemented it with a prefix in webkit. I'm a little worried about
> > differences in how  and when stacking contexts are generated between the
> > browsers which will result in rendering differences.
> 
> I think that's an argument against having a property, or at the very least
> an argument countering your argument that it's a "light-weight feature".

I implemented the rendering and am surprised to see that it matches webkit.
I will attach the rendering portion as another patch to this bug. Do you have a suggestion how I can test this? (A reftest doesn't seem possible and mozilla doesn't seem to do pixel tests)
Attachment #717576 - Flags: review?(dbaron)
(In reply to Rik Cabanier from comment #10)
> (In reply to David Baron [:dbaron] from comment #6)
> I implemented the rendering and am surprised to see that it matches webkit.
> I will attach the rendering portion as another patch to this bug. Do you
> have a suggestion how I can test this? (A reftest doesn't seem possible and
> mozilla doesn't seem to do pixel tests)

Does it match WebKit in cases where an ancestor has 'opacity'?  Where an ancestor has a transform that's animating?  (And does it behave in a sensible way in those cases?)
Comment on attachment 717523 [details] [diff] [review]
Adds parsing of background-blend-mode

> CSS_PROP_BACKGROUND(
>     -moz-background-inline-policy,
>     _moz_background_inline_policy,
>     CSS_PROP_DOMPROP_PREFIXED(BackgroundInlinePolicy),
>-    CSS_PROPERTY_PARSE_VALUE |
>         CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
>         CSS_PROPERTY_APPLIES_TO_PLACEHOLDER,
>     "",
>     VARIANT_HK,
>     kBackgroundInlinePolicyKTable,
>     CSS_PROP_NO_OFFSET,
>     eStyleAnimType_None)

OK, I suppose this change would compile (I think I misread the first time which line it was removing), but it will fail tests.


That said, I'm more worried about whether this can be interoperable and whether it's worth adding in the first place.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> (In reply to Rik Cabanier from comment #10)
> > (In reply to David Baron [:dbaron] from comment #6)
> > I implemented the rendering and am surprised to see that it matches webkit.
> > I will attach the rendering portion as another patch to this bug. Do you
> > have a suggestion how I can test this? (A reftest doesn't seem possible and
> > mozilla doesn't seem to do pixel tests)
> 
> Does it match WebKit in cases where an ancestor has 'opacity'?  Where an
> ancestor has a transform that's animating?  (And does it behave in a
> sensible way in those cases?)

yes.
it matches webkit with
- opacity
- 2d transform (fixed and animating)
- 3d transform
- z-index
- position: fixed
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> (In reply to Rik Cabanier from comment #10)
> > (In reply to David Baron [:dbaron] from comment #6)
> > I implemented the rendering and am surprised to see that it matches webkit.
> > I will attach the rendering portion as another patch to this bug. Do you
> > have a suggestion how I can test this? (A reftest doesn't seem possible and
> > mozilla doesn't seem to do pixel tests)
> 
> Does it match WebKit in cases where an ancestor has 'opacity'?  Where an
> ancestor has a transform that's animating?  (And does it behave in a
> sensible way in those cases?)

yes.
it matches webkit with
- opacity
- 2d transform (fixed and animating)
- 3d transform
- z-index
- position: fixed
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #13)
> Comment on attachment 717523 [details] [diff] [review]
> Adds parsing of background-blend-mode
> 
> > CSS_PROP_BACKGROUND(
> >     -moz-background-inline-policy,
> >     _moz_background_inline_policy,
> >     CSS_PROP_DOMPROP_PREFIXED(BackgroundInlinePolicy),
> >-    CSS_PROPERTY_PARSE_VALUE |
> >         CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> >         CSS_PROPERTY_APPLIES_TO_PLACEHOLDER,
> >     "",
> >     VARIANT_HK,
> >     kBackgroundInlinePolicyKTable,
> >     CSS_PROP_NO_OFFSET,
> >     eStyleAnimType_None)
> 
> OK, I suppose this change would compile (I think I misread the first time
> which line it was removing), but it will fail tests.
 I ran it on the try server and there were no errors

> 
> That said, I'm more worried about whether this can be interoperable and
> whether it's worth adding in the first place.

I had my doubts too. When I proposed to cut it from the level 1 spec, people objected because they thought it was useful.
Attachment #717576 - Flags: review?(dbaron) → review?(roc)
What is the next step?
I see you assigned the rendering to roc. Are you reviewing the CSS parsing portion?
I think feature-level decisions should precede code review, so the next step is deciding if we want to implement the feature.

(In reply to Rik Cabanier from comment #16)
> > That said, I'm more worried about whether this can be interoperable and
> > whether it's worth adding in the first place.
> 
> I had my doubts too. When I proposed to cut it from the level 1 spec, people
> objected because they thought it was useful.

I don't think that's a useful response to people objecting to adding it because it can't be specified in an interoperable and easily-implementable way.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #18)
> I think feature-level decisions should precede code review, so the next step
> is deciding if we want to implement the feature.
> 
> (In reply to Rik Cabanier from comment #16)
> > > That said, I'm more worried about whether this can be interoperable and
> > > whether it's worth adding in the first place.
> > 
> > I had my doubts too. When I proposed to cut it from the level 1 spec, people
> > objected because they thought it was useful.
> 
> I don't think that's a useful response to people objecting to adding it
> because it can't be specified in an interoperable and easily-implementable
> way.

I decided to implement it because it was easy to implement (Just look at the small impact of this patch).
Surprisingly, it was also interoperable, probably because wk and ff have similar logic when it comes to stacking contexts.
Let's see if we can get some comments from other browser vendors on public-fx.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Let's see if we can get some comments from other browser vendors on
> public-fx.

Yes, thanks!
FYI this code is already in WebKit (but behind a compiler flag). I've asked designers and they told me that they would love to have it.
This patch adds support for blending of background color.
It also catches cases where the background is optimized away because the code thinks that the foreground is opaque
Attachment #717523 - Attachment is obsolete: true
Attachment #717576 - Attachment is obsolete: true
Attachment #717523 - Flags: review?(dbaron)
Attachment #717576 - Flags: review?(roc)
Attachment #723039 - Flags: feedback?(dbaron)
updated patch for latest firefox.
Still need detailed description of grouping in a spec. Latest Firefox almost matches WebKit/Blink apart from overflow: scroll
Attachment #739919 - Flags: feedback?(dbaron)
Attachment #723039 - Attachment is obsolete: true
Attachment #739919 - Attachment is obsolete: true
Attachment #723039 - Flags: feedback?(dbaron)
Attachment #739919 - Flags: feedback?(dbaron)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #22)
> Some links:
> http://lists.w3.org/Archives/Public/public-fx/2013JanMar/thread.html#msg28
> http://dbaron.org/log/20130306-compositing-blending

I uploaded a refresh.
Also, background images no longer blend with the backdrop of the element. Instead, they will only blend among themselves and the background color.
Attachment #752378 - Flags: review?(robert)
Attachment #752378 - Flags: feedback?(dbaron)
What are the use-cases for background-blend-mode specifically? Is it redundant if we have CSS filters or support blending on general elements?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> What are the use-cases for background-blend-mode specifically? 

This allows you to specify blending between regular background images. There are currently no other features that allow graphical effects (such as opacity, filters or blending) on background images.
Basically, you can now have a min-photoshop that allows you to blend layers. (ie a gradient on an image)

> Is it
> redundant if we have CSS filters or support blending on general elements?

It's a bit easier and semantically more correct if you just want to apply an effect on background images instead of forcing elements on top of each other to get the desired effect.
Blending between background images is also lighter weight (since it doesn't force stacking contexts) so it should use no additional resources.
Every CSS property we add has a cost. Properties whose effects can be emulated by other properties without too much work have limited value. I'm uncertain whether this feature is worthwhile. However, I agree this feature looks fairly cheap so I lean towards taking it. Cameron, Jet, David, let me know if you have any feelings on this subject.

Regarding the patch, please break it up into the style system part, another part that implements the drawing, and tests.

Also I expected to see a PushGroup here. It looks to me like this latest patch still blends with the backdrop.

Also, in "if(" there should be a space before the (. Also "if" statement bodies should always have {} around them.
(In reply to Rik Cabanier from comment #26)
> Also, background images no longer blend with the backdrop of the element.
> Instead, they will only blend among themselves and the background color.

This is a big simplification, am I right?  I wonder whether it might be simpler to define a CSS image type that can blend together other images?  For example:

  background-image: blend(linear-gradient(to right, #000000 0%, #ffffff 100%)
                          difference
                          url('ducky.png'))

to take the example in the spec.  This is more limited than specifying the blend mode between each background image layer (since the layers can have different positions, etc.), but would be useful in other contexts where CSS image values can be used.
(In reply to Cameron McCormack (:heycam) from comment #30)
> (In reply to Rik Cabanier from comment #26)
> > Also, background images no longer blend with the backdrop of the element.
> > Instead, they will only blend among themselves and the background color.
> 
> This is a big simplification, am I right?  I wonder whether it might be
> simpler to define a CSS image type that can blend together other images? 
> For example:
> 
>   background-image: blend(linear-gradient(to right, #000000 0%, #ffffff 100%)
>                           difference
>                           url('ducky.png'))
> 

That would be pretty useful. However, you might want to blend 3 or more images together
ie the bottom image is opaque, the one in the middle has opacity and the one on top blends.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> 
> Also I expected to see a PushGroup here. It looks to me like this latest
> patch still blends with the backdrop.
> 
Yes, I was a bit surprised by this too.
A month or 2 ago, this was needed but it seems it no longer is.
Should I add it anyway? (I didn't because I wanted to minimize overhead)
(In reply to Rik Cabanier from comment #31)
> That would be pretty useful. However, you might want to blend 3 or more
> images together
> ie the bottom image is opaque, the one in the middle has opacity and the one
> on top blends.

If by "has opacity" you mean the image already is partially transparent, then you would still be able to do:

background-image: blend(blend(url(a.png) difference url(b.png)) multiply url(c.png));

or whatever syntax you'd like for composing these (maybe a single blend() could take more than one image value argument).

(I see btw that CSS Image Values 4 also wants to propose operations on image values, such as cross-fade().)
(In reply to Cameron McCormack (:heycam) from comment #33)
> (In reply to Rik Cabanier from comment #31)
> > That would be pretty useful. However, you might want to blend 3 or more
> > images together
> > ie the bottom image is opaque, the one in the middle has opacity and the one
> > on top blends.
> 
> If by "has opacity" you mean the image already is partially transparent,
> then you would still be able to do:
> 
> background-image: blend(blend(url(a.png) difference url(b.png)) multiply
> url(c.png));
> 
How would you position the different images with background-position? It's a bit weird since with blend() the images are collapsed into a single group.
I can see how blend() would be useful but it's better to keep them separate.
As I said in comment 30, this is more limited than background-blend-mode since it wouldn't let you specify the positions of the images.  Whether or not that is something that the use cases for this feature requires I do not know.
(In reply to Cameron McCormack (:heycam) from comment #35)
> As I said in comment 30, this is more limited than background-blend-mode
> since it wouldn't let you specify the positions of the images.  Whether or
> not that is something that the use cases for this feature requires I do not
> know.

We definitely want to position the background images. I'm trying to get a designer to create some compelling designs.

Where would your 'blend()' function live? Would it be in CSS images or the blending spec?
(In reply to Rik Cabanier from comment #36)
> We definitely want to position the background images. I'm trying to get a
> designer to create some compelling designs.

OK; it would be good to see concrete examples.

> Where would your 'blend()' function live? Would it be in CSS images or the
> blending spec?

It would probably make a little more sense in the former, though I don't think it would matter too much.
Comment on attachment 752378 [details] [diff] [review]
Adds support for background blend mode

I'm ok with implementing the feature.  But some comments on the patch:

 - please follow local style better.  This means:
   - space between "if" and "(" (same for switch, for)
   - opening { for functions not nested within a class definition
     should be on its own line
   - use existing (tabs vs spaces) indentation chars in Makefile.in

 - don't break -moz-background-inline-policy in nsCSSPropList.h

 - Why are you changing nsDisplayBackgroundColor::GetOpaqueRegion?  My
   understanding of the spec is that the property doesn't affect how the
   color is drawn, only images.

 - same question applies to change to changing
   nsCSSRendering::PaintBackgroundColorWithSC

 - You can't use SetBackgroundList in nsRuleNode.cpp since the rule in
   the spec about list repetition doesn't match the other background
   properties.  I'd actually somewhat prefer to fix the spec, though.

 - Please give member initializers in the order the variables are
   declared (e.g., nsStyleBackground constructor*s*) so as not to cause
   build warnings.

 - Please pack the four uint8_t's together in nsStyleBackground::Layer.

 - Please make sure all the mochitests in layout/style/test pass before
   resubmitting.  (The -moz-background-inline-policy change clearly
   would have broken some.)

 - something needs to ensure that when blend modes are used, the
   background images are only compositing against the images and color
   below them **and in the same background**.

 - the if(bg->mBlendModeCount > bg->mImageCount) checks in
   nsCSSRendering::PaintBackgroundWithSC don't make sense to me.  Could
   you explain?

 - in the same function, where you save and restore the operator, and do
   the restoration by explicitly setting to OPERATOR_OVER -- the
   explicit setting to OPERATOR_OVER is probably ok (rather than an
   actual restore), but only if you have an assertion checking that it's
   actually OPERATOR_OVER before the set.

Sorry for the delay getting to this.
Attachment #752378 - Flags: feedback?(dbaron) → feedback-
(If others agree that this is reasonable to implement, I think the next step is seeing a revised patch addressing the comments.)
(In reply to David Baron [:dbaron] (needinfo? me) from comment #39)
> (If others agree that this is reasonable to implement, I think the next step
> is seeing a revised patch addressing the comments.)

Thanks for the review!
I will work with Horia (olaru@adobe.com) on fixing up the patch.
(In reply to Cameron McCormack (:heycam) from comment #37)
> (In reply to Rik Cabanier from comment #36)
> > We definitely want to position the background images. I'm trying to get a
> > designer to create some compelling designs.
> 
> OK; it would be good to see concrete examples.

We created a cool demo (that was supposed to be shown at the CSS F2F).
Unfortunately, our designer used customer images so we can't publish it but I can show it to you privately.
(In reply to David Baron [:dbaron] (needinfo? me) from comment #38)
> Comment on attachment 752378 [details] [diff] [review]
> Adds support for background blend mode
> 
> I'm ok with implementing the feature.  But some comments on the patch:
> 
>  - please follow local style better.  This means:
>    - space between "if" and "(" (same for switch, for)
>    - opening { for functions not nested within a class definition
>      should be on its own line
>    - use existing (tabs vs spaces) indentation chars in Makefile.in
Done
 
>  - don't break -moz-background-inline-policy in nsCSSPropList.h
Done. There should be a test for this property with background blending as well.
 
>  - Why are you changing nsDisplayBackgroundColor::GetOpaqueRegion?  My
>    understanding of the spec is that the property doesn't affect how the
>    color is drawn, only images.
> 
>  - same question applies to change to changing
>    nsCSSRendering::PaintBackgroundColorWithSC
The change in the spec to not allow backgrounds to blend with whatever is behind the element is newer than the previous patch. Removed offending lines.
 
>  - You can't use SetBackgroundList in nsRuleNode.cpp since the rule in
>    the spec about list repetition doesn't match the other background
>    properties.  I'd actually somewhat prefer to fix the spec, though.
The spec has changed to fix this, and this patch fixes the implementation as well.

>  - Please give member initializers in the order the variables are
>    declared (e.g., nsStyleBackground constructor*s*) so as not to cause
>    build warnings.
Done

>  - Please pack the four uint8_t's together in nsStyleBackground::Layer.
Done, if it means just grouping the lines together.

>  - Please make sure all the mochitests in layout/style/test pass before
>    resubmitting.  (The -moz-background-inline-policy change clearly
>    would have broken some.)
I ran the mochitests locally on a MacBook with retina, and had no failures except for layout/style/test/test_bug412901.html. Two sub pixel tests fail on the retina display, but pass when run on an external, non-retina display.
I do not have privileges to give the patch try bots run, so I will have to ask someone to do that for me. What would it take to get a level 1 account for this purpose in the future?
 
>  - something needs to ensure that when blend modes are used, the
>    background images are only compositing against the images and color
>    below them **and in the same background**.
This is a very good point, and as I said earlier, the patch was older than the spec change that put this in place. I have not managed to do this reliably yet, but want to fix it in a future version of the patch.
The first thing I tried was to isolate blending at draw time, but calling gfxContext::PushGroup when the color started drawing, and PopGroupToSource, and then Paint, when the last image was drawn. However, sometimes identifying the last image layer fails, and the group is not popped. It might be doing something wrong, but it still seems like a hacked together method to do it this way. I'll upload a patch just to show how this works.
I think it might be cleaner to do this by adding a new nsDisplay<BackgroundBlending> subclass, and creating a container in the BuildLayer function, adding background and image layers to that container. This would be similar to how nsDisplayMixBlendMode works. I am still getting my bearing in the gfx / rendering / layout code, so I was wondering how this approach sounds.

>  - the if(bg->mBlendModeCount > bg->mImageCount) checks in
>    nsCSSRendering::PaintBackgroundWithSC don't make sense to me.  Could
>    you explain?
Removed. This is related to the old version of the spec, where the background of an element would also blend with what was behind the element itself. If there are more blend modes than images, that implies lastImageIndex + 1 should be used for the background color to blend with what is behind the element. This is no longer the case.

>  - in the same function, where you save and restore the operator, and do
>    the restoration by explicitly setting to OPERATOR_OVER -- the
>    explicit setting to OPERATOR_OVER is probably ok (rather than an
>    actual restore), but only if you have an assertion checking that it's
>    actually OPERATOR_OVER before the set.
I've changed it to use NS_ASSERTION before all the SetOperator calls. Is this good enough, or should I use MOZ_ASSERT?
Attachment #816637 - Flags: review?(robert)
Attachment #816637 - Flags: feedback?(dbaron)
Comment on attachment 752378 [details] [diff] [review]
Adds support for background blend mode

Review of attachment 752378 [details] [diff] [review]:
-----------------------------------------------------------------

Please split the patch into at least these parts:
1) Adding background-blend-mode to the style system
2) Rendering implementation
3) Tests
Attachment #816637 - Attachment is obsolete: true
Attachment #816637 - Flags: review?(robert)
Attachment #816637 - Flags: feedback?(dbaron)
Attachment #817787 - Flags: review?(roc)
Attachment #817787 - Flags: feedback?(dbaron)
This part still needs work. There is only a parsing test, and it does not check the new background property list rotation behavior.
Attachment #817787 - Flags: review?(roc)
Attachment #817787 - Flags: review?(cam)
Attachment #817787 - Flags: feedback?(dbaron)
Comment on attachment 817787 [details] [diff] [review]
Add support background-blend-mode parsing and style integration.

Review of attachment 817787 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall; just one thing:

::: layout/style/test/property_database.js
@@ +1372,5 @@
> +		type: CSS_TYPE_LONGHAND,
> +		initial_values: [ "normal" ],
> +		other_values: [ "multiply", "screen", "overlay", "darken", "lighten", "color-dodge", "color-burn", "hard-light", "soft-light", "difference", "exclusion", "hue", "saturation", "color", "luminosity" ],
> +		invalid_values: []
> +	},

You'll need to add this entry to gCSSProperties only if the layout.css.background-blend-mode.enabled pref is set.  See down the end of this file how to do that.
Attachment #817787 - Flags: review?(cam)
Good point. I have changed it accordingly.
Attachment #821101 - Flags: review?(cam)
This is an updated implementation of rendering that keeps the background blending within the background.

I'm not sure who to add as a reviewer to this one.
Attachment #817787 - Attachment is obsolete: true
Attachment #817789 - Attachment is obsolete: true
Comment on attachment 821101 [details] [diff] [review]
0001-Add-background-blend-mode-to-the-style-parsing-mecha.patch

r=me if you replace the tab characters with spaces for two-space indentation.
Attachment #821101 - Flags: review?(cam) → review+
Attached patch Add-background-blend-mode-tests (obsolete) — Splinter Review
This patch adds about 10 more reftests aside from the previous mochi test. Again, I am unsure as to who to add as a reviewer.
Attachment #817791 - Attachment is obsolete: true
(In reply to Cameron McCormack (:heycam) from comment #50)
> Comment on attachment 821101 [details] [diff] [review]
> 0001-Add-background-blend-mode-to-the-style-parsing-mecha.patch
> 
> r=me if you replace the tab characters with spaces for two-space indentation.

I believe you are referring to the property_database change. I agree copy-paste made me botch indentation (because there are both tabs and spaces there), but that file seems full of tabs, except for the last few lines. Wouldn't it be better to use tabs all around to keep it consistent?
(In reply to Horia Iosif Olaru from comment #52)
> I believe you are referring to the property_database change. I agree
> copy-paste made me botch indentation (because there are both tabs and spaces
> there), but that file seems full of tabs, except for the last few lines.
> Wouldn't it be better to use tabs all around to keep it consistent?

Oh, you're right; I didn't notice the prevalence of tabs in the file at the moment.  So yes, keep it consistent for now.  Thanks!
Changed spaces to tabs in property database js to match file style.
Attachment #821101 - Attachment is obsolete: true
Attachment #821334 - Flags: review?(cam)
Attachment #821334 - Flags: review?(cam) → review+
Comment on attachment 821108 [details] [diff] [review]
Add-background-blend-mode-implementation

Hi Robert. Could you please review, or point me to the right person?
Attachment #821108 - Flags: review?(roc)
Comment on attachment 821108 [details] [diff] [review]
Add-background-blend-mode-implementation

Review of attachment 821108 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2771,5 @@
>                                                state.mDestArea, state.mFillArea,
>                                                state.mAnchor + aBorderArea.TopLeft(),
>                                                clipState.mDirtyRect);
> +          if (state.mCompositingOp != gfxContext::OPERATOR_OVER)
> +            ctx->SetOperator(gfxContext::OPERATOR_OVER);

{} around if body.

::: layout/base/nsDisplayList.cpp
@@ +1773,5 @@
>      if (bg->mLayers[i].mImage.IsEmpty()) {
>        continue;
>      }
> +    if (!useBackgroundBlending && bg->mLayers[i].mBlendMode != NS_STYLE_BLEND_NORMAL)
> +      useBackgroundBlending = true;

{} around if body.

Skip the !useBackgroundBlending check, it's unnecessary.

@@ +1783,5 @@
>  
> +  if (useBackgroundBlending) {
> +    aList->AppendNewToTop(
> +      new (aBuilder) nsDisplayBlendContainer(aBuilder, aFrame, aList));
> +  }

This is wrong. The aList that is passed in may already have items in it from other elements, and it's wrong to wrap them all in an nsDisplayBlendContainer. When there is blending, you need to wrap the background image layers and the element's background color (if any) in the nsDisplayBlendContainer --- and no others. So you'll need a temporary list in AppendBackgroundItemsToTop.
Attachment #821108 - Flags: review?(roc) → review-
I have addressed the comments. Good point on the auxiliary list, though I wish I had found a cleaner way to handle early returns.
Attachment #821108 - Attachment is obsolete: true
Attachment #823379 - Flags: review?(roc)
Comment on attachment 823379 [details] [diff] [review]
Add-background-blend-mode-implementation

Review of attachment 823379 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +1771,4 @@
>      return false;
>    }
>  
> +  bool useBackgroundBlending = false;

call this "needBlendContainer".

@@ +2116,4 @@
>    if (mBackgroundStyle->mBackgroundInlinePolicy == NS_STYLE_BG_INLINE_POLICY_EACH_BOX ||
>        (!mFrame->GetPrevContinuation() && !mFrame->GetNextContinuation())) {
>      const nsStyleBackground::Layer& layer = mBackgroundStyle->mLayers[mLayer];
> +    if (layer.mImage.IsOpaque() && (layer.mBlendMode == NS_STYLE_BLEND_NORMAL)) {

Don't need inner ()
Attachment #823379 - Flags: review?(roc) → review+
Fixed.
Attachment #823379 - Attachment is obsolete: true
Attachment #824325 - Flags: review?(roc)
Comment on attachment 824325 [details] [diff] [review]
Add-background-blend-mode-implementation

Canceling the review, since I just read from the docs that it is unnecessary. Sorry about that.
Attachment #824325 - Flags: review?(roc)
Comment on attachment 821112 [details] [diff] [review]
Add-background-blend-mode-tests

Please let me know if you're not the right person.
Attachment #821112 - Flags: review?(roc)
This second test patch adds a set of tests for each particular blend mode. It re-uses the results in reftests/svg (such as blend-screen-ref.svg). Currently I am copying them. Is there a way to reference them directly instead?
Attachment #824337 - Flags: review?(roc)
Attachment #752378 - Attachment is obsolete: true
Attachment #752378 - Flags: review?(robert)
Comment on attachment 821112 [details] [diff] [review]
Add-background-blend-mode-tests

Review of attachment 821112 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/css-blending/background-blending-isolation.html
@@ +25,5 @@
> +
> +</style>
> +<body>
> +	<div class="reftest image"></div>
> +	<div class="reftest color"></div>

Make indentation consistent (here and everywhere else). 2 space indent would be better.
Done. I had fixed indentation locally but the patch was not updated.
Attachment #821112 - Attachment is obsolete: true
Attachment #821112 - Flags: review?(roc)
Attachment #824529 - Flags: review?(roc)
Replaced a found tab character.
Attachment #824337 - Attachment is obsolete: true
Refresh after rebase.
Attachment #824325 - Attachment is obsolete: true
It seems git diff has a bug that breaks patches when ignore space parameters are used. This is a fixed patch - only the diff context is changed to make it apply.
Attachment #824969 - Attachment is obsolete: true
Same fix as previous upload. Only diff context changed in the patch.
Attachment #824531 - Attachment is obsolete: true
Rebased patch.
Attachment #821334 - Attachment is obsolete: true
Rebased patch.
Attachment #825469 - Attachment is obsolete: true
Rebased patch and added some fuzz to account for some tests that fail because of small differences. I assume the errors are related to some color conversion that is happening, and will fix them later.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=a5636e0fccf6
Attachment #824529 - Attachment is obsolete: true
Attachment #827478 - Flags: review?(roc)
Rebased patch and added some fuzz to account for some tests that fail because of small differences. I assume the errors are related to some color conversion that is happening, and will fix them later.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=a5636e0fccf6
Attachment #825471 - Attachment is obsolete: true
Attachment #827479 - Flags: review?(roc)
Fix typo in previous patch.
Attachment #827478 - Attachment is obsolete: true
Attachment #827478 - Flags: review?(roc)
Attachment #827579 - Flags: review?(roc)
Fix last try server errors in reftest.list.
https://tbpl.mozilla.org/?tree=Try&rev=fd630058885e
Attachment #827479 - Attachment is obsolete: true
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #63)
Last try server results passed (the failures are not related to this patch).
Robert, could you please add a 'checking-needed' if there is nothing else that needs to be done? I don't think I have permissions to add the keyword myself.
Keywords: checkin-needed
Depends on: 950416
Depends on: 959674
Whiteboard: [DocArea=CSS]
Depends on: 1042411
Depends on: 1054748
Depends on: 1228716
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: