902525 - Implement support for blending of SVG and HTML elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Rik Cabanier]

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

Comment 1

[Rik Cabanier]

Attachment: rev1.patch

Comment 2

[Rik Cabanier]

try run: https://tbpl.mozilla.org/?tree=Try&rev=dc98f7b57c87
Fails on win7 and above. investigating.

Comment 3

[Rik Cabanier]

Clicking on the element causes it to blend as expected. page redraw will show incorrect result again.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is going to require very careful handling of interactions with isolation. Can you explain how you're going to handle that?

Comment 5

[Rik Cabanier]

(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."

Comment 6

[Rik Cabanier]

(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?

Comment 7

[Rik Cabanier]

Attachment: rev_3.patch

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 10

[Rik Cabanier]

Attachment: layers.patch

try server run: https://tbpl.mozilla.org/?tree=Try&rev=241a1f8520fb

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 13

[Robert Longson [:longsonr]]

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.

Comment 14

[Rik Cabanier]

Attachment: layers_2.patch

Comment 15

[Rik Cabanier]

(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

Comment 16

[Rik Cabanier]

Attachment: layers_3.patch

Comment 17

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 18

[Rik Cabanier]

(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

Comment 19

[Rik Cabanier]

Attachment: stacking_1.patch

patch for layerization of blend mode and blend mode container

Comment 20

[Robert Longson [:longsonr]]

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

Comment 21

[Rik Cabanier]

Attachment: stacking_2.patch

Comment 22

[Rik Cabanier]

(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 23

[Robert Longson [:longsonr]]

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.

Comment 24

[Rik Cabanier]

Attachment: stacking_3.patch

Comment 25

[Rik Cabanier]

(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 26

[Robert Longson [:longsonr]]

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.

Comment 27

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's helpful to have your patches be numbered "Part 1", "Part 2" etc so we can see what order they apply in.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 32

[Rik Cabanier]

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.

Comment 33

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 34

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 38

[Rik Cabanier]

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.

Comment 39

[Rik Cabanier]

(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.

Comment 40

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 41

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 44

[Rik Cabanier]

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 :-)

Comment 45

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 46

[Rik Cabanier]

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 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 49

[Rik Cabanier]

(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)

Comment 50

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 52

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 54

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 55

[Rik Cabanier]

(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.

Comment 56

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 59

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 60

[Rik Cabanier]

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 :-))

Comment 61

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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"

Comment 64

[Rik Cabanier]

(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.

Comment 65

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 66

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 67

[Rik Cabanier]

Attachment: Part 1: Layers changes

Changed a typo in this patch + brought back 'getEffectiveBlendMode'.
Blending wasn't working without visiting the parent layer.

Comment 68

[Rik Cabanier]

Attachment: part 3: create a layer for content that stores the blend mode

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 70

[Rik Cabanier]

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.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 72

[Rik Cabanier]

Attachment: output with MOZ_DUMP_PAINT_LIST=1

It looks like the layers that draw were optimized away

Comment 73

[Rik Cabanier]

pastebin of patch with computeVisibility: http://pastebin.mozilla.org/2975347

Comment 74

[Rik Cabanier]

Attachment: part 3: create a layer for content that stores the blend mode

I ducplicated the logic of ComputeVisibility and TryMerge from nsDisplayOpacity to nsDisplayMixBlendMode.

That fixed the problem

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 76

[Rik Cabanier]

Attachment: part 3: create a layer for content that stores the blend mode

Comment 77

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 79

[Rik Cabanier]

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.

Comment 80

[Rik Cabanier]

(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?)

Comment 81

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 82

[Rik Cabanier]

Attachment: part 3: create a layer for content that stores the blend mode

Comment 83

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 84

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 85

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 86

[Rik Cabanier]

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).

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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"?

Comment 89

[Rik Cabanier]

(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?

Comment 90

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 91

[Rik Cabanier]

Attachment: step 5: fixes for blending of HTML elements

Comment 92

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 93

[Rik Cabanier]

(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 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 96

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 97

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 98

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 99

[Rik Cabanier]

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.

Comment 100

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 101

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 102

[Rik Cabanier]

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

Comment 103

[Rik Cabanier]

Attachment: step 7: fix to turn off text anti-aliasing

The test is disabled until I can figure out how I can turn off subpixel AA in the ref file

Comment 104

[Rik Cabanier]

Attachment: step 7: fix to turn off text anti-aliasing

Comment 105

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 106

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 107

[Rik Cabanier]

Attachment: 804247: step 7: fix to turn off text anti-aliasing

Comment 108

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 110

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 111

[Rik Cabanier]

(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.

Comment 112

[Rik Cabanier]

(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?

Comment 113

[Rik Cabanier]

(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.

Comment 114

[Rik Cabanier]

(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.

Comment 115

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

I enabled some of the testfiles. Others keep failing because of color management issues

Comment 116

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 117

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 118

[Rik Cabanier]

(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

Comment 119

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 120

[Rik Cabanier]

Attachment: step 7: fix to turn off text anti-aliasing

Comment 121

[Rik Cabanier]

(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?

Comment 122

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 123

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

new patch because of color management issues.

Comment 124

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 125

[Rik Cabanier]

try server build: https://tbpl.mozilla.org/?tree=Try&rev=fc7673edf36a

Comment 126

[Robert Longson [:longsonr]]

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.

Comment 127

[Robert Longson [:longsonr]]

checkin+ means this is a multi-stage bug and I (or someone else) has checked in this bit of it.

Comment 128

[Rik Cabanier]

(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.

Comment 129

[Ryan VanderMeulen [:RyanVM]]

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

Comment 130

[Rik Cabanier]

Attachment: Part 1: Layers changes

Comment 131

[Rik Cabanier]

Attachment: Part 2: Create layers for isolated groups when blending is involved

Comment 132

[Rik Cabanier]

Attachment: part 3: create a layer for content that stores the blend mode

Comment 133

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 134

[Rik Cabanier]

Attachment: step 5: fixes for blending of HTML elements

Comment 135

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 136

[Rik Cabanier]

Attachment: 804759: step 7: fix to turn off text anti-aliasing

Comment 137

[Rik Cabanier]

(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?

Comment 138

[Ryan VanderMeulen [:RyanVM]]

Pushed with your name added to all 7 patches and consistently using "Part N" instead of a combination of Part/Step.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e553e8da44e
https://hg.mozilla.org/integration/mozilla-inbound/rev/520fcd422150
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef3a516bd68
https://hg.mozilla.org/integration/mozilla-inbound/rev/e562afcb3c89
https://hg.mozilla.org/integration/mozilla-inbound/rev/3137dadb4fcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb386989dbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbca63772c83

Comment 139

[Ryan VanderMeulen [:RyanVM]]

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

Comment 140

[Ryan VanderMeulen [:RyanVM]]

More B2G reftest failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=27907330&tree=Mozilla-Inbound

Comment 141

[Rik Cabanier]

(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 :-(

Comment 142

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 143

[Rik Cabanier]

Attachment: step 6: some basic test files for HTML blending

Comment 144

[Cameron McCormack (:heycam)]

Let's do one more try run just to be sure.

Comment 145

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=a469c16e13bf

Comment 146

[Rik Cabanier]

Attachment: step 4: first set of testfiles for SVG blending

Comment 147

[Cameron McCormack (:heycam)]

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

Comment 148

[Cameron McCormack (:heycam)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9121c28d2d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc6876a6b9b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73412ba47db1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb87122fecef
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df40143705a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9e4f98c445
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/446f75a65963

Comment 149

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9121c28d2d3
https://hg.mozilla.org/mozilla-central/rev/3fc6876a6b9b
https://hg.mozilla.org/mozilla-central/rev/73412ba47db1
https://hg.mozilla.org/mozilla-central/rev/fb87122fecef
https://hg.mozilla.org/mozilla-central/rev/df40143705a8
https://hg.mozilla.org/mozilla-central/rev/2c9e4f98c445
https://hg.mozilla.org/mozilla-central/rev/446f75a65963

Comment 150

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/blend-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/mix-blend-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/background-blend-mode