1232939 - The background image in a div is causing a spill over of the border where the border-radius cuts into the div

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Flash]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151208100201

Steps to reproduce:

I isolated the problem in the css background style to be the the inclusion of the image here is the code

background:linear-gradient(to right, rgba(0,0,0,.3), rgba(0,0,0,.3)), url(../images/IMGBalloon.jpg) fixed no-repeat top center;





Actual results:

When I removed the gradient, it was still there, but when I removed the image information part the black spill over on the border-radius of the div disappeared, so the problem was connected to the image background.  This div is a dropdown menu

Here is the url: http://flashjohnson.com/GCU/

This was not acting like this before and it is not acting like this in other browsers, this must be a result of a recent update.

Comment 1

[Loic]

What's the exact problem? Could you attach a small testcase (not all the website source code) and some screenshots from IE/Chrome/FF.

Comment 2

[Flash]

Attachment: Screenshots of the problem with some css code examples

Uploading this as requested by: Loic <[e-mail]>

Comment 3

[Loic]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=63cc176d76a66b1807ea4b9500651063032123b1&tochange=d24b11e4beb29e7b0941121b93b10fc155c53a4e

Regressed by bug 1166301.

Botond, can you check the issue, please.

Comment 4

[Markus Stange [:mstange]]

Is this still happening in Nightly? (I don't see anything wrong in Nightly on first sight.)
If not, this was probably fixed by bug 1216580. It looks like we should have uplifted that bug...

Comment 5

[Loic]

Attachment: IMGBalloon.jpg

Comment 6

[Loic]

Attachment: 1232939.html (reduced testcase)

(In reply to Markus Stange [:mstange] from comment #4)
> Is this still happening in Nightly? (I don't see anything wrong in Nightly
> on first sight.)
> If not, this was probably fixed by bug 1216580. It looks like we should have
> uplifted that bug...

Yes, the bug is still here in Nightly. Reduced testcase attached.

Comment 7

[Loic]

We should not see the white corners around border-radius.

Comment 8

[Botond Ballo [:botond]]

I can't repro the problem with today's nightly on Linux, on either the linked site or the reduced testcase. Perhaps it's Windows-only? (That would explain why Markus, who was testing on a Mac, didn't see it either.)

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> I can't repro the problem with today's nightly on Linux, on either the
> linked site or the reduced testcase. Perhaps it's Windows-only? (That would
> explain why Markus, who was testing on a Mac, didn't see it either.)

Actually, maybe that's not the reason.

Loic / Flash, do either of you have e10s disabled? There is a report of a similar issue in bug 1233590 which only occurs with e10s disabled.

I _am_ able to reproduce the problem on the reduced testcase with e10s disabled, though not on the linked site. Interestingly, scrolling the page makes the problem go away for me.

Comment 10

[Loic]

Indeed, with e10s enabled, the problem doesn't occur.

Comment 11

[Botond Ballo [:botond]]

It looks like reproducing the problem also requires APZ to be *enabled* while e10s is disabled. I don't think this is a supported configuration...

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> It looks like reproducing the problem also requires APZ to be *enabled*
> while e10s is disabled. I don't think this is a supported configuration...

Actually, never mind. We should be ignoring the APZ pref if e10s is disabled, so there is a bug somewhere.

Comment 13

[Botond Ballo [:botond]]

Attachment: [Good] Client-side display list and layer dump with textures

Comment 14

[Botond Ballo [:botond]]

Attachment: [Good] Compositor-side layer dump

Comment 15

[Botond Ballo [:botond]]

Attachment: [Good] Composited image

Comment 16

[Botond Ballo [:botond]]

Attachment: [Bad] Client-side display list and layer dump with textures

Comment 17

[Botond Ballo [:botond]]

Attachment: [Bad] Compositor-side layer dump

Comment 18

[Botond Ballo [:botond]]

Attachment: [Bad] Composited image

Comment 19

[Botond Ballo [:botond]]

I uploaded some dumps that might help diagnose the problem. The [Good] dumps were taken with e10s enables, and the [Bad] ones with e10s disabled.

Comment 20

[Botond Ballo [:botond]]

Markus pointed out that the layer that has the mask layer is marked [opaqueContent] in the bad dump, but not in the good dump. This is probably the source of the problem.

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> Markus pointed out that the layer that has the mask layer is marked
> [opaqueContent] in the bad dump, but not in the good dump. This is probably
> the source of the problem.

The reason this happens is that in bug 1166301, we made a change to clip fixed background display items at the layer level rather than the display item level.

ContainerState::ProcessDisplayItems() removes the clip from the fixed-background display item [1], saves it [2], and sets it on the layer later [3].

As a result, opaque rect computation [4] doesn't consider the clip that used to be on the display item. In this case, the clip on the display item represents the rounded corners, so as a result, the opaque region is incorrectly calculated to include the corners when it should exclude them.

[1] https://dxr.mozilla.org/mozilla-central/rev/66fb852962c0d5f6f5fe0604204da4f5d17763c9/layout/base/FrameLayerBuilder.cpp#3916
[2] https://dxr.mozilla.org/mozilla-central/rev/66fb852962c0d5f6f5fe0604204da4f5d17763c9/layout/base/FrameLayerBuilder.cpp#4228
[3] https://dxr.mozilla.org/mozilla-central/rev/66fb852962c0d5f6f5fe0604204da4f5d17763c9/layout/base/FrameLayerBuilder.cpp#3266
[4] https://dxr.mozilla.org/mozilla-central/rev/66fb852962c0d5f6f5fe0604204da4f5d17763c9/layout/base/FrameLayerBuilder.cpp#4214

Comment 22

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/rounded-objects-with-fixed-background-image-are-not-trimmed-properly/

Comment 23

[Botond Ballo [:botond]]

I wrote a patch that applies the saved clip to the opaque region in ContainerState::FinishPaintedLayerData(), and the layer is no longer marked [opaqueContent], but the rendering issue persists, so I guess there's more to the story.

Comment 24

[Botond Ballo [:botond]]

Attachment: Apply the saved clip to the opaque region in FinishPaintedLayerData()

Posting my patch for the record.

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #23)
> I wrote a patch that applies the saved clip to the opaque region in
> ContainerState::FinishPaintedLayerData(), and the layer is no longer marked
> [opaqueContent], but the rendering issue persists, so I guess there's more
> to the story.

I think what's happening is that the damage (the entire corners-included area of the fixed background layer being excised from the visible region of the black background sibling layer) has already been done by the time we fix up the opaque region in FinishPaintedLayerData().

(That theory fits well with why APZ+e10s masks this bug: with APZ+e10s I believe we don't do this sort of content-side culling because layers can be async-translated by the compositor. We do compositor-side culling, but there we exclude layers that have mask layers [1]).

[1] https://dxr.mozilla.org/mozilla-central/rev/66fb852962c0d5f6f5fe0604204da4f5d17763c9/gfx/layers/composite/LayerManagerComposite.cpp#323

Comment 26

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
> I think what's happening is that the damage (the entire corners-included
> area of the fixed background layer being excised from the visible region of
> the black background sibling layer) has already been done by the time we fix
> up the opaque region in FinishPaintedLayerData().

Turns out, the "place where the damage was being done" was in FinishPaintedLayerData(), just a few lines above my additions, where the (incorrect) opaque region was propagated from the PaintedLayerData to the NewLayerEntry. (It was then picked up in ContainerState::PostProcessRetainedLayers(), which is where the occlusion culling happens.)

Moving the code I added up a bit, to happen before the propagation, seems to fix the problem.

Comment 27

[Botond Ballo [:botond]]

I iterated on the patch a bit to handle coordinate conversions correctly, and wrote a test. Posting for review.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e543d1dee47a

Comment 28

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

Review commit: https://reviewboard.mozilla.org/r/29247/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29247/

Comment 29

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #27)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e543d1dee47a

I should probably get some b2g coverage as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e6662508d7

Comment 30

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #27)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e543d1dee47a

On Windows, there are a few 1-pixel, 1-{color value} differences at the edge of the rounded corner. I assume it's reasonable to fuzz this?

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

https://reviewboard.mozilla.org/r/29247/#review26059

I'd really like the test to be reduced a little more. There must be a reason that the presence of the ul and the li affects the rendering, and I'd rather we find that reason than just leave it like that.
(Also, there's strange whitespace in the test.)

::: layout/base/FrameLayerBuilder.cpp:3208
(Diff revision 1)
> +    nsRegion clipRegion = data->mItemClip.ApproximateIntersectInward(clipRect);

ApproximateIntersectInward returns a rect. Let's keep it as a rect all the way until the AndWith call, since both the scale operation and the intersection are cheaper when done on a rect.

Comment 32

[Markus Stange [:mstange]]

(In reply to Botond Ballo [:botond] from comment #30)
> (In reply to Botond Ballo [:botond] from comment #27)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e543d1dee47a
> 
> On Windows, there are a few 1-pixel, 1-{color value} differences at the edge
> of the rounded corner. I assume it's reasonable to fuzz this?

Absolutely. Whenever you're testing anti-aliased rounded corners with different layerization in test and reference, it's fine to fuzz.

Comment 33

[Loic]

I thin there is a similar reduced testcase in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1233590 without ul/li.

Comment 34

[Botond Ballo [:botond]]

(In reply to Loic from comment #33)
> I thin there is a similar reduced testcase in bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=1233590 without ul/li.

Thanks for pointing this out! I initially ignored that testcase because I couldn't reproduce the bug with it normally in the browser, but it does reproduce the bug in the reftest suite, which is what I really need.

Comment 35

[Botond Ballo [:botond]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29247/diff/1-2/

Comment 36

[Botond Ballo [:botond]]

Whoops, looks like you can't have a space between the arguments to fuzzy().

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29247/diff/2-3/

Comment 38

[Botond Ballo [:botond]]

Updated test to use a smaller image, as the large image was causing reftest-analyzer to choke on the reftest output.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c5935f44a1

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29247/diff/3-4/

Comment 40

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #38)
> Updated test to use a smaller image, as the large image was causing
> reftest-analyzer to choke on the reftest output.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c5935f44a1

There are some reftest failures due to the differences larger than the fuzzing I specified, such as "max difference: 36, number of differing pixels: 90". It's not clear to me whether this is still within the acceptable fuzzing limit.

Comment 41

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #40)
> There are some reftest failures due to the differences larger than the
> fuzzing I specified, such as "max difference: 36, number of differing
> pixels: 90". It's not clear to me whether this is still within the
> acceptable fuzzing limit.

After discussing this some more with Markus, we believe the test can be tweaked to avoid this high max difference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7786080d5f9e

Comment 42

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #41)
> After discussing this some more with Markus, we believe the test can be
> tweaked to avoid this high max difference.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7786080d5f9e

Oops, I forgot to make the adjustment to the 'ref' page. Let's try that again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76659299e40b

Comment 43

[Markus Stange [:mstange]]

There's a typo in your patch, by the way: insideRoudnedCornersScaled
                                                   ^^

Comment 44

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #42)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=76659299e40b

Looking much better! Posting updated patch for review.

(In reply to Markus Stange [:mstange] from comment #43)
> There's a typo in your patch, by the way: insideRoudnedCornersScaled
>                                                    ^^

Fixed in the updated patch.

Comment 45

[Botond Ballo [:botond]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29247/diff/4-5/

Comment 46

[Markus Stange [:mstange]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

https://reviewboard.mozilla.org/r/29247/#review26669

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/110fb501fa45

Comment 49

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Even though this is a valid issue, given that we have live with this issue for at least one release, I think this will now be a wontfix for FF44. If I misunderstand the extent to which this can impact Firefox end-users please let me know and I can reconsider.

Comment 50

[G J Piper]

(In reply to Ritu Kothari (:ritu) from comment #49)
> Even though this is a valid issue, given that we have live with this issue
> for at least one release, I think this will now be a wontfix for FF44. If I
> misunderstand the extent to which this can impact Firefox end-users please
> let me know and I can reconsider.

Are you saying this will never be fixed in any upcoming versions? It is a serious bug that has no workaround. Please see this website and let me know what to tell my customer when their site looks completely different in Firefox than it does in IE, Chrome, Safari, and every other browser: http://harborfitllc.com

So if you can get people to eat rotten oranges for at least a week they don't deserve fresh ones anymore since they obviously "live with this issue"? ;-)

Give me a way to work around this in CSS without changing the design and I'll accept that.

Comment 51

[Botond Ballo [:botond]]

(In reply to G J Piper from comment #50)
> Are you saying this will never be fixed in any upcoming versions? It is a
> serious bug that has no workaround.

No one is saying that. The bug will definitely be fixed in Firefox 46, and possibly also in Firefox 45 (pending uplift approval). 

What Ritu was suggesting is not to fix it for Firefox 44. We are in the middle of the beta cycle for Firefox 44, and we tend to be cautious with accepting patches at this point. 

That said, this is not set in stone, either. User reports like yours matter. I will submit a risk assessment shortly. Ritu can then make the final call about Firefox 44 and 45.

Comment 52

[G J Piper]

(In reply to Botond Ballo [:botond] from comment #51)
> (In reply to G J Piper from comment #50)
> > Are you saying this will never be fixed in any upcoming versions? It is a
> > serious bug that has no workaround.
> 
> No one is saying that. The bug will definitely be fixed in Firefox 46, and
> possibly also in Firefox 45 (pending uplift approval). 
> 
> What Ritu was suggesting is not to fix it for Firefox 44. We are in the
> middle of the beta cycle for Firefox 44, and we tend to be cautious with
> accepting patches at this point. 
> 
> That said, this is not set in stone, either. User reports like yours matter.
> I will submit a risk assessment shortly. Ritu can then make the final call
> about Firefox 44 and 45.

Oh, ok. My misunderstanding. Thanks. lol
As long as it will eventually get fixed I have something to tell my customer.

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/110fb501fa45

Comment 54

[Botond Ballo [:botond]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1166301.

[User impact if declined]:
  Elements with a both a fixed background and a border-radius
  can be rendered incorrectly; a background color can be
  rendered behind the corners instead of the element directly
  behind.

[Describe test coverage new/current, TreeHerder]:
  A reftest has been added for the regression.

[Risks and why]: 
  Moderate. The part of the codebase that deals with rendering
  of fixed backgrounds is tricky. We have several reftests
  covering this area, but we still miss regressions, like this
  one, from time to time. It's conceivable that this fix could
  regress some other scenario involving fixed backgrounds that
  is not caught by our existing reftests. It's hard to predict
  how the severity of any such hypothetical regression might
  compare to the severity of this bug.

[String/UUID change made/needed]:
  None

Based on the risk assessment, I'm requesting aurora approval only. If someone feels strongly that the patch should be uplifted to beta as well, please feel free to request that.

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703006 [details]
MozReview Request: Bug 1232939 - Ensure the opaque region of a fixed background layer is correctly clipped. r=mstange

OK, let's try. We can always backout it if it causes more regressions than it fixes.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4e34060f382

Comment 58

[Flash]

I appreciate all the time that all of you are putting into fixing this bug, but I am wondering if it would be better to revert whatever change was made in the first place that caused this bug, would this not be a lot simpler way to resolve it.  From a user point of view I am not aware of any useful functionality that came from whatever that change may have been, but I am very aware of the problems that it has caused with the border-radius display.

Comment 59

[Botond Ballo [:botond]]

(In reply to Flash from comment #58)
> I appreciate all the time that all of you are putting into fixing this bug,
> but I am wondering if it would be better to revert whatever change was made
> in the first place that caused this bug, would this not be a lot simpler way
> to resolve it.

The bug is already fixed.

> From a user point of view I am not aware of any useful
> functionality that came from whatever that change may have been, but I am
> very aware of the problems that it has caused with the border-radius display.

The change that regressed this (bug 1166301) is needed to support background-attachment:fixed with asynchronous scrolling.

Comment 60

[Flash]

Does that mean that the fix is in version 45, because I am using 44 now and it is still present.

Comment 61

[Botond Ballo [:botond]]

(In reply to Flash from comment #60)
> Does that mean that the fix is in version 45, because I am using 44 now and
> it is still present.

Yes. (You can see this in the "Tracking Flags" section at the top of the bug, where "firefox45" and "firefox46" are marked as "fixed".)

For 44 we decided the fix was too risky (see comments 49-55).

Comment 62

[Flash]

Thank you for the clarification.  Appreciate all your work guys.