Closed Bug 1211264 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect)

41 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- wontfix
firefox42 + wontfix
firefox43 + wontfix
firefox44 - affected
firefox45 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: 2045singularity, Assigned: mchang)

References

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

Attached image 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.
[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
Component: Untriaged → Graphics
Ever confirmed: true
Flags: needinfo?(mchang)
Keywords: regression
Recent regression, tracking.
Markus, can you help with this bug? Thanks
Assignee: nobody → mchang
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Flags: needinfo?(mchang)
Version: 41 Branch → 44 Branch
Bug 1155828 only landed in Gecko 44.
(In reply to Mason Chang [:mchang] from comment #3)
> Bug 1155828 only landed in Gecko 44.

I think Bug 1155828 was landed in 41.
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.
(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.
This is too late for 42 now.
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.
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)
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)
Hmm, can't really create rects that are pixel aligned in device space due to the rotation.
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)
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.
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+
https://hg.mozilla.org/mozilla-central/rev/16d3dd16ef70
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Will uplift after bug 1217905.
Flags: needinfo?(mchang)
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.
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)
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?
Sure, thanks for checking. Yes, this should either be included by tonight's nightly, if not tonight, then definitely by tomorrow.
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)
(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.
Flags: needinfo?(mchang)
Wontfixing for 43, I think we can let this ride with 45 as ritu suggested.
Duplicate of this bug: 1215868
You need to log in before you can comment on or make changes to this bug.