Closed
Bug 1211264
Opened 9 years ago
Closed 9 years ago
Box-shadow, for img on transform rotate 180deg, creates thin-odd bottom border
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: 2045singularity, Assigned: mchang)
References
Details
(Keywords: regression)
Attachments
(4 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
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•9 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
Comment 2•9 years ago
|
||
Recent regression, tracking.
Markus, can you help with this bug? Thanks
Flags: needinfo?(mstange)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mchang
Status: NEW → ASSIGNED
tracking-firefox42:
+ → ---
tracking-firefox43:
+ → ---
Flags: needinfo?(mstange)
Flags: needinfo?(mchang)
Version: 41 Branch → 44 Branch
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1155828 only landed in Gecko 44.
Comment 4•9 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•9 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.
Assignee | ||
Comment 6•9 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.
Comment 8•9 years ago
|
||
This is too late for 42 now.
Assignee | ||
Comment 9•9 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•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
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•9 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•9 years ago
|
||
Hmm, can't really create rects that are pixel aligned in device space due to the rotation.
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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 17•9 years ago
|
||
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•9 years ago
|
||
Updated with feedback from comment 17.
Attachment #8678405 -
Attachment is obsolete: true
Attachment #8678405 -
Flags: review?(mstange)
Attachment #8682652 -
Flags: review?(mstange)
Updated•9 years ago
|
Attachment #8682652 -
Flags: review?(mstange) → review+
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 23•9 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•9 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•9 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•9 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•9 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•9 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.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mchang)
Wontfixing for 43, I think we can let this ride with 45 as ritu suggested.
You need to log in
before you can comment on or make changes to this bug.
Description
•