Closed Bug 1232939 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox42 --- unaffected
firefox43 - wontfix
firefox44 - wontfix
firefox45 + fixed
firefox46 + fixed

People

(Reporter: 77inbox, Assigned: botond)

References

()

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(10 files, 1 obsolete file)

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.
What's the exact problem? Could you attach a small testcase (not all the website source code) and some screenshots from IE/Chrome/FF.
Flags: needinfo?(77inbox)
Keywords: css3testcase-wanted
Summary: The background image in a div is causing a spill over of the border where the border-radius cuts into the div but it is not the image it is just black, this is a new problem, there must have just been some sort of update that introduced this problem → The background image in a div is causing a spill over of the border where the border-radius cuts into the div
Uploading this as requested by: Loic <epinal99-bugzilla2@yahoo.fr>
Flags: needinfo?(77inbox)
[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.
Blocks: 1166301
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Flags: needinfo?(botond)
Product: Firefox → Core
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...
Attached image IMGBalloon.jpg
(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.
We should not see the white corners around border-radius.
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.)
(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.
See Also: → 1233590
Assignee: nobody → botond
Flags: needinfo?(botond)
Indeed, with e10s enabled, the problem doesn't occur.
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...
(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.
Attached file [Bad] Composited image
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.
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.
See Also: → 1230680
(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
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.
Posting my patch for the record.
(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
(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.
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
(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
(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 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.
Attachment #8703006 - Flags: review?(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.
I thin there is a similar reduced testcase in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1233590 without ul/li.
(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 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/
Attachment #8703006 - Flags: review?(mstange)
Whoops, looks like you can't have a space between the arguments to fuzzy().
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/
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 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/
(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.
(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
(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
There's a typo in your patch, by the way: insideRoudnedCornersScaled
                                                   ^^
(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 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 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
Attachment #8703006 - Flags: review?(mstange) → review+
Duplicate of this bug: 1233590
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.
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/110fb501fa45
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Attachment #8700226 - Attachment is obsolete: true
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.
Attachment #8703006 - Flags: approval-mozilla-aurora?
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.
Attachment #8703006 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1245020
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.
(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.
Does that mean that the fix is in version 45, because I am using 44 now and it is still present.
(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).
Thank you for the clarification.  Appreciate all your work guys.
You need to log in before you can comment on or make changes to this bug.