Implement support for blending of SVG and HTML elements

RESOLVED FIXED in mozilla27

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Rik Cabanier, Assigned: Rik Cabanier)

Tracking

({dev-doc-complete})

Trunk
mozilla27
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS][DocArea=HTML][DocArea=SVG])

Attachments

(7 attachments, 55 obsolete attachments)

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

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 901375
Assignee: nobody → cabanier
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

5 years ago
Created attachment 787075 [details] [diff] [review]
rev1.patch
Attachment #787075 - Flags: review?(cam)
Keywords: dev-doc-needed
(Assignee)

Comment 2

5 years ago
try run: https://tbpl.mozilla.org/?tree=Try&rev=dc98f7b57c87
Fails on win7 and above. investigating.
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Comment 5

5 years ago
(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."
(Assignee)

Comment 6

5 years ago
(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?
(Assignee)

Comment 7

5 years ago
Created attachment 790529 [details] [diff] [review]
rev_3.patch
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!
(Assignee)

Updated

5 years ago
Attachment #790612 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 14

5 years ago
Created attachment 790683 [details] [diff] [review]
layers_2.patch
Attachment #790612 - Attachment is obsolete: true
Attachment #790612 - Flags: review?(roc)
Attachment #790683 - Flags: review?(roc)
(Assignee)

Comment 15

5 years ago
(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
(Assignee)

Comment 16

5 years ago
Created attachment 790684 [details] [diff] [review]
layers_3.patch
Attachment #790683 - Attachment is obsolete: true
Attachment #790683 - Flags: review?(roc)
Attachment #790684 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #790684 - Attachment is obsolete: true
Attachment #790684 - Flags: review?(roc)
(Assignee)

Comment 17

5 years ago
Created attachment 790685 [details] [diff] [review]
Part 1: Layers changes
Attachment #790685 - Flags: review?(roc)
(Assignee)

Comment 18

5 years ago
(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
(Assignee)

Comment 19

5 years ago
Created attachment 790697 [details] [diff] [review]
stacking_1.patch

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

Comment 21

5 years ago
Created attachment 790831 [details] [diff] [review]
stacking_2.patch
Attachment #790697 - Attachment is obsolete: true
Attachment #790697 - Flags: review?(roc)
Attachment #790831 - Flags: review?(longsonr)
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 24

5 years ago
Created attachment 790932 [details] [diff] [review]
stacking_3.patch
Attachment #790831 - Attachment is obsolete: true
Attachment #790831 - Flags: review?(longsonr)
Attachment #790932 - Flags: review?(longsonr)
(Assignee)

Comment 25

5 years ago
(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+
(Assignee)

Updated

5 years ago
Attachment #790685 - Flags: review?(roc) → review?(longsonr)
(Assignee)

Comment 27

5 years ago
Created attachment 791019 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 32

5 years ago
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.
(Assignee)

Comment 33

5 years ago
Created attachment 791133 [details] [diff] [review]
Part 1: Layers changes
Attachment #790685 - Attachment is obsolete: true
Attachment #790685 - Flags: review?(roc)
Attachment #791133 - Flags: review?(roc)
(Assignee)

Comment 34

5 years ago
Created attachment 791142 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
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.
(Assignee)

Comment 38

5 years ago
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.
(Assignee)

Comment 39

5 years ago
(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.
(Assignee)

Comment 40

5 years ago
Created attachment 791408 [details] [diff] [review]
Part 1: Layers changes
Attachment #791133 - Attachment is obsolete: true
Attachment #791133 - Flags: review?(roc)
Attachment #791408 - Flags: review?(roc)
(Assignee)

Comment 41

5 years ago
Created attachment 791409 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
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 (
(Assignee)

Comment 44

5 years ago
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 :-)
(Assignee)

Comment 45

5 years ago
Created attachment 792592 [details] [diff] [review]
Part 1: Layers changes
Attachment #791408 - Attachment is obsolete: true
Attachment #791408 - Flags: review?(roc)
Attachment #792592 - Flags: review?(roc)
(Assignee)

Comment 46

5 years ago
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.
(Assignee)

Comment 49

5 years ago
(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)
(Assignee)

Comment 50

5 years ago
Created attachment 792651 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
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.
(Assignee)

Comment 52

5 years ago
Created attachment 793031 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
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.
(Assignee)

Comment 54

5 years ago
Created attachment 793690 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
Attachment #793031 - Attachment is obsolete: true
Attachment #793031 - Flags: review?(roc)
Attachment #793690 - Flags: review?(roc)
(Assignee)

Comment 55

5 years ago
(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.
(Assignee)

Comment 56

5 years ago
Created attachment 793722 [details] [diff] [review]
Part 1: Layers changes
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.
(Assignee)

Comment 59

5 years ago
Created attachment 794415 [details] [diff] [review]
Part 1: Layers changes
Attachment #793722 - Attachment is obsolete: true
Attachment #793722 - Flags: review?(roc)
Attachment #794415 - Flags: review?(roc)
(Assignee)

Comment 60

5 years ago
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 :-))
(Assignee)

Comment 61

5 years ago
Created attachment 794425 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
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+
(Assignee)

Comment 64

5 years ago
(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.
(Assignee)

Comment 65

5 years ago
Created attachment 795636 [details] [diff] [review]
Part 1: Layers changes
Attachment #794415 - Attachment is obsolete: true
Attachment #794415 - Flags: review?(roc)
Attachment #795636 - Flags: review?(roc)
(Assignee)

Comment 66

5 years ago
Created attachment 795637 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
(Assignee)

Comment 67

5 years ago
Created attachment 798358 [details] [diff] [review]
Part 1: Layers changes

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

Comment 68

5 years ago
Created attachment 798359 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode
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+
(Assignee)

Comment 70

5 years ago
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.
(Assignee)

Comment 72

5 years ago
Created attachment 799947 [details]
output with MOZ_DUMP_PAINT_LIST=1

It looks like the layers that draw were optimized away
(Assignee)

Comment 73

5 years ago
pastebin of patch with computeVisibility: http://pastebin.mozilla.org/2975347
(Assignee)

Comment 74

5 years ago
Created attachment 800368 [details] [diff] [review]
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
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+
(Assignee)

Comment 76

5 years ago
Created attachment 800582 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode
Attachment #798359 - Attachment is obsolete: true
(Assignee)

Comment 77

5 years ago
Created attachment 800584 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #800584 - Flags: superreview?(roc)
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 79

5 years ago
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.
(Assignee)

Comment 80

5 years ago
(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?)
(Assignee)

Comment 81

5 years ago
Created attachment 801893 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
Attachment #795637 - Attachment is obsolete: true
(Assignee)

Comment 82

5 years ago
Created attachment 801900 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode
Attachment #800582 - Attachment is obsolete: true
(Assignee)

Comment 83

5 years ago
Created attachment 802659 [details] [diff] [review]
Part 1: Layers changes
Attachment #798358 - Attachment is obsolete: true
(Assignee)

Comment 84

5 years ago
Created attachment 802660 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
Attachment #801893 - Attachment is obsolete: true
(Assignee)

Comment 85

5 years ago
Created attachment 802661 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #800584 - Attachment is obsolete: true
Attachment #800584 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #794425 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #795636 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #800368 - Attachment is obsolete: true
(Assignee)

Comment 86

5 years ago
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).
(Assignee)

Updated

5 years ago
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"?
(Assignee)

Comment 89

5 years ago
(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?
(Assignee)

Comment 90

5 years ago
Created attachment 803253 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #802661 - Attachment is obsolete: true
Attachment #802661 - Flags: review?(roc)
Attachment #803253 - Flags: review?(roc)
(Assignee)

Comment 91

5 years ago
Created attachment 803493 [details] [diff] [review]
step 5: fixes for blending of HTML elements
Attachment #803493 - Flags: review?(roc)
(Assignee)

Comment 92

5 years ago
Created attachment 803494 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #803494 - Flags: review?(roc)
(Assignee)

Comment 93

5 years ago
(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?
(Assignee)

Comment 96

5 years ago
Created attachment 803969 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #803253 - Attachment is obsolete: true
Attachment #803253 - Flags: review?(roc)
(Assignee)

Comment 97

5 years ago
Created attachment 803972 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #803969 - Attachment is obsolete: true
Attachment #803972 - Flags: review?(roc)
(Assignee)

Comment 98

5 years ago
Created attachment 803974 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #803972 - Attachment is obsolete: true
Attachment #803972 - Flags: review?(roc)
Attachment #803974 - Flags: review?(roc)
(Assignee)

Comment 99

5 years ago
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.
(Assignee)

Comment 100

5 years ago
Created attachment 804031 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #803494 - Attachment is obsolete: true
Attachment #803494 - Flags: review?(roc)
(Assignee)

Comment 101

5 years ago
Created attachment 804032 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #804031 - Attachment is obsolete: true
Attachment #804032 - Flags: review?(roc)
(Assignee)

Comment 102

5 years ago
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
(Assignee)

Comment 103

5 years ago
Created attachment 804245 [details] [diff] [review]
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
Attachment #804245 - Flags: review?(roc)
(Assignee)

Comment 104

5 years ago
Created attachment 804247 [details] [diff] [review]
step 7: fix to turn off text anti-aliasing
Attachment #804245 - Attachment is obsolete: true
Attachment #804245 - Flags: review?(roc)
Attachment #804247 - Flags: review?(roc)
(Assignee)

Comment 105

5 years ago
Created attachment 804257 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #803974 - Attachment is obsolete: true
Attachment #803974 - Flags: review?(roc)
Attachment #804257 - Flags: review?(roc)
(Assignee)

Comment 106

5 years ago
Created attachment 804258 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #804032 - Attachment is obsolete: true
Attachment #804032 - Flags: review?(roc)
Attachment #804258 - Flags: review?(roc)
(Assignee)

Comment 107

5 years ago
Created attachment 804259 [details] [diff] [review]
804247: step 7: fix to turn off text anti-aliasing
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?
(Assignee)

Comment 111

5 years ago
(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.
(Assignee)

Comment 112

5 years ago
(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?
(Assignee)

Comment 113

5 years ago
(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.
(Assignee)

Comment 114

5 years ago
(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.
(Assignee)

Comment 115

5 years ago
Created attachment 804754 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

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

Comment 117

5 years ago
Created attachment 804757 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #804258 - Attachment is obsolete: true
Attachment #804757 - Flags: review?(roc)
Attachment #804258 - Flags: review?(roc)
(Assignee)

Comment 118

5 years ago
(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.
(Assignee)

Comment 120

5 years ago
Created attachment 804759 [details] [diff] [review]
step 7: fix to turn off text anti-aliasing
Attachment #804259 - Attachment is obsolete: true
Attachment #804259 - Flags: review?(roc)
Attachment #804759 - Flags: review?(roc)
(Assignee)

Comment 121

5 years ago
(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.
(Assignee)

Comment 123

5 years ago
Created attachment 804825 [details] [diff] [review]
step 4: first set of testfiles for SVG blending

new patch because of color management issues.
Attachment #804754 - Attachment is obsolete: true
(Assignee)

Comment 124

5 years ago
Created attachment 804841 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #804825 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #801900 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #802659 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #802660 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #803493 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #804759 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #804841 - Flags: checkin+
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 128

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #801900 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #802659 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #802660 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #803493 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #804757 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #804759 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #804841 - Flags: checkin+
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 130

5 years ago
Created attachment 804960 [details] [diff] [review]
Part 1: Layers changes
Attachment #802659 - Attachment is obsolete: true
(Assignee)

Comment 131

5 years ago
Created attachment 804961 [details] [diff] [review]
Part 2: Create layers for isolated groups when blending is involved
Attachment #802660 - Attachment is obsolete: true
(Assignee)

Comment 132

5 years ago
Created attachment 804962 [details] [diff] [review]
part 3: create a layer for content that stores the blend mode
Attachment #801900 - Attachment is obsolete: true
(Assignee)

Comment 133

5 years ago
Created attachment 804963 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #804841 - Attachment is obsolete: true
(Assignee)

Comment 134

5 years ago
Created attachment 804964 [details] [diff] [review]
step 5: fixes for blending of HTML elements
Attachment #803493 - Attachment is obsolete: true
(Assignee)

Comment 135

5 years ago
Created attachment 804965 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #804757 - Attachment is obsolete: true
(Assignee)

Comment 136

5 years ago
Created attachment 804966 [details] [diff] [review]
804759: step 7: fix to turn off text anti-aliasing
Attachment #804759 - Attachment is obsolete: true
(Assignee)

Comment 137

5 years ago
(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?
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 141

5 years ago
(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 :-(
(Assignee)

Comment 142

5 years ago
Created attachment 805073 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
Attachment #804963 - Attachment is obsolete: true
(Assignee)

Comment 143

5 years ago
Created attachment 805076 [details] [diff] [review]
step 6: some basic test files for HTML blending
Attachment #804965 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Let's do one more try run just to be sure.
Keywords: checkin-needed
(Assignee)

Comment 146

5 years ago
Created attachment 805137 [details] [diff] [review]
step 4: first set of testfiles for SVG blending
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
(Assignee)

Updated

5 years ago
Summary: Implement support for blending of SVG elements → Implement support for blending of SVG and HTML elements
(Assignee)

Updated

4 years ago
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.