952051 - [CSS blending] No blending for an element with overflow:hidden and border-radius

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[mire]

Attachment: Parent_with_overflow_hidden.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1751.0 Safari/537.36

Steps to reproduce:

Have a parent element with "overflow: hidden;" and "border-radius: 1em 5em;" and a child element with "mix-blend-mode" other than normal.[Test file - mix-blend-mode-parent-element-overflow-hidden-and-border-radius.html]


Actual results:

No blending between the parent and the child.


Expected results:

Blending expected. [Ref file - reference/mix-blend-mode-blended-element-overflow-hidden-and-border-radius-ref.html]

Comment 1

[mire]

Tested with Nightly 29.0a1 (2013-12-18).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why is this blocking bug 901375?

As far as I can tell, we don't support the mix-blend-mode property by default yet.  Is that all this bug is about?

Comment 3

[mire]

(In reply to Vacation Dec 19 to Jan 1 from comment #2)
> Why is this blocking bug 901375?
> 
> As far as I can tell, we don't support the mix-blend-mode property by
> default yet.  Is that all this bug is about?
I forgot to mention in the bug description: the issue is for Firefox with the flag layout.css.mix-blend-mode.enabled.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Rik, do we have a tracking bug for mix-blend-mode issues that need to be fixed before the pref is flipped?

Comment 5

[Rik Cabanier]

I don't think so. How do I create one?

We need more testing and probably support in the compositor before we can turn it on.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Just file a bug with the summary "Enable mix-blend-mode by default" or something along those lines, and then make it depend on whatever bugs block that.

Comment 7

[Horia Iosif Olaru]

Attachment: border_radius_clipping.patch

This issue happens because when adding border-radius to an element that has overflow:hidden, the content layers are hidden using masks. Without a border radius, the usual layer clipping would be used. The mask adds an intermediate surface (equivalent to isolation) between the blended element and its parent. This is the same limitation as having a stacking context.

The solution I am proposing is to extend layer clipping to be able to accept rounded corner rectangles, and clip the layer at paint time, thus not requiring a mask.

I am attaching a patch that contains a prototype of this implementation, which I would like feedback on. In theory, clipping should be more efficient than masking, but I have not measured this. The patch does not currently address the case when the DisplayItemClip contains more than one clip rect.

I also have a few questions that I have not answered yet, and help is appreciated.
1. Are there cases where this approach would not work?
2. Where should I start looking in order to make sure there are no performance regressions?
3. Do the mochi tests cover regressions on overflow:hidden cases, or should I look at some other test batteries?
4. I did not get a chance to test this approach on other platforms than Mac. I will soon, but is there any reason this would not work with other graphics backends (than Cairo)?
5. What are the cases in which DisplayItemClip contains more than one clip rectangle?

For simplicity, this patch replaces masking with clipping for all elements with rounded borders. This reveals an issue where active layer children are not clipped by the rounded corner. However, clipping works correctly when mix-blend-mode is applied to a child element that should have an active layer (such as having transform:rotateZ(45deg) ). This is because of the way mix-blend-mode is implemented right now. An element with mix-blend-mode goes up the DOM tree to the nearest parent element that creates a stacking context, and forces all its children to be inactive layers.

I see two possible solutions for this. The first is to use clipping instead of masking only for inactive layers, or only when there is a blend mode applied to one of the children of the clipping element, and use masking otherwise. This solution has the drawback that when blending would be integrated with the compositor, and active layers would no longer be forced inactive, it would break.

The second solution would be to distribute the clip rectangle to active child layers, so that they can clip themselves at paint time. This solution would probably be somewhat more complex and error prone to implement.

Let me know which, if any of these solutions is acceptable, or if there are any showstoppers to these approaches that you can foresee.

Comment 8

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

(In reply to Horia Iosif Olaru from comment #7)
> This issue happens because when adding border-radius to an element that has
> overflow:hidden, the content layers are hidden using masks. Without a border
> radius, the usual layer clipping would be used. The mask adds an
> intermediate surface (equivalent to isolation) between the blended element
> and its parent.

Can you be a bit more specific? What do the display list and inactive layer trees look like in this case? I'd expect to see an inactive BasicColorLayer with a mask attached directly to it and a mixblendmode operator set on it. In that case, I'd expect things to work: BasicColorLayer::Paint sets the mixblendmode operator, then calls FillWithMask which clips to the ColorLayer's rectangle and does a Mask operation, the results of which should be blended as required. No intermediate surface involved. So I don't understand what the bug is.

Comment 9

[Horia Iosif Olaru]

Attachment: simple test file

Just comment or uncomment the .parent border-radius property to see the difference.

Comment 10

[Horia Iosif Olaru]

Attachment: no_border_radius_dump.html

Comment 11

[Horia Iosif Olaru]

Attachment: with_border_radius_dump.html

Comment 12

[Horia Iosif Olaru]

I have attached a couple of dump files that resulted from MOZ_DUMP_PAINT_LIST, along with the sample html used (comment/uncomment the border-radius property). The display lists as well as retained layer trees look the same between having or not having border radius.
Is there a similar way to log the inactive layer trees?

What I am seeing is - the inner-most container layer has both a blend mode and a mask layer set on it. It has a BasicColorLayer inactive child which has neither. (If I add extra content to the element with mix-blend-mode - such as another child, or a border, the BasicColorLayer becomes a ThebesLayer, but will still have no mask, and no mixBlendMode).

Because the container layer has an associated mask layer, it will have to use an intermediate surface. The flag is set in BasicContainerLayer::ComputeEffectiveTransforms. This has two consequences that I want to point out:
1. Because of how GetEffectiveMixBlendMode is implemented, the child layers of a container layer with both a mask and mix-blend-mode set will not inherit the blend mode, because the search up the layer tree will stop at the first intermediate surface parent. I still need to understand more about the layer tree to be sure if this behavior is actually wrong.
2. Because a layer with a mask needs to use an intermediate surface, it will create a group.

Within BasicLayerManager::PaintLayer, if no mask is set, the blended container layer will simply call PaintSelfOrChildren, and things will blend normally.

However, when a mask is set, it will first push the group, then call PaintSelfOrChildren, and then pop the group. Because of 1., the children have normal mix-blend-mode, but even if they did inherit the mix-blend-mode, they would still have no backdrop to blend with, because the group does not copy it.
After the group is popped, it is then flushed by FlushGroup. A possible workaround would be to make FlushGroup aware of the blend mode of the container, and use it instead of the graphics context composite operator. However, this approach has issues: 
a) Flush group uses the current context compositing operator. Using the blend-mode operator instead would force the compositing operator to OVER. I am not sure if there is a case now where we would need to use both a blend mode other than normal and composite with something other than OP_OVER at the same time.
b) Because of the way groups are pushed and popped, I am not sure if a group higher on the stack could access content in a lower group. Normally, this would not be an issue since groups should only be created by CSS properties which create a stacking context, so the blending element does not need to access a backdrop outside the group. However, this mask will create a group that does not have an associated stacking context. Therefore, the blending element will need to look 'behind' the group created by the mask. I will try to see if I can obtain some sample HTML that would have this issue.

Comment 13

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

Thanks for all that information. It's very helpful. Sorry about the delay in reply; please use needinfo if you need a quicker reply.

FlushGroup does:
    BasicContainerLayer* container = static_cast<BasicContainerLayer*>(aPaintContext.mLayer);
    AutoSetOperator setOperator(aPaintContext.mTarget, container->GetOperator());
So I think it should do what BasicColorLayer::Paint does:
    gfxContext::GraphicsOperator mixBlendMode = GetEffectiveMixBlendMode();
    AutoSetOperator setOptimizedOperator(aContext, mixBlendMode != gfxContext::OPERATOR_OVER ? mixBlendMode : GetOperator());

Almost every call site to GetOperator currently has to take mixBlendMode into account. There are only two exceptions: the one in FlushGroup, which should be fixed, and the one in BasicThebesLayer::PaintThebes:
    mContentClient->DrawTo(this, aContext->GetDrawTarget(), opacity,
                           CompositionOpForOp(GetOperator()),
                           maskSurface, maskTransform);
which I think needs to remain unchanged. So we should probably just define BasicImplData::GetEffectiveOperator(), which takes mix-blend-mode into account, and have all but one of the GetOperator callsites use it.

(In reply to Horia Iosif Olaru from comment #12)
> b) Because of the way groups are pushed and popped, I am not sure if a group
> higher on the stack could access content in a lower group. Normally, this
> would not be an issue since groups should only be created by CSS properties
> which create a stacking context, so the blending element does not need to
> access a backdrop outside the group. However, this mask will create a group
> that does not have an associated stacking context. Therefore, the blending
> element will need to look 'behind' the group created by the mask. I will try
> to see if I can obtain some sample HTML that would have this issue.

Use of rounded-rect clipping does not cause layer creation, so I don't think this is a problem. We only introduce layer masking when there is already a layer and it needs to be clipped to a rounded rect.

I think that all layers generated by MixBlendMode need to be direct children of the layer for their BlendContainer, otherwise we have a bug. As long as that's true, and we fix the FlushGroup stuff above, rounded-rect clipping shouldn't cause us additional problems.

Comment 14

[Horia Iosif Olaru]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Thanks for all that information. It's very helpful. Sorry about the delay in
> reply; please use needinfo if you need a quicker reply.
> 
> FlushGroup does:
>     BasicContainerLayer* container =
> static_cast<BasicContainerLayer*>(aPaintContext.mLayer);
>     AutoSetOperator setOperator(aPaintContext.mTarget,
> container->GetOperator());
> So I think it should do what BasicColorLayer::Paint does:
>     gfxContext::GraphicsOperator mixBlendMode = GetEffectiveMixBlendMode();
>     AutoSetOperator setOptimizedOperator(aContext, mixBlendMode !=
> gfxContext::OPERATOR_OVER ? mixBlendMode : GetOperator());
> 
> Almost every call site to GetOperator currently has to take mixBlendMode
> into account. There are only two exceptions: the one in FlushGroup, which
> should be fixed, and the one in BasicThebesLayer::PaintThebes:
>     mContentClient->DrawTo(this, aContext->GetDrawTarget(), opacity,
>                            CompositionOpForOp(GetOperator()),
>                            maskSurface, maskTransform);
> which I think needs to remain unchanged. So we should probably just define
> BasicImplData::GetEffectiveOperator(), which takes mix-blend-mode into
> account, and have all but one of the GetOperator callsites use it.
> 
> (In reply to Horia Iosif Olaru from comment #12)
> > b) Because of the way groups are pushed and popped, I am not sure if a group
> > higher on the stack could access content in a lower group. Normally, this
> > would not be an issue since groups should only be created by CSS properties
> > which create a stacking context, so the blending element does not need to
> > access a backdrop outside the group. However, this mask will create a group
> > that does not have an associated stacking context. Therefore, the blending
> > element will need to look 'behind' the group created by the mask. I will try
> > to see if I can obtain some sample HTML that would have this issue.
> 
> Use of rounded-rect clipping does not cause layer creation, so I don't think
> this is a problem. We only introduce layer masking when there is already a
> layer and it needs to be clipped to a rounded rect.
> 
> I think that all layers generated by MixBlendMode need to be direct children
> of the layer for their BlendContainer, otherwise we have a bug. As long as
> that's true, and we fix the FlushGroup stuff above, rounded-rect clipping
> shouldn't cause us additional problems.

I was wondering if there could be a situation in which a masked layer would come between the blended layer and its parent backdrop. This is not an issue for layers that are siblings of the blended layer, just like you said.
I tried to find out if a layer hierarchy similar to the following would be possible:
[P] Parent layer (Backdrop)
  [I] Intermediate layer (child of [P], parent of [B])
  	[B] Blended layer

For this configuration to pose a blending issue, the following assumptions need to be true:
- [I] is not associated with an element that creates a stacking context (and so does not isolate).
- both [I] and [B] overflow [P] in a way that they also need to be clipped with a rounded rect.

This would cause a mask to be added to [I] and implicitly a group to be created. This would have caused incorrect blending.

Since a mask would only be applied to a layer, I looked for display items that create layers. Such display items were almost always associated with a stacking context creating element, so it was okay for their associated layers to create groups.

It seems unlikely there could be a intermediate layer [I] that is not associated with a stacking context. However, if you know of such a situation please let me know.

This also means that future display item implementations should be careful when creating layers. Unless the item is associated with a stacking context creating element, this new layer will break blending in the above scenario.

That being said, I will create a patch replacing GetOperator calls with GetEffectiveOperator. However, GetEffectiveOperator will need to be implemented in Layer (not BasicImplData), because GetEffectiveMixBlend is a Layer method, and needs access to the layer hierarchy. Let me know if this approach is ok.

Comment 15

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

(In reply to Horia Iosif Olaru from comment #14)
> I was wondering if there could be a situation in which a masked layer would
> come between the blended layer and its parent backdrop. This is not an issue
> for layers that are siblings of the blended layer, just like you said.
> I tried to find out if a layer hierarchy similar to the following would be
> possible:
> [P] Parent layer (Backdrop)
>   [I] Intermediate layer (child of [P], parent of [B])
>   	[B] Blended layer

It's a good thing to think about, but I don't think this can happen. We have designed the blend-mode spec so that this should not happen.

> It seems unlikely there could be a intermediate layer [I] that is not
> associated with a stacking context. However, if you know of such a situation
> please let me know.

This should not happen. However, there is one case at the moment: async scrolling. For that, we create an nsDisplayScrollLayer item, which creates a ContainerLayer --- but it is not a stacking context.

However, don't worry about that, because we're going to rewrite the way that works.

> That being said, I will create a patch replacing GetOperator calls with
> GetEffectiveOperator. However, GetEffectiveOperator will need to be
> implemented in Layer (not BasicImplData), because GetEffectiveMixBlend is a
> Layer method, and needs access to the layer hierarchy. Let me know if this
> approach is ok.

That sounds good.

Comment 16

[Horia Iosif Olaru]

Attachment: border-radius-flush-group.patch

Comment 17

[Horia Iosif Olaru]

Attachment: border-radius-flush-group-test.patch

Comment 19

[Horia Iosif Olaru]

Attachment: border-radius-flush-group-test2.patch

I have just marked bug 947121 as a duplicate of this issue (the reasons are explained in that bug). This patch adds a reftest for the behavior described in that issue, that stems from the same root cause.

Comment 20

[Horia Iosif Olaru]

First try run doesn't look so good. The clipping with overflow:hidden test fails on Windows and Linux platforms, and most of the B2G builds fail because of the addition of the GetEffectiveOperator call to BasicCanvasLayer.
https://tbpl.mozilla.org/?tree=Try&rev=c61e04b66fe4

I found the source of some of the reftest failures, and am doing a try run right now with a fix. Not sure how to fix the B2G builds right now though.

Comment 21

[Horia Iosif Olaru]

Attachment: border-radius-flush-group.patch

Fixed build on B2G by adding the missing header.
Fixed blending not working for masked surfaces in cairo, by setting the operator inside DrawSurfaceCairo::MaskSurface. This should make the bug fix work for Linux and Windows as well.

Comment 22

[Horia Iosif Olaru]

Attachment: border-radius-flush-group-test.patch

Made test fuzzy on all platforms, because the same issues related to border radius clipping can be seen everywhere.

Comment 23

[Horia Iosif Olaru]

Attachment: border-radius-flush-group-test2.patch

Rebasing to apply on previous patch.
Also, forgot to make the previous patch obsolete.

Comment 24

[Horia Iosif Olaru]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=8741df7a4d5a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdb998af6c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/67402b120e86

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bdb998af6c1
https://hg.mozilla.org/mozilla-central/rev/67402b120e86