Closed Bug 902525 Opened 11 years ago Closed 11 years ago

Implement support for blending of SVG and HTML elements

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: cabanier, Assigned: cabanier)

References

Details

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

Attachments

(7 files, 55 obsolete files)

10.22 KB, patch
Details | Diff | Splinter Review
16.46 KB, patch
Details | Diff | Splinter Review
13.10 KB, patch
Details | Diff | Splinter Review
2.80 KB, patch
Details | Diff | Splinter Review
1.61 KB, patch
Details | Diff | Splinter Review
11.49 KB, patch
Details | Diff | Splinter Review
54.26 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36
Depends on: 901375
Assignee: nobody → cabanier
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Attached patch rev1.patch (obsolete) — Splinter Review
Attachment #787075 - Flags: review?(cam)
try run: https://tbpl.mozilla.org/?tree=Try&rev=dc98f7b57c87
Fails on win7 and above. investigating.
Clicking on the element causes it to blend as expected. page redraw will show incorrect result again.
This is going to require very careful handling of interactions with isolation. Can you explain how you're going to handle that?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> This is going to require very careful handling of interactions with
> isolation. Can you explain how you're going to handle that?

After talking on the mailing list: 
"roc: here's what I think you should do. You should write code to walk up the element hierarchy from a mix-blend-mode element to find the nearest ancestor that is supposed to be an isolated group, according to spec. Then you should force creation of a layer for that element, and force that layer to make all its descendants "inactive". that way a single ThebesLayer will be used for all .its descendants and mix-blend-mode will work correctly for them."
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> This is going to require very careful handling of interactions with
> isolation. Can you explain how you're going to handle that?

Where should I create this 'tree walking' code? In a part of FrameLayerBuilder.cpp?
Attached patch rev_3.patch (obsolete) — Splinter Review
Attachment #787075 - Attachment is obsolete: true
Attachment #787075 - Flags: review?(cam)
Attachment #790529 - Flags: feedback?(roc)
Comment on attachment 790529 [details] [diff] [review]
rev_3.patch

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

::: gfx/layers/Layers.h
@@ +898,5 @@
>    }
>  
>    // These getters can be used anytime.
>    float GetOpacity() { return mOpacity; }
> +  uint8_t GetMixBlendMode() { return mMixBlendMode; }

Can we have a proper enum for these modes instead of a uint8_t?
Please break this patch up into smaller patches for easier review and maintenance. Each prefix of the list of patches should build and pass our existing tests. I think you can do it like this:
1) Layers changes.
2) Changes to force layerization of the "blend container" stacking context.
3) Implementation of mix-blend-mode.
4) New tests.
Thanks!
Attached patch layers.patch (obsolete) — Splinter Review
try server run: https://tbpl.mozilla.org/?tree=Try&rev=241a1f8520fb
Attachment #790612 - Flags: review?(roc)
Attachment #790529 - Flags: feedback?(roc)
You might as well attach all the patches as you produce them. We generally don't require an actual tryserver run for each prefix of the patch list.
Comment on attachment 790612 [details] [diff] [review]
layers.patch

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

::: gfx/layers/Layers.cpp
@@ +173,5 @@
>    mMaskLayer(nullptr),
>    mPostXScale(1.0f),
>    mPostYScale(1.0f),
>    mOpacity(1.0),
> +  mMixBlendMode(NS_STYLE_BLEND_NORMAL),

Layers code should not depend on the style system. Use gfxContext values instead.

::: gfx/layers/Layers.h
@@ +710,5 @@
>        Mutated();
>      }
>    }
>  
> +  void SetMixBlendMode(uint8_t aMixBlendMode)

We should be using an enum here.
Comment on attachment 790612 [details] [diff] [review]
layers.patch

>   // These getters can be used anytime.
>   float GetOpacity() { return mOpacity; }
>+  uint8_t GetMixBlendMode() { return mMixBlendMode; }

Getters should be const methods ideally.
Attached patch layers_2.patch (obsolete) — Splinter Review
Attachment #790612 - Attachment is obsolete: true
Attachment #790612 - Flags: review?(roc)
Attachment #790683 - Flags: review?(roc)
(In reply to Robert Longson from comment #13)
> Comment on attachment 790612 [details] [diff] [review]
> layers.patch
> 
> >   // These getters can be used anytime.
> >   float GetOpacity() { return mOpacity; }
> >+  uint8_t GetMixBlendMode() { return mMixBlendMode; }
> 
> Getters should be const methods ideally.

oops. Just saw this review; I will upload a new patch
Attached patch layers_3.patch (obsolete) — Splinter Review
Attachment #790683 - Attachment is obsolete: true
Attachment #790683 - Flags: review?(roc)
Attachment #790684 - Flags: review?(roc)
Attachment #790684 - Attachment is obsolete: true
Attachment #790684 - Flags: review?(roc)
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #790685 - Flags: review?(roc)
(In reply to Robert Longson from comment #13)
> Comment on attachment 790612 [details] [diff] [review]
> layers.patch
> 
> >   // These getters can be used anytime.
> >   float GetOpacity() { return mOpacity; }
> >+  uint8_t GetMixBlendMode() { return mMixBlendMode; }
> 
> Getters should be const methods ideally.

One of the getters uses getParent which is non-const (??) so I couldn't change its signature
Attached patch stacking_1.patch (obsolete) — Splinter Review
patch for layerization of blend mode and blend mode container
Attachment #790697 - Flags: review?(roc)
Comment on attachment 790697 [details] [diff] [review]
stacking_1.patch

>+  void SetContainsBlendMode(bool aContainsBlendMode) { mContainsBlendMode = aContainsBlendMode; }
>+  bool ContainsBlendMode() { return mContainsBlendMode; }

This should be a const method
Attached patch stacking_2.patch (obsolete) — Splinter Review
Attachment #790697 - Attachment is obsolete: true
Attachment #790697 - Flags: review?(roc)
Attachment #790831 - Flags: review?(longsonr)
(In reply to Robert Longson from comment #20)
> Comment on attachment 790697 [details] [diff] [review]
> stacking_1.patch
> 
> >+  void SetContainsBlendMode(bool aContainsBlendMode) { mContainsBlendMode = aContainsBlendMode; }
> >+  bool ContainsBlendMode() { return mContainsBlendMode; }
> 
> This should be a const method
uploaded new patch
Comment on attachment 790831 [details] [diff] [review]
stacking_2.patch

>+nsDisplayMixBlendMode::nsDisplayMixBlendMode(nsDisplayListBuilder* aBuilder,
>+                                     nsIFrame* aFrame, nsDisplayList* aList,
>+                                     uint32_t aFlags)

The lines above should align with the (

> 
>   bool inTransform = aBuilder->IsInTransform();
>   bool isTransformed = IsTransformed();
>+  ManageBlendMode manageBlendMode(*aBuilder);

A comment to explain why we're temporarily overriding the blend mode would be useful.

>   if (isTransformed) {
>     if (aBuilder->IsForPainting() &&
>         nsDisplayTransform::ShouldPrerenderTransformedContent(aBuilder, this)) {
>       dirtyRect = GetVisualOverflowRectRelativeToSelf();
>     } else {
>       // Trying to  back-transform arbitrary rects gives us really weird results. I believe 
>       // this is from points that lie beyond the vanishing point. As a workaround we transform t
>       // he overflow rect into screen space and compare in that coordinate system.
>@@ -1799,21 +1819,22 @@ nsIFrame::BuildDisplayListForStackingCon
>       if (!Preserves3DChildren() && !dirtyRect.Intersects(GetVisualOverflowRectRelativeToSelf())) {
>         return;
>       }
>     }
>     inTransform = true;
>   }
> 
>   bool useOpacity = HasOpacity() && !nsSVGUtils::CanOptimizeOpacity(this);
>+  bool useBlendMode = disp->mMixBlendMode != NS_STYLE_BLEND_NORMAL;
>   bool usingSVGEffects = nsSVGIntegrationUtils::UsingEffectsForFrame(this);
> 
>   DisplayListClipState::AutoSaveRestore clipState(aBuilder);
> 
...

>@@ -1915,23 +1936,28 @@ nsIFrame::BuildDisplayListForStackingCon
>    * item even if resultList is empty, since a filter can produce graphical
>    * output even if the element being filtered wouldn't otherwise do so.
>    */
>   if (usingSVGEffects) {
>     /* List now emptied, so add the new list to the top. */
>     resultList.AppendNewToTop(
>         new (aBuilder) nsDisplaySVGEffects(aBuilder, this, &resultList));
>   }
>-  /* Else, if the list is non-empty and there is CSS group opacity without SVG
>-   * effects, wrap it up in an opacity item.
>+  /* Else, if the list is non-empty and there is CSS group opacity or blending
>+   * without SVG effects, wrap it up in an opacity item.
>    */

Not sure the comment is consistent with the code useOpacity won't be set if there's
blending and opacity = 1 so what are we trying to do here when opacity and blending interact?

>   else if (useOpacity && !resultList.IsEmpty()) {
>     resultList.AppendNewToTop(
>         new (aBuilder) nsDisplayOpacity(aBuilder, this, &resultList));
>   }
>+    
>+  if (useBlendMode) {
>+    resultList.AppendNewToTop(
>+        new (aBuilder) nsDisplayMixBlendMode(aBuilder, this, &resultList));
>+  }

Should have tests for opacity and blending interaction and SVG effects and blending interaction.
Attached patch stacking_3.patch (obsolete) — Splinter Review
Attachment #790831 - Attachment is obsolete: true
Attachment #790831 - Flags: review?(longsonr)
Attachment #790932 - Flags: review?(longsonr)
(In reply to Robert Longson from comment #23)
> Comment on attachment 790831 [details] [diff] [review]
> stacking_2.patch
> 
> A comment to explain why we're temporarily overriding the blend mode would
> be useful.

done

> 
> >   DisplayListClipState::AutoSaveRestore clipState(aBuilder);
> > 
> ...

?

> 
> >-  /* Else, if the list is non-empty and there is CSS group opacity without SVG
> >-   * effects, wrap it up in an opacity item.
> >+  /* Else, if the list is non-empty and there is CSS group opacity or blending
> >+   * without SVG effects, wrap it up in an opacity item.
> >    */
> 
> Not sure the comment is consistent with the code useOpacity won't be set if
> there's
> blending and opacity = 1 so what are we trying to do here when opacity and
> blending interact?

You can apply blending after doing opacity on the content (but not the other way around).
I added comments.

> 
> >   else if (useOpacity && !resultList.IsEmpty()) {
> >     resultList.AppendNewToTop(
> >         new (aBuilder) nsDisplayOpacity(aBuilder, this, &resultList));
> >   }
> >+    
> >+  if (useBlendMode) {
> >+    resultList.AppendNewToTop(
> >+        new (aBuilder) nsDisplayMixBlendMode(aBuilder, this, &resultList));
> >+  }
> 
> Should have tests for opacity and blending interaction and SVG effects and
> blending interaction.

Yes. I already have a bunch of tests. 
Those (and more) will be sumbitted as the last step.
Comment on attachment 790932 [details] [diff] [review]
stacking_3.patch

>     return;
>   }
> 
>   nsRect dirtyRect = aDirtyRect;
> 
>   bool inTransform = aBuilder->IsInTransform();
>   bool isTransformed = IsTransformed();
>+  // reset blend mode so we can keep track if this stacking context need have

s/need have/needs to have/

>+  // a nsDisplayBlendContainer. Set the blend mode back when the routine exists
>+  // so we keep track if the parent stacking context needs a container too.
Attachment #790932 - Flags: review?(longsonr) → review+
Attachment #790685 - Flags: review?(roc) → review?(longsonr)
Attachment #790685 - Flags: review?(longsonr) → review?(roc)
It's helpful to have your patches be numbered "Part 1", "Part 2" etc so we can see what order they apply in.
Attachment #790685 - Attachment description: layers_4.patch → Part 1: Layers changes
Comment on attachment 791019 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

Carrying forward rlongson's review.

This patch isn't really about creating stacking contexts. It's about creating layers for stacking contexts that need to be isolated groups.
Attachment #791019 - Attachment description: stacking_4.patch → Part 2: Create layers for isolated groups when blending is involved
Attachment #791019 - Flags: review+
Comment on attachment 790685 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/Layers.cpp
@@ +676,5 @@
>  
> +gfxContext::GraphicsOperator
> +Layer::GetEffectiveMixBlendMode()
> +{
> +  if(mMixBlendMode != gfxContext::OPERATOR_SOURCE)

Space before ( (here and elsewhere)

@@ +679,5 @@
> +{
> +  if(mMixBlendMode != gfxContext::OPERATOR_SOURCE)
> +      return mMixBlendMode;
> +  for (ContainerLayer* c = GetParent(); c && !c->UseIntermediateSurface();
> +    c = c->GetParent()) {

Indent this to after the ( on the preceding line.

::: gfx/layers/Layers.h
@@ +717,5 @@
> +    if (mMixBlendMode != aMixBlendMode) {
> +      MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) MixBlendMode", this));
> +      mMixBlendMode = aMixBlendMode;
> +      Mutated();
> +    }

You should document that this is currently only supported for BasicLayers, and assert that.

@@ +1063,5 @@
>    const nsIntRect* GetEffectiveClipRect();
>    const nsIntRegion& GetEffectiveVisibleRegion();
> +
> +  // returns the blend mode that should be used for this layer
> +  gfxContext::GraphicsOperator GetEffectiveMixBlendMode();

Why do we need this? It's not clear to me.

::: gfx/layers/basic/BasicContainerLayer.h
@@ +166,5 @@
>     * Having a mask layer always forces our own push group
>     */
>    aContainer->mUseIntermediateSurface =
> +    aContainer->GetMaskLayer() ||
> +      ((aContainer->GetEffectiveOpacity() != 1.0 || aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE)

Fix line breaking.

I don't understand this code. It seems to me you should create an intermediate surface if you have any child layer that uses blending.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +857,5 @@
>                               aPaintContext.mLayer->GetEffectiveVisibleRegion());
>      }
>      BasicContainerLayer* container = static_cast<BasicContainerLayer*>(aPaintContext.mLayer);
>      AutoSetOperator setOperator(aPaintContext.mTarget, container->GetOperator());
>      PaintWithMask(aPaintContext.mTarget, aPaintContext.mLayer->GetEffectiveOpacity(),

We already have code to set the operator here. I think you should pass the correct operator to setOperator, i.e. setting mMixBlendMode as the operator if that's not OVER, otherwise using container->GetOperator(). Might make sense to rename GetOperator/SetOperator to Get/SetOptimizedOperator to make it clearer.

::: gfx/layers/basic/BasicLayersImpl.cpp
@@ +88,3 @@
>        aContext->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> +      if(aMixBlendMode != gfxContext::OPERATOR_OVER)
> +        aContext->SetOperator(aMixBlendMode);

{} around if body (here and below)
Comment on attachment 791019 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

The nsDisplayMixBlendMode class should be moved to the patch where you actually implement blending.

::: layout/base/nsDisplayList.cpp
@@ +3028,5 @@
> +}
> +#endif
> +
> +static gfxContext::GraphicsOperator GetGFXBlendMode(uint8_t mBlendMode) {
> +  switch(mBlendMode) {

Space before (

@@ +3079,5 @@
> +    MOZ_COUNT_DTOR(nsDisplayMixBlendMode);
> +}
> +#endif
> +
> +// nsDisplayOpacity uses layers for rendering

Fix comment.

@@ +3088,5 @@
> +    nsRefPtr<Layer> layer = aManager->GetLayerBuilder()->
> +    BuildContainerLayerFor(aBuilder, aManager, mFrame, this, mList,
> +                           aContainerParameters, nullptr);
> + 
> +    return layer.forget();

No need for "layer", you can just return the the result of BuildContainerLayerFor directly.

::: layout/base/nsDisplayList.h
@@ +2490,5 @@
>  
> +class nsDisplayMixBlendMode : public nsDisplayWrapList {
> +public:
> +
> +    nsDisplayMixBlendMode(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,

Remove blank line

@@ +2508,5 @@
> +    }
> +    virtual bool TryMerge(nsDisplayListBuilder* aBuilder, nsDisplayItem* aItem) MOZ_OVERRIDE
> +    {
> +        // Don't allow merging, each sublist must have its own layer
> +        return false;

You can allow merging, in fact, like opacity, you should. Overlapping CSS boxes from the same element should composite as a single group instead of drawing one over the top of another (I assume).

@@ +2533,5 @@
> +    }
> +    virtual bool TryMerge(nsDisplayListBuilder* aBuilder, nsDisplayItem* aItem) MOZ_OVERRIDE
> +    {
> +        // Don't allow merging, each sublist must have its own layer
> +        return false;

Same here.
Comment on attachment 790685 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/Layers.cpp
@@ +676,5 @@
>  
> +gfxContext::GraphicsOperator
> +Layer::GetEffectiveMixBlendMode()
> +{
> +  if(mMixBlendMode != gfxContext::OPERATOR_SOURCE)

done and fixed up rest of the code as well.

::: gfx/layers/Layers.h
@@ +717,5 @@
> +    if (mMixBlendMode != aMixBlendMode) {
> +      MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) MixBlendMode", this));
> +      mMixBlendMode = aMixBlendMode;
> +      Mutated();
> +    }

How do I check for that?

@@ +1063,5 @@
>    const nsIntRect* GetEffectiveClipRect();
>    const nsIntRegion& GetEffectiveVisibleRegion();
> +
> +  // returns the blend mode that should be used for this layer
> +  gfxContext::GraphicsOperator GetEffectiveMixBlendMode();

Just like with transparency, the layer that draws needs to get the blending from the layers that are above it. I agree it's strange but opacity seems to work like that too.

::: gfx/layers/basic/BasicContainerLayer.h
@@ +166,5 @@
>     * Having a mask layer always forces our own push group
>     */
>    aContainer->mUseIntermediateSurface =
> +    aContainer->GetMaskLayer() ||
> +      ((aContainer->GetEffectiveOpacity() != 1.0 || aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE)

yes. I had that at one point but it broke in certain cases.
However, I think that was most likely because of another issue. I'll change the code back and we can debug it later.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +857,5 @@
>                               aPaintContext.mLayer->GetEffectiveVisibleRegion());
>      }
>      BasicContainerLayer* container = static_cast<BasicContainerLayer*>(aPaintContext.mLayer);
>      AutoSetOperator setOperator(aPaintContext.mTarget, container->GetOperator());
>      PaintWithMask(aPaintContext.mTarget, aPaintContext.mLayer->GetEffectiveOpacity(),

done.
This file will just revert since no changes are needed after that.
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #790685 - Attachment is obsolete: true
Attachment #790685 - Flags: review?(roc)
Attachment #791133 - Flags: review?(roc)
Attachment #791019 - Attachment is obsolete: true
Attachment #791142 - Flags: review?(roc)
(In reply to Rik Cabanier from comment #32)
> ::: gfx/layers/Layers.h
> @@ +717,5 @@
> > +    if (mMixBlendMode != aMixBlendMode) {
> > +      MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) MixBlendMode", this));
> > +      mMixBlendMode = aMixBlendMode;
> > +      Mutated();
> > +    }
> 
> How do I check for that?

I don't understand the question.

> @@ +1063,5 @@
> >    const nsIntRect* GetEffectiveClipRect();
> >    const nsIntRegion& GetEffectiveVisibleRegion();
> > +
> > +  // returns the blend mode that should be used for this layer
> > +  gfxContext::GraphicsOperator GetEffectiveMixBlendMode();
> 
> Just like with transparency, the layer that draws needs to get the blending
> from the layers that are above it. I agree it's strange but opacity seems to
> work like that too.

Opacity is doing an optimization where if we have a ContainerLayer with opacity and only a single child, we just apply the opacity to the child. I don't think we need to do that optimization here ... but I suppose it's OK to do so.
Comment on attachment 791133 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/Layers.cpp
@@ +677,5 @@
> +gfxContext::GraphicsOperator
> +Layer::GetEffectiveMixBlendMode()
> +{
> +  if (mMixBlendMode != gfxContext::OPERATOR_SOURCE)
> +      return mMixBlendMode;

two-space indent.

::: gfx/layers/basic/BasicContainerLayer.h
@@ +161,5 @@
>     * Having a mask layer always forces our own push group
>     */
>    aContainer->mUseIntermediateSurface =
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||

For the optimization I mentioned to be effective, you should call GetEffectiveMixBlendMode and also do a HasMultipleChildren check, just like opacity does.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> The nsDisplayMixBlendMode class should be moved to the patch where you
> actually implement blending.

I think you missed this comment.
Comment on attachment 791133 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/basic/BasicContainerLayer.h
@@ +161,5 @@
>     * Having a mask layer always forces our own push group
>     */
>    aContainer->mUseIntermediateSurface =
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||

I gave it some more thought and agree that GetEffectiveMixBlendMode should not be needed.
I add comments to this function to show that we always make a layer for blending and I stated that the opacity can't be promoted if there's one child and that child is blending.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> > The nsDisplayMixBlendMode class should be moved to the patch where you
> > actually implement blending.
> 
> I think you missed this comment.

Sorry. I uploaded a patch with this class removed.
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #791133 - Attachment is obsolete: true
Attachment #791133 - Flags: review?(roc)
Attachment #791408 - Flags: review?(roc)
Attachment #791142 - Attachment is obsolete: true
Attachment #791142 - Flags: review?(roc)
Attachment #791409 - Flags: review?(roc)
Comment on attachment 791408 [details] [diff] [review]
Part 1: Layers changes

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

You still need GetEffectiveMixBlendMode if you're going to do this optimization.

::: gfx/layers/basic/BasicContainerLayer.h
@@ +155,5 @@
>    aContainer->ComputeEffectiveTransformForMaskLayer(aTransformToSurface);
>  
> +  Layer* child = aContainer->GetFirstChild();
> +  bool hasSingleBlendingChild = false;
> +  if(!aContainer->HasMultipleChildren() && child)

space before (

@@ +156,5 @@
>  
> +  Layer* child = aContainer->GetFirstChild();
> +  bool hasSingleBlendingChild = false;
> +  if(!aContainer->HasMultipleChildren() && child)
> +    hasSingleBlendingChild = child->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE;

{} around if body

@@ +168,4 @@
>     */
> +  aContainer->mUseIntermediateSurface |=
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||

You need to check for the case where the blendmode is being moved down to the single child.
Comment on attachment 791409 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

::: layout/base/nsDisplayItemTypesList.h
@@ +28,5 @@
>  DECLARE_DISPLAY_ITEM_TYPE(FRAMESET_BLANK)
>  DECLARE_DISPLAY_ITEM_TYPE(HEADER_FOOTER)
>  DECLARE_DISPLAY_ITEM_TYPE(IMAGE)
>  DECLARE_DISPLAY_ITEM_TYPE(LIST_FOCUS)
> +DECLARE_DISPLAY_ITEM_TYPE(MIX_BLEND_MODE)

Move this to the patch where we create this display item.

::: layout/base/nsDisplayList.cpp
@@ +3019,5 @@
> +                                                 nsIFrame* aFrame, nsDisplayList* aList,
> +                                                 uint32_t aFlags)
> +    : nsDisplayWrapList(aBuilder, aFrame, aList)
> +{
> +    MOZ_COUNT_CTOR(nsDisplayBlendContainer);

2-space indent here and below

::: layout/base/nsDisplayList.h
@@ +628,5 @@
>  
> +  void SetContainsBlendMode(bool aContainsBlendMode) { mContainsBlendMode = aContainsBlendMode; }
> +  bool ContainsBlendMode() const { return mContainsBlendMode; }
> +
> +

unnecessary blank line

::: layout/generic/nsFrame.cpp
@@ +1746,5 @@
>  
> +class ManageBlendMode
> +{
> +    nsDisplayListBuilder& mBuilder;
> +    bool                  mContainsBlendMode;

2-space indent.

This class should be called AutoSaveRestoreBlendMode.

@@ +1792,5 @@
>    bool isTransformed = IsTransformed();
> +  // reset blend mode so we can keep track if this stacking context needs have
> +  // a nsDisplayBlendContainer. Set the blend mode back when the routine exists
> +  // so we keep track if the parent stacking context needs a container too.
> +  ManageBlendMode manageBlendMode(*aBuilder);

I think we need to clear the contains-blend flag here. For example an element that's a later sibling of a blended element and a stacking context, but contains no blending itself, need not create a layer.

@@ +1974,5 @@
>          new (aBuilder) nsDisplayTransform(aBuilder, this, &resultList));
>      }
>    }
>  
> +  if(aBuilder->ContainsBlendMode()) {

space before (

@@ +1977,5 @@
>  
> +  if(aBuilder->ContainsBlendMode()) {
> +    resultList.AppendNewToTop(
> +      new (aBuilder) nsDisplayBlendContainer(aBuilder, this, &resultList));
> +  }

So first we apply opacity, then we transform, then we blend. Is that what you want?

@@ +2174,5 @@
>  
>    nsDisplayList list;
>    nsDisplayList extraPositionedDescendants;
>    if (isStackingContext) {
> +    if(disp->mMixBlendMode != NS_STYLE_BLEND_NORMAL)

space before (
Comment on attachment 791408 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/basic/BasicContainerLayer.h
@@ +168,4 @@
>     */
> +  aContainer->mUseIntermediateSurface |=
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||

I removed that optimization for now so the blend mode is not being moved.
We can add that back later once blending works and I understand the code better :-)
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #791408 - Attachment is obsolete: true
Attachment #791408 - Flags: review?(roc)
Attachment #792592 - Flags: review?(roc)
Comment on attachment 791409 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

::: layout/generic/nsFrame.cpp
@@ +1792,5 @@
>    bool isTransformed = IsTransformed();
> +  // reset blend mode so we can keep track if this stacking context needs have
> +  // a nsDisplayBlendContainer. Set the blend mode back when the routine exists
> +  // so we keep track if the parent stacking context needs a container too.
> +  ManageBlendMode manageBlendMode(*aBuilder);

Does that not happen on line 1756?
>   mBuilder.SetContainsBlendMode(false);
Comment on attachment 792592 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/basic/BasicContainerLayer.h
@@ +169,5 @@
>     */
> +  aContainer->mUseIntermediateSurface |=
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||
> +    (aContainer->GetEffectiveOpacity() != 1.0 && aContainer->HasMultipleChildren() && !hasSingleBlendingChild);

hasSingleBlendingChild must be false if aContainer->HasMultipleChildren() is true, so the last && is irrelevant and can be dropped.
Comment on attachment 791409 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

::: layout/generic/nsFrame.cpp
@@ +1746,5 @@
>  
> +class ManageBlendMode
> +{
> +    nsDisplayListBuilder& mBuilder;
> +    bool                  mContainsBlendMode;

So call this AutoResetContainsBlendMode.

@@ +1792,5 @@
>    bool isTransformed = IsTransformed();
> +  // reset blend mode so we can keep track if this stacking context needs have
> +  // a nsDisplayBlendContainer. Set the blend mode back when the routine exists
> +  // so we keep track if the parent stacking context needs a container too.
> +  ManageBlendMode manageBlendMode(*aBuilder);

Yes, you're right.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> Comment on attachment 792592 [details] [diff] [review]
> Part 1: Layers changes
> 
> Review of attachment 792592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicContainerLayer.h
> @@ +169,5 @@
> >     */
> > +  aContainer->mUseIntermediateSurface |=
> > +    aContainer->GetMaskLayer() ||
> > +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||
> > +    (aContainer->GetEffectiveOpacity() != 1.0 && aContainer->HasMultipleChildren() && !hasSingleBlendingChild);
> 
> hasSingleBlendingChild must be false if aContainer->HasMultipleChildren() is
> true, so the last && is irrelevant and can be dropped.

Too many double negatives :-)
So, a new layer is needed if there's opacity AND (there are multiple children or the one child has a blend mode)
Attachment #791409 - Attachment is obsolete: true
Attachment #791409 - Flags: review?(roc)
Attachment #792651 - Flags: review?(roc)
Comment on attachment 792651 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

::: gfx/layers/basic/BasicContainerLayer.h
@@ +169,5 @@
>     */
>    aContainer->mUseIntermediateSurface |=
>      aContainer->GetMaskLayer() ||
>      aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||
> +    (aContainer->GetEffectiveOpacity() != 1.0 && (aContainer->HasMultipleChildren() || hasSingleBlendingChild));

This doesn't belong in this patch.

::: layout/generic/nsFrame.cpp
@@ +1751,5 @@
> +public:
> +  AutoSaveRestoreBlendMode(nsDisplayListBuilder& aBuilder)
> +    : mBuilder(aBuilder),
> +      AutoResetContainsBlendMode(aBuilder.ContainsBlendMode()) {
> +    mBuilder.SetContainsBlendMode(false);

It would be clearer if this call moves out to the caller so we can see this is clearing the current state as well as saving and restoring it.
Attachment #792651 - Attachment is obsolete: true
Attachment #792651 - Flags: review?(roc)
Attachment #793031 - Flags: review?(roc)
Comment on attachment 793031 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

The change to BasicContainerLayer.h doesn't belong in this patch.
Attachment #793031 - Attachment is obsolete: true
Attachment #793031 - Flags: review?(roc)
Attachment #793690 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> Comment on attachment 793031 [details] [diff] [review]
> Part 2: Create layers for isolated groups when blending is involved
> 
> Review of attachment 793031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The change to BasicContainerLayer.h doesn't belong in this patch.

argh. Sorry about that. I got my patches mixed up.
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #792592 - Attachment is obsolete: true
Attachment #792592 - Flags: review?(roc)
Attachment #793722 - Flags: review?(roc)
Comment on attachment 793722 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/basic/BasicContainerLayer.h
@@ +156,5 @@
>  
> +  Layer* child = aContainer->GetFirstChild();
> +  bool hasSingleBlendingChild = false;
> +  if (!aContainer->HasMultipleChildren() && child) {
> +    hasSingleBlendingChild = child->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE;

shouldn't this be checking OPERATOR_OVER?

@@ +169,5 @@
>     */
> +  aContainer->mUseIntermediateSurface |=
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||
> +    (aContainer->GetEffectiveOpacity() != 1.0 && (aContainer->HasMultipleChildren() || hasSingleBlendingChild));

Add a comment to say that if the child needs blending, we can't do the optimization to push opacity into the child.
Comment on attachment 793690 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

::: layout/base/nsDisplayList.h
@@ +626,5 @@
>    void SetContainsPluginItem() { mContainsPluginItem = true; }
>    bool ContainsPluginItem() { return mContainsPluginItem; }
>  
> +  void SetContainsBlendMode(bool aContainsBlendMode) { mContainsBlendMode = aContainsBlendMode; }
> +  bool ContainsBlendMode() const { return mContainsBlendMode; }

Need some documentation around these methods. In particular what "contains blend-mode" means. I think it means something like "we're building the contents of a stacking context containing a blend-mode display item for which this stacking context is the nearest enclosing stacking context."

::: layout/generic/nsFrame.cpp
@@ +2173,5 @@
>    nsDisplayList list;
>    nsDisplayList extraPositionedDescendants;
>    if (isStackingContext) {
> +    if (disp->mMixBlendMode != NS_STYLE_BLEND_NORMAL)
> +      aBuilder->SetContainsBlendMode(true);

{} around this statement.
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #793722 - Attachment is obsolete: true
Attachment #793722 - Flags: review?(roc)
Attachment #794415 - Flags: review?(roc)
Comment on attachment 793722 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/Layers.cpp
@@ +173,5 @@
>    mMaskLayer(nullptr),
>    mPostXScale(1.0f),
>    mPostYScale(1.0f),
>    mOpacity(1.0),
> +  mMixBlendMode(gfxContext::OPERATOR_SOURCE),

This one had to change as well.

::: gfx/layers/basic/BasicContainerLayer.h
@@ +156,5 @@
>  
> +  Layer* child = aContainer->GetFirstChild();
> +  bool hasSingleBlendingChild = false;
> +  if (!aContainer->HasMultipleChildren() && child) {
> +    hasSingleBlendingChild = child->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE;

Yes.
I noticed that today but didn't update this patch yet (because nothing is working anymore :-))
Attachment #793690 - Attachment is obsolete: true
Attachment #793690 - Flags: review?(roc)
Attachment #794425 - Flags: review?(roc)
Comment on attachment 794415 [details] [diff] [review]
Part 1: Layers changes

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

::: gfx/layers/basic/BasicContainerLayer.h
@@ +170,4 @@
>     */
> +  aContainer->mUseIntermediateSurface |=
> +    aContainer->GetMaskLayer() ||
> +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||

OPERATOR_OVER
Comment on attachment 794425 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved

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

r+ with those changes

::: layout/base/nsDisplayList.h
@@ +628,5 @@
>  
> +  /**
> +   * mContainsBlendMode is true if we processed a layer that has a
> +   * blend mode attached. We do this so we can insert a 
> +   * nsDisplayBlendContainer in the parent stacking context.

"if we processed a display item"

::: layout/generic/nsFrame.cpp
@@ +1787,5 @@
>  
>    bool inTransform = aBuilder->IsInTransform();
>    bool isTransformed = IsTransformed();
> +  // reset blend mode so we can keep track if this stacking context needs have
> +  // a nsDisplayBlendContainer. Set the blend mode back when the routine exists

"exits"
Attachment #794425 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #62)
> Comment on attachment 794415 [details] [diff] [review]
> Part 1: Layers changes
> 
> Review of attachment 794415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicContainerLayer.h
> @@ +170,4 @@
> >     */
> > +  aContainer->mUseIntermediateSurface |=
> > +    aContainer->GetMaskLayer() ||
> > +    aContainer->GetMixBlendMode() != gfxContext::OPERATOR_SOURCE ||
> 
> OPERATOR_OVER

Thanks! this is probably why the code wasn't working any more.
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #794415 - Attachment is obsolete: true
Attachment #794415 - Flags: review?(roc)
Attachment #795636 - Flags: review?(roc)
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Changed a typo in this patch + brought back 'getEffectiveBlendMode'.
Blending wasn't working without visiting the parent layer.
Attachment #798359 - Flags: review?(roc)
Comment on attachment 798359 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode

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

Why is there no call to Layer::SetMixBlendMode? Shouldn't you be adding that in this patch?

::: layout/base/nsDisplayList.cpp
@@ +2943,5 @@
>    if (!container)
>      return nullptr;
>  
>    container->SetOpacity(mFrame->StyleDisplay()->mOpacity);
> +  

Trailing whitespace. Just remove this hunk

@@ +3016,5 @@
>  }
>  
> +nsDisplayMixBlendMode::nsDisplayMixBlendMode(nsDisplayListBuilder* aBuilder,
> +                                     nsIFrame* aFrame, nsDisplayList* aList,
> +                                     uint32_t aFlags)

Fix indent

@@ +3036,5 @@
> +  return nsRegion();
> +}
> +
> +static gfxContext::GraphicsOperator GetGFXBlendMode(uint8_t mBlendMode) {
> +  switch(mBlendMode) {

Space before (

@@ +3052,5 @@
> +    case NS_STYLE_BLEND_EXCLUSION:   return gfxContext::OPERATOR_EXCLUSION;
> +    case NS_STYLE_BLEND_HUE:         return gfxContext::OPERATOR_HUE;
> +    case NS_STYLE_BLEND_SATURATION:  return gfxContext::OPERATOR_SATURATION;
> +    case NS_STYLE_BLEND_COLOR:       return gfxContext::OPERATOR_COLOR;
> +    case NS_STYLE_BLEND_LUMINOSITY:  return gfxContext::OPERATOR_LUMINOSITY;

add a default: with MOZ_ASSERT(false) and return OPERATOR_OVER

@@ +3054,5 @@
> +    case NS_STYLE_BLEND_SATURATION:  return gfxContext::OPERATOR_SATURATION;
> +    case NS_STYLE_BLEND_COLOR:       return gfxContext::OPERATOR_COLOR;
> +    case NS_STYLE_BLEND_LUMINOSITY:  return gfxContext::OPERATOR_LUMINOSITY;
> +  }
> +  

trailing whitespace

@@ +3062,5 @@
> +// nsDisplayMixBlendMode uses layers for rendering
> +already_AddRefed<Layer>
> +nsDisplayMixBlendMode::BuildLayer(nsDisplayListBuilder* aBuilder,
> +                                   LayerManager* aManager,
> +                                   const ContainerParameters& aContainerParameters) {

Fix indent

::: layout/base/nsDisplayList.h
@@ +2512,5 @@
> +    return mozilla::LAYER_INACTIVE;
> +  }
> +  virtual bool TryMerge(nsDisplayListBuilder* aBuilder, nsDisplayItem* aItem) MOZ_OVERRIDE
> +  {
> +    return false;

Should return true since we want rendering of boxes for the same element with mix-blend-mode to be merged into a single group if possible

::: layout/generic/nsFrame.cpp
@@ +1971,5 @@
>        resultList.AppendNewToTop(
>          new (aBuilder) nsDisplayTransform(aBuilder, this, &resultList));
>      }
>    }
> +  

Don't add trailing whitespace

@@ +1980,5 @@
> +
> +  if (useBlendMode && !resultList.IsEmpty()) {
> +      resultList.AppendNewToTop(
> +        new (aBuilder) nsDisplayMixBlendMode(aBuilder, this, &resultList));
> +    }

Fix indent

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +433,5 @@
>      }
>    }
>  
>    float opacity = aFrame->StyleDisplay()->mOpacity;
> +  uint8_t mixBlendMode = aFrame->StyleDisplay()->mMixBlendMode;

Move this down into the expression, don't need a local variable
Attachment #798359 - Flags: review?(roc) → review+
Comment on attachment 798359 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode

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

Sorry for taking so long to continue the patches. I was on PTO last week (and still am) so couldn't spend much time on this.
It also took me a while to find the silly typo that was creating random results.

::: layout/base/nsDisplayList.h
@@ +2512,5 @@
> +    return mozilla::LAYER_INACTIVE;
> +  }
> +  virtual bool TryMerge(nsDisplayListBuilder* aBuilder, nsDisplayItem* aItem) MOZ_OVERRIDE
> +  {
> +    return false;

Yeah, I remember that you made this remark before. 
However, when I do so, rendering is all wrong. (Basically, nothing before the content with the blend mode is rendered).
Can we keep this as-is for now and debug later?

I will fix all indent issues.
(In reply to Rik Cabanier from comment #70)
> ::: layout/base/nsDisplayList.h
> @@ +2512,5 @@
> > +    return mozilla::LAYER_INACTIVE;
> > +  }
> > +  virtual bool TryMerge(nsDisplayListBuilder* aBuilder, nsDisplayItem* aItem) MOZ_OVERRIDE
> > +  {
> > +    return false;
> 
> Yeah, I remember that you made this remark before. 
> However, when I do so, rendering is all wrong. (Basically, nothing before
> the content with the blend mode is rendered).
> Can we keep this as-is for now and debug later?

I'd rather debug it now. Set MOZ_DUMP_PAINT_LIST=1 in your environment, then you should get a display list dump that should help.
Attached file output with MOZ_DUMP_PAINT_LIST=1 (obsolete) —
It looks like the layers that draw were optimized away
pastebin of patch with computeVisibility: http://pastebin.mozilla.org/2975347
I ducplicated the logic of ComputeVisibility and TryMerge from nsDisplayOpacity to nsDisplayMixBlendMode.

That fixed the problem
Attachment #799947 - Attachment is obsolete: true
Attachment #800368 - Flags: review?(roc)
Comment on attachment 800368 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode

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

::: layout/base/nsDisplayList.cpp
@@ +2943,5 @@
>    if (!container)
>      return nullptr;
>  
>    container->SetOpacity(mFrame->StyleDisplay()->mOpacity);
> +

Remove this hunk

::: layout/generic/nsFrame.cpp
@@ +1971,5 @@
>        resultList.AppendNewToTop(
>          new (aBuilder) nsDisplayTransform(aBuilder, this, &resultList));
>      }
>    }
> +  

Don't add trailing whitespace

@@ +1980,5 @@
> +
> +  if (useBlendMode && !resultList.IsEmpty()) {
> +      resultList.AppendNewToTop(
> +        new (aBuilder) nsDisplayMixBlendMode(aBuilder, this, &resultList));
> +    }

Fix indent.
Attachment #800368 - Flags: review?(roc) → review+
Attachment #798359 - Attachment is obsolete: true
Attachment #800584 - Flags: superreview?(roc)
Attachment #800584 - Flags: superreview?(roc) → review?(roc)
Comment on attachment 800584 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

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

We should have some tests for mix-blend-mode on HTML.

I'd also like to see tests for the stacking context stuff. For example, we should have a test checking that a mix-blend-mode element inside an abs-pos DIV with z-index:0 (a stacking context) actually turns that DIV into an isolated group (i.e., doesn't blend with whatever's under the DIV).

Thanks!!!

::: layout/reftests/svg/blend-color-burn.svg
@@ +9,5 @@
> +    <rect x="10" y="0" width="10" height="40" fill="rgb(0,255,0)"/>
> +    <rect x="20" y="0" width="10" height="40" fill="rgb(0,0,255)"/>
> +    <rect x="30" y="0" width="10" height="40" fill="rgb(127,127,0)"/>
> +  </g>
> +  <rect x="0" y="0" width="10" height="10" fill="rgb(255,0,0)"/>

What's the purpose of this rect?

::: layout/reftests/svg/reftest.list
@@ +366,5 @@
>  == svg-effects-area-zoomed-out.xhtml svg-effects-area-zoomed-out-ref.xhtml
>  == href-attr-change-restyles.svg href-attr-change-restyles-ref.svg
>  == mask-img.html mask-img-ref.html
> +
> +skip-if(d2d) pref(layout.css.mix-blend-mode.enabled,true) == blend-color-burn.svg blend-color-burn-ref.svg

Why are we skipping these for D2D?
Comment on attachment 800584 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

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

There are tests for stacking context, but only within an SVG (ie blend-layer-mask, blend-layer-filter).

Should I add tests that blend SVG with HTML content?

::: layout/reftests/svg/blend-color-burn.svg
@@ +9,5 @@
> +    <rect x="10" y="0" width="10" height="40" fill="rgb(0,255,0)"/>
> +    <rect x="20" y="0" width="10" height="40" fill="rgb(0,0,255)"/>
> +    <rect x="30" y="0" width="10" height="40" fill="rgb(127,127,0)"/>
> +  </g>
> +  <rect x="0" y="0" width="10" height="10" fill="rgb(255,0,0)"/>

No reason. I will remove it.

::: layout/reftests/svg/reftest.list
@@ +366,5 @@
>  == svg-effects-area-zoomed-out.xhtml svg-effects-area-zoomed-out-ref.xhtml
>  == href-attr-change-restyles.svg href-attr-change-restyles-ref.svg
>  == mask-img.html mask-img-ref.html
> +
> +skip-if(d2d) pref(layout.css.mix-blend-mode.enabled,true) == blend-color-burn.svg blend-color-burn-ref.svg

blending wasn't working correctly for d2d.
Maybe with the new changes, it is. I will check.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #78)
> Comment on attachment 800584 [details] [diff] [review]
> step 4: first set of testfiles for SVG blending
> 
> Review of attachment 800584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should have some tests for mix-blend-mode on HTML.
> 
> I'd also like to see tests for the stacking context stuff. For example, we
> should have a test checking that a mix-blend-mode element inside an abs-pos
> DIV with z-index:0 (a stacking context) actually turns that DIV into an
> isolated group (i.e., doesn't blend with whatever's under the DIV).
> 
I made that test case and the display tree was once again optimized too much.
I will debug this. (blending of HTML elements is not expected to work, right?)
Attachment #795637 - Attachment is obsolete: true
Attachment #800582 - Attachment is obsolete: true
Attached patch Part 1: Layers changes (obsolete) — Splinter Review
Attachment #798358 - Attachment is obsolete: true
Attachment #801893 - Attachment is obsolete: true
Attachment #800584 - Attachment is obsolete: true
Attachment #800584 - Flags: review?(roc)
Attachment #794425 - Attachment is obsolete: true
Attachment #795636 - Attachment is obsolete: true
Attachment #800368 - Attachment is obsolete: true
I updated the patches so they can be applied to the latest changes + added a test file to test stacking (as suggested by Comment 78).
Attachment #802661 - Flags: review?(roc)
(In reply to Rik Cabanier from comment #79)
> Should I add tests that blend SVG with HTML content?

Sure.

(In reply to Rik Cabanier from comment #80)
> I made that test case and the display tree was once again optimized too much.
> I will debug this. (blending of HTML elements is not expected to work,
> right?)

Blending HTML should work actually.
Comment on attachment 802661 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

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

::: layout/reftests/svg/blend-color-burn.svg
@@ +10,5 @@
> +    <rect x="20" y="0" width="10" height="40" fill="rgb(0,0,255)"/>
> +    <rect x="30" y="0" width="10" height="40" fill="rgb(127,127,0)"/>
> +  </g>
> +</defs>
> +<g transform="scale(4 4)">

What's the scale for? I assume it's not actually really needed? If so I'd prefer to leave it out, to have simpler test cases.

::: layout/reftests/svg/blend-difference-stacking-ref.html
@@ +1,3 @@
> +<html>
> +<style>
> +#b { 

Removing trailing whitespace

@@ +12,5 @@
> +}
> +</style>
> +<div id="b">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" width="100px" height="100px" viewBox="0 0 100 100" >
> +  <rect  style="fill:#000000;" width="100" height="100"/>

Lose the version attribute. Only one space before 'style'. You don't need the xmlns attribute.

::: layout/reftests/svg/blend-layer-blend.svg
@@ +1,5 @@
> +<!--
> +     Any copyright is dedicated to the Public Domain.
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" width="400px" height="400px" >

Shouldn't this be width="400"?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #87)
> (In reply to Rik Cabanier from comment #79)
> > Should I add tests that blend SVG with HTML content?
> 
> Sure.
> 
> (In reply to Rik Cabanier from comment #80)
> > I made that test case and the display tree was once again optimized too much.
> > I will debug this. (blending of HTML elements is not expected to work,
> > right?)
> 
> Blending HTML should work actually.

It doesn't work. This bug is only about blending SVG. Should we open another one for HTML elements?
Attachment #802661 - Attachment is obsolete: true
Attachment #802661 - Flags: review?(roc)
Attachment #803253 - Flags: review?(roc)
Attachment #803493 - Flags: review?(roc)
Attachment #803494 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #87)
> (In reply to Rik Cabanier from comment #79)
> > Should I add tests that blend SVG with HTML content?
> 
> Sure.
> 
> (In reply to Rik Cabanier from comment #80)
> > I made that test case and the display tree was once again optimized too much.
> > I will debug this. (blending of HTML elements is not expected to work,
> > right?)
> 
> Blending HTML should work actually.

I talked to Matt Woodrow and added another patch as well as a couple of files that exercise the new code.
Comment on attachment 803253 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

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

::: layout/reftests/svg/blend-color-burn-ref.svg
@@ +1,5 @@
> +<!--
> +     Any copyright is dedicated to the Public Domain.
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +--> 
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" width="400" height="400">

Can't we remove the "version" attribute everywhere?

::: layout/reftests/svg/blend-difference-stacking-ref.html
@@ +3,5 @@
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html>
> +<style>
> +#b { 

Remove trailing whitespace (there are several occurrences)
Comment on attachment 803494 [details] [diff] [review]
step 6: some basic test files for HTML blending

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

::: layout/reftests/css-blending/blend-canvas-ref.html
@@ +6,5 @@
> +  <script type="text/javascript">
> +window.onload = function() {
> +	var c = document.getElementById("b");
> +	c.width = 100;
> +	c.height = 100;

Set these in the canvas element markup instead of via script. (below too)

@@ +9,5 @@
> +	c.width = 100;
> +	c.height = 100;
> +	var ctx = c.getContext("2d");
> +	ctx.fillStyle = "rgb(0,0,0)";
> +	ctx.fillRect(0,0,100,100);

Fix indent. (below too)

::: layout/reftests/css-blending/blend-difference-stacking.html
@@ +18,5 @@
> +}
> +#d {
> +  z-index: 1;
> +  position: absolute;
> +  top:100px;

This won't work because of body margins. I'm not sure what you're trying to do here? Is this why the test is commented out?
Attachment #803253 - Attachment is obsolete: true
Attachment #803253 - Flags: review?(roc)
Attachment #803969 - Attachment is obsolete: true
Attachment #803972 - Flags: review?(roc)
Attachment #803972 - Attachment is obsolete: true
Attachment #803972 - Flags: review?(roc)
Attachment #803974 - Flags: review?(roc)
Comment on attachment 803494 [details] [diff] [review]
step 6: some basic test files for HTML blending

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

::: layout/reftests/css-blending/blend-canvas-ref.html
@@ +6,5 @@
> +  <script type="text/javascript">
> +window.onload = function() {
> +	var c = document.getElementById("b");
> +	c.width = 100;
> +	c.height = 100;

done

@@ +9,5 @@
> +	c.width = 100;
> +	c.height = 100;
> +	var ctx = c.getContext("2d");
> +	ctx.fillStyle = "rgb(0,0,0)";
> +	ctx.fillRect(0,0,100,100);

done

::: layout/reftests/css-blending/blend-difference-stacking.html
@@ +18,5 @@
> +}
> +#d {
> +  z-index: 1;
> +  position: absolute;
> +  top:100px;

I'm creating a stacking context.
It is working fine when I open it in the application but for some reason, the script flags it as a different.
Attachment #803494 - Attachment is obsolete: true
Attachment #803494 - Flags: review?(roc)
Attachment #804031 - Attachment is obsolete: true
Attachment #804032 - Flags: review?(roc)
Comment on attachment 803494 [details] [diff] [review]
step 6: some basic test files for HTML blending

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

::: layout/reftests/css-blending/blend-difference-stacking.html
@@ +19,5 @@
> +#d {
> +  z-index: 1;
> +  position: absolute;
> +  top:100px;
> +}

you were right (of course) :-)

Fixed this in the latest patch
The test is disabled until I can figure out how I can turn off subpixel AA in the ref file
Attachment #804245 - Flags: review?(roc)
Attachment #804245 - Attachment is obsolete: true
Attachment #804245 - Flags: review?(roc)
Attachment #804247 - Flags: review?(roc)
Attachment #803974 - Attachment is obsolete: true
Attachment #803974 - Flags: review?(roc)
Attachment #804257 - Flags: review?(roc)
Attachment #804032 - Attachment is obsolete: true
Attachment #804032 - Flags: review?(roc)
Attachment #804258 - Flags: review?(roc)
Attachment #804247 - Attachment is obsolete: true
Attachment #804247 - Flags: review?(roc)
Attachment #804259 - Flags: review?(roc)
Comment on attachment 804259 [details] [diff] [review]
804247: step 7: fix to turn off text anti-aliasing

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

Nice catch!!!

::: layout/reftests/css-blending/reftest.list
@@ +2,5 @@
>  pref(layout.css.mix-blend-mode.enabled,true) == blend-constant-background-color.html blend-constant-background-color-ref.html
>  pref(layout.css.mix-blend-mode.enabled,true) == blend-gradient-background-color.html blend-gradient-background-color-ref.html
>  pref(layout.css.mix-blend-mode.enabled,true) == blend-image.html blend-image-ref.html
>  pref(layout.css.mix-blend-mode.enabled,true) == blend-difference-stacking.html blend-difference-stacking-ref.html
> +#pref(layout.css.mix-blend-mode.enabled,true) == blend-constant-text-color.html blend-constant-text-color-ref.html

Why is the test disabled?
Comment on attachment 804257 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

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

::: layout/reftests/svg/reftest.list
@@ +387,5 @@
> +pref(layout.css.mix-blend-mode.enabled,true) == blend-normal.svg blend-normal-ref.svg
> +# pref(layout.css.mix-blend-mode.enabled,true) == blend-overlay.svg blend-overlay-ref.svg
> +# pref(layout.css.mix-blend-mode.enabled,true) == blend-saturation.svg blend-saturation-ref.svg
> +# pref(layout.css.mix-blend-mode.enabled,true) == blend-screen.svg blend-screen-ref.svg
> +# pref(layout:css.mix-blend-mode.enabled,true) == blend-soft-light.svg blend-soft-light-ref.svg

Why are these tests disabled?
Comment on attachment 804258 [details] [diff] [review]
step 6: some basic test files for HTML blending

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

The rest looks good.

::: layout/reftests/css-blending/blend-difference-stacking.html
@@ +18,5 @@
> +}
> +#d {
> +  z-index: 1;
> +  position: absolute;
> +  top:110px;

I don't understand how this top:110px matches anything in the reference.

@@ +24,5 @@
> +</style>
> +<div id="b" class="a">
> +  <div class="a c"></div>
> +  <div id="d">
> +    <div class="a c"></div>

Why do you have two DIVs using mix-blend-mode? Wouldn't it be simpler to just use one?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #108)
> Comment on attachment 804259 [details] [diff] [review]
> 804247: step 7: fix to turn off text anti-aliasing
> 
> Review of attachment 804259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice catch!!!
> 
> ::: layout/reftests/css-blending/reftest.list
> @@ +2,5 @@
> >  pref(layout.css.mix-blend-mode.enabled,true) == blend-constant-background-color.html blend-constant-background-color-ref.html
> >  pref(layout.css.mix-blend-mode.enabled,true) == blend-gradient-background-color.html blend-gradient-background-color-ref.html
> >  pref(layout.css.mix-blend-mode.enabled,true) == blend-image.html blend-image-ref.html
> >  pref(layout.css.mix-blend-mode.enabled,true) == blend-difference-stacking.html blend-difference-stacking-ref.html
> > +#pref(layout.css.mix-blend-mode.enabled,true) == blend-constant-text-color.html blend-constant-text-color-ref.html
> 
> Why is the test disabled?

I can't figure out a way to disable subpixel AA in the ref file. 
Talked to MattW and others on IRC and there doesn't seem to be a way. Forcing the layer active will do it, but that will disable blending.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #110)
> Comment on attachment 804258 [details] [diff] [review]
> step 6: some basic test files for HTML blending
> 
> Review of attachment 804258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest looks good.
> 
> ::: layout/reftests/css-blending/blend-difference-stacking.html
> @@ +18,5 @@
> > +}
> > +#d {
> > +  z-index: 1;
> > +  position: absolute;
> > +  top:110px;
> 
> I don't understand how this top:110px matches anything in the reference.

It doesn't :-)
The green box with difference blend mode is in a transparent stacking context. When that stacking context is composed on the green box, it disappears.

> 
> @@ +24,5 @@
> > +</style>
> > +<div id="b" class="a">
> > +  <div class="a c"></div>
> > +  <div id="d">
> > +    <div class="a c"></div>
> 
> Why do you have two DIVs using mix-blend-mode? Wouldn't it be simpler to
> just use one?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #110)
> Comment on attachment 804258 [details] [diff] [review]
> step 6: some basic test files for HTML blending
> 
> Review of attachment 804258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest looks good.
> 
> ::: layout/reftests/css-blending/blend-difference-stacking.html
> @@ +18,5 @@
> > +}
> > +#d {
> > +  z-index: 1;
> > +  position: absolute;
> > +  top:110px;
> 
> I don't understand how this top:110px matches anything in the reference.
> 
> @@ +24,5 @@
> > +</style>
> > +<div id="b" class="a">
> > +  <div class="a c"></div>
> > +  <div id="d">
> > +    <div class="a c"></div>
> 
> Why do you have two DIVs using mix-blend-mode? Wouldn't it be simpler to
> just use one?
Yes, I will make that change.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #109)
> Comment on attachment 804257 [details] [diff] [review]
> step 4: first set of testfiles for SVG blending
> 
> Review of attachment 804257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/svg/reftest.list
> @@ +387,5 @@
> > +pref(layout.css.mix-blend-mode.enabled,true) == blend-normal.svg blend-normal-ref.svg
> > +# pref(layout.css.mix-blend-mode.enabled,true) == blend-overlay.svg blend-overlay-ref.svg
> > +# pref(layout.css.mix-blend-mode.enabled,true) == blend-saturation.svg blend-saturation-ref.svg
> > +# pref(layout.css.mix-blend-mode.enabled,true) == blend-screen.svg blend-screen-ref.svg
> > +# pref(layout:css.mix-blend-mode.enabled,true) == blend-soft-light.svg blend-soft-light-ref.svg
> 
> Why are these tests disabled?

Firefox is applying some sort of color management to all the colors. I haven't figured out a way to turn this off.
I tried to make it match manually, but can't get it to work.
I enabled some of the testfiles. Others keep failing because of color management issues
Attachment #804257 - Attachment is obsolete: true
Attachment #804257 - Flags: review?(roc)
Attachment #804754 - Flags: review?(roc)
(In reply to Rik Cabanier from comment #111)
> I can't figure out a way to disable subpixel AA in the ref file. 
> Talked to MattW and others on IRC and there doesn't seem to be a way.
> Forcing the layer active will do it, but that will disable blending.

Just don't test with text then. There's nothing special about text here anyway.
Attachment #804258 - Attachment is obsolete: true
Attachment #804258 - Flags: review?(roc)
Attachment #804757 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #116)
> (In reply to Rik Cabanier from comment #111)
> > I can't figure out a way to disable subpixel AA in the ref file. 
> > Talked to MattW and others on IRC and there doesn't seem to be a way.
> > Forcing the layer active will do it, but that will disable blending.
> 
> Just don't test with text then. There's nothing special about text here
> anyway.
OK. I will remove the test from the patch
(In reply to Rik Cabanier from comment #114)
> Firefox is applying some sort of color management to all the colors. I
> haven't figured out a way to turn this off.
> I tried to make it match manually, but can't get it to work.

I don't think we are doing that. This needs debugging.
Attachment #804259 - Attachment is obsolete: true
Attachment #804259 - Flags: review?(roc)
Attachment #804759 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #119)
> (In reply to Rik Cabanier from comment #114)
> > Firefox is applying some sort of color management to all the colors. I
> > haven't figured out a way to turn this off.
> > I tried to make it match manually, but can't get it to work.
> 
> I don't think we are doing that. This needs debugging.

That should be done in another bug, correct?
(In reply to Rik Cabanier from comment #121)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #119)
> > (In reply to Rik Cabanier from comment #114)
> > > Firefox is applying some sort of color management to all the colors. I
> > > haven't figured out a way to turn this off.
> > > I tried to make it match manually, but can't get it to work.
> > 
> > I don't think we are doing that. This needs debugging.
> 
> That should be done in another bug, correct?

I suppose that's OK.
new patch because of color management issues.
Attachment #804754 - Attachment is obsolete: true
Attachment #804825 - Attachment is obsolete: true
Attachment #801900 - Flags: checkin+
Attachment #802659 - Flags: checkin+
Attachment #802660 - Flags: checkin+
Attachment #803493 - Flags: checkin+
Attachment #804759 - Flags: checkin+
Attachment #804841 - Flags: checkin+
Attachment #804757 - Flags: checkin+
Have you checked this into mozilla-inbound or central then? That's what checkin+ means. 

I suspect you haven't in which case clear the flags and set the checkin-needed keyword on the whole bug instead.

If you have checked in you should add the checkin URL as a comment to the bug like you did for try.
checkin+ means this is a multi-stage bug and I (or someone else) has checked in this bit of it.
(In reply to Robert Longson from comment #126)
> Have you checked this into mozilla-inbound or central then? That's what
> checkin+ means. 
> 
> I suspect you haven't in which case clear the flags and set the
> checkin-needed keyword on the whole bug instead.
> 
> If you have checked in you should add the checkin URL as a comment to the
> bug like you did for try.

ah, sorry. No, I didn't check it in.
I will clear the flags and set the correct one.
Attachment #801900 - Flags: checkin+
Attachment #802659 - Flags: checkin+
Attachment #802660 - Flags: checkin+
Attachment #803493 - Flags: checkin+
Attachment #804757 - Flags: checkin+
Attachment #804759 - Flags: checkin+
Attachment #804841 - Flags: checkin+
Keywords: checkin-needed
These do not apply cleanly to inbound. Please rebase. Also, these patches are missing commit information. Please correct before adding checkin-needed back.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
Attachment #802659 - Attachment is obsolete: true
Attachment #801900 - Attachment is obsolete: true
Attachment #804841 - Attachment is obsolete: true
Attachment #803493 - Attachment is obsolete: true
Attachment #804757 - Attachment is obsolete: true
Attachment #804759 - Attachment is obsolete: true
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #129)
> These do not apply cleanly to inbound. Please rebase. Also, these patches
> are missing commit information. Please correct before adding checkin-needed
> back.
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

I updated the patches so they have the right info and apply cleanly (for now).
Can you try again?
Keywords: checkin-needed
Backed out for B2G reftest failures. Also, when you post updated patches, please fix the issues with the commit messages given above. Also, please add r=roc to them :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/62c338adb0fb

https://tbpl.mozilla.org/php/getParsedLog.php?id=27907260&tree=Mozilla-Inbound
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #139)
> Backed out for B2G reftest failures. Also, when you post updated patches,
> please fix the issues with the commit messages given above. Also, please add
> r=roc to them :)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/62c338adb0fb
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27907260&tree=Mozilla-
> Inbound

Would be nice if the try servers caught this :-(
Attachment #804963 - Attachment is obsolete: true
Attachment #804965 - Attachment is obsolete: true
Keywords: checkin-needed
Let's do one more try run just to be sure.
Keywords: checkin-needed
Attachment #805073 - Attachment is obsolete: true
The try run only shows a failure on blend-difference-stacking.html on B2G; here's a try run with your updated patch with the test skipped on B2G: https://tbpl.mozilla.org/?tree=Try&rev=f9c577687d6f
Summary: Implement support for blending of SVG elements → Implement support for blending of SVG and HTML elements
Blocks: 952643
Whiteboard: [DocArea=CSS][DocArea=HTML][DocArea=SVG]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: