Box-shadow, for img on transform rotate 180deg, creates thin-odd bottom border

RESOLVED FIXED in Firefox 45

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: SU, Assigned: mchang)

Tracking

({regression})

41 Branch
mozilla45
regression
Points:
---

Firefox Tracking Flags

(firefox41 wontfix, firefox42+ wontfix, firefox43+ wontfix, firefox44- affected, firefox45 fixed, firefox-esr38 unaffected)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8669432 [details]
firefox bug.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20150929144111

Steps to reproduce:

Hover over image. A line appears underneath the box shadow. I guess it is a line showing the margin limit, edge, of the blur, but the blur-shadow is not near the margin edge. 360deg or any other deg seems fine. Only 180deg seems to be problem.

https://jsfiddle.net/8y0ghmLa/

It also happens if using flip... transform: scaleY(-1); filter: flipv;

<style>

img.imagine

{
float: right; 
margin: 35px auto 95px auto; 
width: 100%;
height: auto;
border-radius:3px;
box-shadow: 0px 4px 15px -4px #808080;
transition: all 0s !important;
max-height: 400px;
border-width:0px;
}

img.imagine:hover

{
border-radius:5px;
transform: rotate(-180deg);
box-shadow: 0px -4px 15px -4px #808080;
}

</style>

<img class="imagine" src="https://i.imgur.com/e0Gzg1x.png"></img>


Actual results:

A thin line appears, beyond the box shadow, a line not the full width of the image. In the attached screen-shot I put a red box around the line so you can see what I mean. It is a light-grey line, not very easy to see perhaps.


Expected results:

No thin line should appear. Just the box shadow only should appear.

Comment 1

2 years ago
[Tracking Requested - why for this release]: rendering regression


Regressed since Firefox41

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cbe9a2aea954&tochange=5ddf0a252b08

Via local build.
Last Good: a4bcc7ee6e5c + 5ddf0a252b08
First Bad: 28bbd1fb7ed1 + 5ddf0a252b08

Regressed by: 28bbd1fb7ed1	Mason Chang — Bug 1155828 - Draw box-shadows using an approach inspired by border-image. r=mstange
Blocks: 1155828
Status: UNCONFIRMED → NEW
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → unaffected
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
Component: Untriaged → Graphics
Ever confirmed: true
Flags: needinfo?(mchang)
Keywords: regression
Recent regression, tracking.
Markus, can you help with this bug? Thanks
status-firefox41: affected → wontfix
tracking-firefox42: ? → +
tracking-firefox43: ? → +
tracking-firefox44: ? → +
Flags: needinfo?(mstange)
(Assignee)

Updated

2 years ago
Assignee: nobody → mchang
Status: NEW → ASSIGNED
status-firefox41: wontfix → unaffected
status-firefox42: affected → unaffected
status-firefox43: affected → unaffected
tracking-firefox42: + → ---
tracking-firefox43: + → ---
Flags: needinfo?(mstange)
Flags: needinfo?(mchang)
Version: 41 Branch → 44 Branch
(Assignee)

Comment 3

2 years ago
Bug 1155828 only landed in Gecko 44.

Comment 4

2 years ago
(In reply to Mason Chang [:mchang] from comment #3)
> Bug 1155828 only landed in Gecko 44.

I think Bug 1155828 was landed in 41.

Comment 5

2 years ago
I can reproduce on Firefox41.0.1,Beta42.0b5,Aurora43.0a2 and Nightly44.0a1.

So, this is definitely affected to 42, 43 as well as 44.
status-firefox41: unaffected → wontfix
status-firefox42: unaffected → affected
status-firefox43: unaffected → affected
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
Version: 44 Branch → 41 Branch
(Assignee)

Comment 6

2 years ago
(In reply to Alice0775 White from comment #4)
> (In reply to Mason Chang [:mchang] from comment #3)
> > Bug 1155828 only landed in Gecko 44.
> 
> I think Bug 1155828 was landed in 41.

Ahh whoops you're right, I was thinking of a different bug.
Tracking for 43+ since this is a recent regression.
tracking-firefox43: ? → +
This is too late for 42 now.
status-firefox42: affected → wontfix
tracking-firefox42: ? → +
(Assignee)

Comment 9

2 years ago
I can only reproduce this on Windows. On Windows 10 with a hidpi display, it's really difficult to see. On Windows 7 with a non-hidpi display, it's more apparent. It is non-existent on OS X, with or without a retina display. Digging.
(Assignee)

Comment 10

2 years ago
Created attachment 8677572 [details]
Reduced offline test case
(Assignee)

Comment 11

2 years ago
Created attachment 8677758 [details] [diff] [review]
Pretransform dest rects for outer box blurs

The problem with this test case was that we would have a non-integer / non device pixel aligned translation on the destination rect. We'd do the blur on integer rects, then translate the final result to a non pixel aligned location, hence the line. This patch ensures we always pre-transform the dest rects, then set the destination matrix to the identity matrix.
Attachment #8677758 - Flags: review?(mstange)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8677758 [details] [diff] [review]
Pretransform dest rects for outer box blurs

Will try another patch with creating rects that are pixel aligned post transform.
Attachment #8677758 - Flags: review?(mstange)
(Assignee)

Comment 13

2 years ago
Hmm, can't really create rects that are pixel aligned in device space due to the rotation.
(Assignee)

Comment 14

2 years ago
Created attachment 8678402 [details] [diff] [review]
Fallback to rendering dest rect with rotated box shadows

I couldn't really figure out a clean way to draw a rotated box shadow and not have the seams, unless we want to round out the translation on the destination context. Instead, this patch falls back to the old rendering path and just blurs the whole destination rect if we have a rotated blur. It also cleans some code up, which is nice. Although it's odd that this bug only happens on 180 degree rotations. 

We can try the option for rounding out the translation on the destination matrix if you'd prefer that.
Attachment #8678402 - Flags: review?(mstange)
(Assignee)

Comment 15

2 years ago
Created attachment 8678405 [details] [diff] [review]
Fallback to rendering dest rect with rotated box shadows

Updated to include a reftest.
Attachment #8678402 - Attachment is obsolete: true
Attachment #8678402 - Flags: review?(mstange)
Attachment #8678405 - Flags: review?(mstange)
Don't think this is the kind of visual regression that requires tracking.
Comment on attachment 8678405 [details] [diff] [review]
Fallback to rendering dest rect with rotated box shadows

Review of attachment 8678405 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxBlur.cpp
@@ +527,5 @@
>    IntSize minSize =
>      ComputeMinSizeForShadowShape(aCornerRadii, aBlurRadius, aSlice, aRectSize);
>  
> +  // We can get seams using the min size rect when drawing to the destination rect
> +  // If we have a rotation on the destination context. In those cases,

This seam is not caused by the rotation, it's caused by the non-integer translation on the draw target. The non-integer translation causes the dest rects for the box-shadow parts to not be pixel-aligned, so we touch additional pixels on the outside of those rectangles, and the repeat mode filling samples from the opposite side of the source.
So I think, instead of HasNonTranslationOrFlip, you should check whether the current matrix is rectilinear with an integer translation.
(Assignee)

Comment 18

2 years ago
Created attachment 8682652 [details] [diff] [review]
Fallback to rendering dest rect if non int transform on destination context

Updated with feedback from comment 17.
Attachment #8678405 - Attachment is obsolete: true
Attachment #8678405 - Flags: review?(mstange)
Attachment #8682652 - Flags: review?(mstange)
Attachment #8682652 - Flags: review?(mstange) → review+
(Assignee)

Comment 20

2 years ago
Created attachment 8683703 [details] [diff] [review]
Fallback to rendering dest rect if non int transform on dest context

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

Had to add a bit more fuzz to reftests.
Attachment #8682652 - Attachment is obsolete: true
Attachment #8683703 - Flags: review+

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16d3dd16ef70
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Comment 22

2 years ago
Will uplift after bug 1217905.
Flags: needinfo?(mchang)

Comment 23

2 years ago
hi,
this bug seems to be the same that https://bugzilla.mozilla.org/show_bug.cgi?id=1221169
It affects Youtube, which is very frequented.

Can we see this bug (1211262) fixed in the next release of Firefox (which is 43) instead of 45?

Youtube is a major website. Think about the effect on the Firefox users.
(Assignee)

Comment 24

2 years ago
Thanks for the notification. Is the bug fixed for you in nightly? (Not sure the patch made it in today's, but either today's nightly or tomorrows).
Flags: needinfo?(ratm6)

Comment 25

2 years ago
I will look if the bug is fixed in nightly tomorrow, OK?
Are you sure this will be included in tomorrow' update of Firefox Nightly by the way?
(Assignee)

Comment 26

2 years ago
Sure, thanks for checking. Yes, this should either be included by tonight's nightly, if not tonight, then definitely by tomorrow.

Comment 27

2 years ago
I took a look at youtube in nightly at 1:30 PM (Paris time) today and my bug was still visible.
Alice0775 White also noticed that : https://bugzilla.mozilla.org/show_bug.cgi?id=1221840#c8
Should I check again tomorrow?   I suppose not but who knows?
Flags: needinfo?(ratm6)
(Assignee)

Comment 28

2 years ago
(In reply to Julien from comment #27)
> I took a look at youtube in nightly at 1:30 PM (Paris time) today and my bug
> was still visible.
> Alice0775 White also noticed that :
> https://bugzilla.mozilla.org/show_bug.cgi?id=1221840#c8
> Should I check again tomorrow?   I suppose not but who knows?

Thanks for checking. Bug 1221840 is probably a different bug than this one.
Mason and I discussed this bug and looking at the screen snapshot while I agree that this is a valid bug, it still seems minor enough to let it ride the Nightly to Aurora train for 45. The main concern I have with uplifting to Aurora is the patch seems pretty big for an issue that we might be able to live with in 44/43. Please let me know if there are any concerns.
I don't think this needs to be tracked for 44. Please see my previous comment.
tracking-firefox44: + → -
(Assignee)

Updated

2 years ago
Flags: needinfo?(mchang)
Wontfixing for 43, I think we can let this ride with 45 as ritu suggested.
status-firefox43: affected → wontfix
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1215868
You need to log in before you can comment on or make changes to this bug.