Last Comment Bug 841601 - Add support for background-blend-mode
: Add support for background-blend-mode
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla28
Assigned To: Horia Iosif Olaru
:
Mentors:
Depends on: 1234649 950416 959674 1042411 1054748 1228716
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-14 16:24 PST by Rik Cabanier
Modified: 2015-12-22 12:19 PST (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First try (11.89 KB, patch)
2013-02-14 16:30 PST, Rik Cabanier
no flags Details | Diff | Review
Fixed strange diff (10.73 KB, patch)
2013-02-14 17:03 PST, Rik Cabanier
dbaron: feedback-
Details | Diff | Review
Fixed patch per David's recommendation (21.29 KB, patch)
2013-02-23 08:48 PST, Rik Cabanier
no flags Details | Diff | Review
Adds parsing of background-blend-mode (21.28 KB, patch)
2013-02-23 08:52 PST, Rik Cabanier
no flags Details | Diff | Review
Implements rendering of background blending (5.50 KB, patch)
2013-02-23 20:15 PST, Rik Cabanier
no flags Details | Diff | Review
implement parsing and rendering of background-blend-mode (31.14 KB, patch)
2013-03-08 19:49 PST, Rik Cabanier
no flags Details | Diff | Review
Add support for background blending (30.90 KB, patch)
2013-04-19 20:56 PDT, Rik Cabanier
no flags Details | Diff | Review
Adds support for background blend mode (30.94 KB, patch)
2013-05-21 14:23 PDT, Rik Cabanier
dbaron: feedback-
Details | Diff | Review
Add CSS background-blend-mode - address David's comments (16.36 KB, patch)
2013-10-14 08:57 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Add support background-blend-mode parsing and style integration. (8.72 KB, patch)
2013-10-16 05:51 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Add support background-blend-mode rendering (without isolation) (5.28 KB, patch)
2013-10-16 05:57 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Add tests for background-blend-mode (2.57 KB, patch)
2013-10-16 05:58 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
0001-Add-background-blend-mode-to-the-style-parsing-mecha.patch (8.89 KB, patch)
2013-10-23 09:55 PDT, Horia Iosif Olaru
cam: review+
Details | Diff | Review
Add-background-blend-mode-implementation (6.09 KB, patch)
2013-10-23 09:58 PDT, Horia Iosif Olaru
roc: review-
Details | Diff | Review
Add-background-blend-mode-tests (242.55 KB, patch)
2013-10-23 10:00 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Add-background-blend-mode-to-the-style-parsing-mechanism (8.88 KB, patch)
2013-10-23 15:35 PDT, Horia Iosif Olaru
cam: review+
Details | Diff | Review
Add-background-blend-mode-implementation (7.36 KB, patch)
2013-10-28 08:29 PDT, Horia Iosif Olaru
roc: review+
Details | Diff | Review
Add-background-blend-mode-implementation (7.35 KB, patch)
2013-10-29 15:59 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
patch 4 - Test background blending for each individual blend mode (41.28 KB, patch)
2013-10-29 16:11 PDT, Horia Iosif Olaru
roc: review+
Details | Diff | Review
Patch 3 - Add-background-blend-mode-tests (242.50 KB, patch)
2013-10-30 01:47 PDT, Horia Iosif Olaru
roc: review+
Details | Diff | Review
patch 4 - Test background blending for each individual blend mode (41.27 KB, patch)
2013-10-30 01:49 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 2 - Add-background-blend-mode-implementation (7.35 KB, patch)
2013-10-30 16:14 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 2 - Add-background-blend-mode-implementation (7.36 KB, patch)
2013-10-31 13:03 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 4 - Add-a-second-batch-of-tests-of-tests-for-background (41.54 KB, patch)
2013-10-31 13:04 PDT, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 1: Add-background-blend-mode-to-the-style-parsing-mecha (8.89 KB, patch)
2013-11-05 09:31 PST, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 2: Add-background-blend-mode-implementation (7.35 KB, patch)
2013-11-05 09:32 PST, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 3: Add-background-blend-mode-tests (242.54 KB, patch)
2013-11-05 09:34 PST, Horia Iosif Olaru
no flags Details | Diff | Review
Patch 4-Add-a-second-batch-of-tests-of-tests-for-background (41.63 KB, patch)
2013-11-05 09:35 PST, Horia Iosif Olaru
roc: review+
Details | Diff | Review
Patch 3: Add-background-blend-mode-tests (242.53 KB, patch)
2013-11-05 12:15 PST, Horia Iosif Olaru
roc: review+
Details | Diff | Review
Patch 4: Add-a-second-batch-of-tests-of-tests-for-background-blending. (41.66 KB, patch)
2013-11-06 00:55 PST, Horia Iosif Olaru
no flags Details | Diff | Review

Description Rik Cabanier 2013-02-14 16:24:56 PST
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
Comment 1 Rik Cabanier 2013-02-14 16:30:11 PST
Created attachment 714166 [details] [diff] [review]
First try
Comment 2 Rik Cabanier 2013-02-14 17:03:16 PST
Created attachment 714185 [details] [diff] [review]
Fixed strange diff
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-14 20:22:11 PST
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.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-14 20:23:28 PST
Comment on attachment 714185 [details] [diff] [review]
Fixed strange diff

see comment 3
Comment 5 Rik Cabanier 2013-02-15 00:05:50 PST
(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.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-15 09:42:01 PST
(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.
Comment 7 Rik Cabanier 2013-02-16 13:42:14 PST
(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 :-)
Comment 8 Rik Cabanier 2013-02-23 08:48:11 PST
Created attachment 717522 [details] [diff] [review]
Fixed patch per David's recommendation
Comment 9 Rik Cabanier 2013-02-23 08:52:13 PST
Created attachment 717523 [details] [diff] [review]
Adds parsing of background-blend-mode

Fixed patch per David's recommendation
Comment 10 Rik Cabanier 2013-02-23 20:12:30 PST
(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)
Comment 11 Rik Cabanier 2013-02-23 20:15:16 PST
Created attachment 717576 [details] [diff] [review]
Implements rendering of background blending
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-24 12:59:13 PST
(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 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-24 13:03:35 PST
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.
Comment 14 Rik Cabanier 2013-02-24 15:23:29 PST
(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
Comment 15 Rik Cabanier 2013-02-24 15:23:43 PST
(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
Comment 16 Rik Cabanier 2013-02-24 15:26:07 PST
(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.
Comment 17 Rik Cabanier 2013-02-26 16:11:46 PST
What is the next step?
I see you assigned the rendering to roc. Are you reviewing the CSS parsing portion?
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-26 16:16:00 PST
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.
Comment 19 Rik Cabanier 2013-02-26 17:11:46 PST
(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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-03-03 20:52:40 PST
Let's see if we can get some comments from other browser vendors on public-fx.
Comment 21 Rik Cabanier 2013-03-04 09:05:56 PST
(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.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-03-06 17:22:41 PST
Some links:
http://lists.w3.org/Archives/Public/public-fx/2013JanMar/thread.html#msg28
http://dbaron.org/log/20130306-compositing-blending
Comment 23 Rik Cabanier 2013-03-08 19:49:57 PST
Created attachment 723039 [details] [diff] [review]
implement parsing and rendering of background-blend-mode

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
Comment 24 Rik Cabanier 2013-04-19 20:56:39 PDT
Created attachment 739919 [details] [diff] [review]
Add support for background blending

updated patch for latest firefox.
Still need detailed description of grouping in a spec. Latest Firefox almost matches WebKit/Blink apart from overflow: scroll
Comment 25 Rik Cabanier 2013-05-21 14:23:57 PDT
Created attachment 752378 [details] [diff] [review]
Adds support for background blend mode
Comment 26 Rik Cabanier 2013-05-21 14:25:40 PDT
(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.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-05-27 03:03:54 PDT
What are the use-cases for background-blend-mode specifically? Is it redundant if we have CSS filters or support blending on general elements?
Comment 28 Rik Cabanier (don't use this account) 2013-05-27 19:51:04 PDT
(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.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-06 05:11:36 PDT
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.
Comment 30 Cameron McCormack (:heycam) 2013-06-07 20:30:05 PDT
(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.
Comment 31 Rik Cabanier (don't use this account) 2013-06-07 20:46:51 PDT
(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.
Comment 32 Rik Cabanier (don't use this account) 2013-06-07 20:48:10 PDT
(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)
Comment 33 Cameron McCormack (:heycam) 2013-06-07 21:07:34 PDT
(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().)
Comment 34 Rik Cabanier (don't use this account) 2013-06-07 21:49:30 PDT
(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.
Comment 35 Cameron McCormack (:heycam) 2013-06-07 21:58:17 PDT
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.
Comment 36 Rik Cabanier (don't use this account) 2013-06-07 22:05:37 PDT
(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?
Comment 37 Cameron McCormack (:heycam) 2013-06-07 22:08:21 PDT
(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 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-09-17 15:25:55 PDT
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.
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-09-17 15:48:49 PDT
(If others agree that this is reasonable to implement, I think the next step is seeing a revised patch addressing the comments.)
Comment 40 Rik Cabanier 2013-09-17 15:51:25 PDT
(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.
Comment 41 Rik Cabanier 2013-09-17 15:53:28 PDT
(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.
Comment 42 Horia Iosif Olaru 2013-10-14 08:57:46 PDT
Created attachment 816637 [details] [diff] [review]
Add CSS background-blend-mode - address David's comments

(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?
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-14 15:39:01 PDT
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
Comment 44 Horia Iosif Olaru 2013-10-16 05:51:10 PDT
Created attachment 817787 [details] [diff] [review]
Add support background-blend-mode parsing and style integration.
Comment 45 Horia Iosif Olaru 2013-10-16 05:57:17 PDT
Created attachment 817789 [details] [diff] [review]
Add support background-blend-mode rendering (without isolation)
Comment 46 Horia Iosif Olaru 2013-10-16 05:58:52 PDT
Created attachment 817791 [details] [diff] [review]
Add tests for background-blend-mode

This part still needs work. There is only a parsing test, and it does not check the new background property list rotation behavior.
Comment 47 Cameron McCormack (:heycam) 2013-10-22 08:42:33 PDT
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.
Comment 48 Horia Iosif Olaru 2013-10-23 09:55:32 PDT
Created attachment 821101 [details] [diff] [review]
0001-Add-background-blend-mode-to-the-style-parsing-mecha.patch

Good point. I have changed it accordingly.
Comment 49 Horia Iosif Olaru 2013-10-23 09:58:08 PDT
Created attachment 821108 [details] [diff] [review]
Add-background-blend-mode-implementation

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.
Comment 50 Cameron McCormack (:heycam) 2013-10-23 09:59:12 PDT
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.
Comment 51 Horia Iosif Olaru 2013-10-23 10:00:25 PDT
Created attachment 821112 [details] [diff] [review]
Add-background-blend-mode-tests

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.
Comment 52 Horia Iosif Olaru 2013-10-23 10:11:02 PDT
(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?
Comment 53 Cameron McCormack (:heycam) 2013-10-23 12:40:43 PDT
(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!
Comment 54 Horia Iosif Olaru 2013-10-23 15:35:41 PDT
Created attachment 821334 [details] [diff] [review]
Add-background-blend-mode-to-the-style-parsing-mechanism

Changed spaces to tabs in property database js to match file style.
Comment 55 Horia Iosif Olaru 2013-10-24 07:49:58 PDT
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?
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-24 13:31:11 PDT
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.
Comment 57 Horia Iosif Olaru 2013-10-28 08:29:37 PDT
Created attachment 823379 [details] [diff] [review]
Add-background-blend-mode-implementation

I have addressed the comments. Good point on the auxiliary list, though I wish I had found a cleaner way to handle early returns.
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-28 19:11:01 PDT
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 ()
Comment 59 Horia Iosif Olaru 2013-10-29 15:59:10 PDT
Created attachment 824325 [details] [diff] [review]
Add-background-blend-mode-implementation

Fixed.
Comment 60 Horia Iosif Olaru 2013-10-29 16:08:11 PDT
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.
Comment 61 Horia Iosif Olaru 2013-10-29 16:09:51 PDT
Comment on attachment 821112 [details] [diff] [review]
Add-background-blend-mode-tests

Please let me know if you're not the right person.
Comment 62 Horia Iosif Olaru 2013-10-29 16:11:41 PDT
Created attachment 824337 [details] [diff] [review]
patch 4 - Test background blending for each individual blend mode

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?
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-29 18:39:12 PDT
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.
Comment 64 Horia Iosif Olaru 2013-10-30 01:47:03 PDT
Created attachment 824529 [details] [diff] [review]
Patch 3 - Add-background-blend-mode-tests

Done. I had fixed indentation locally but the patch was not updated.
Comment 65 Horia Iosif Olaru 2013-10-30 01:49:42 PDT
Created attachment 824531 [details] [diff] [review]
patch 4 - Test background blending for each individual blend mode

Replaced a found tab character.
Comment 66 Horia Iosif Olaru 2013-10-30 16:14:33 PDT
Created attachment 824969 [details] [diff] [review]
Patch 2 - Add-background-blend-mode-implementation

Refresh after rebase.
Comment 67 Horia Iosif Olaru 2013-10-31 13:03:35 PDT
Created attachment 825469 [details] [diff] [review]
Patch 2 - Add-background-blend-mode-implementation

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.
Comment 68 Horia Iosif Olaru 2013-10-31 13:04:44 PDT
Created attachment 825471 [details] [diff] [review]
Patch 4 - Add-a-second-batch-of-tests-of-tests-for-background

Same fix as previous upload. Only diff context changed in the patch.
Comment 69 Horia Iosif Olaru 2013-11-05 09:31:10 PST
Created attachment 827476 [details] [diff] [review]
Patch 1: Add-background-blend-mode-to-the-style-parsing-mecha

Rebased patch.
Comment 70 Horia Iosif Olaru 2013-11-05 09:32:25 PST
Created attachment 827477 [details] [diff] [review]
Patch 2: Add-background-blend-mode-implementation

Rebased patch.
Comment 71 Horia Iosif Olaru 2013-11-05 09:34:26 PST
Created attachment 827478 [details] [diff] [review]
Patch 3: Add-background-blend-mode-tests

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
Comment 72 Horia Iosif Olaru 2013-11-05 09:35:07 PST
Created attachment 827479 [details] [diff] [review]
Patch 4-Add-a-second-batch-of-tests-of-tests-for-background

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
Comment 73 Horia Iosif Olaru 2013-11-05 12:15:49 PST
Created attachment 827579 [details] [diff] [review]
Patch 3: Add-background-blend-mode-tests

Fix typo in previous patch.
Comment 74 Horia Iosif Olaru 2013-11-06 00:55:49 PST
Created attachment 827907 [details] [diff] [review]
Patch 4: Add-a-second-batch-of-tests-of-tests-for-background-blending.

Fix last try server errors in reftest.list.
https://tbpl.mozilla.org/?tree=Try&rev=fd630058885e
Comment 75 Andrew Quartey [:drexler] 2013-11-07 15:08:35 PST
Pushed patches to try: remote:   https://tbpl.mozilla.org/?tree=Try&rev=6dafc32ebc8b
Comment 76 Horia Iosif Olaru 2013-11-08 03:26:57 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.