Closed Bug 1209649 Opened 7 years ago Closed 7 years ago

Bad box shadow

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- unaffected
firefox44 - fixed

People

(Reporter: jrmuizel, Assigned: mchang)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(5 files, 1 obsolete file)

Attached file out.html
Test attached
Assignee: nobody → mchang
Status: NEW → ASSIGNED
Whiteboard: gfx-noted
Attached image Screenshot
From attachment 8667468 [details], which is based off the find search bar, we need to take into account the border radius size to ensure that the min inset box shadow is large enough to actually draw the border radius. 

I tried to come up with a reftest for this, but couldn't really think of another way without using a box-shadow / border radius itself. But there really isn't any difference between having the border radius be exactly the right size for the min box shadow or having the border radius be much longer in terms of producing the artifact.
Attachment #8667923 - Flags: review?(mstange)
Comment on attachment 8667923 [details] [diff] [review]
Take into account border radius size when calculating min inset box shadow

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

::: gfx/thebes/gfxBlur.cpp
@@ +952,5 @@
>                   extendBy, minInsetBlur);
>    return minInsetBlur.forget();
>  }
>  
> +

nit: I'll delete this.
See Also: → 1206629
Please add a test for this case.
[Tracking Requested - why for this release]: Visual regression
(In reply to Mason Chang [:mchang] from comment #3)
> I tried to come up with a reftest for this, but couldn't really think of
> another way without using a box-shadow / border radius itself.

It's the middle part that's drawing badly, right? So you could have a test with a border radius and a reference without a border radius (both using box-shadows), and then just compare the middle parts by clipping the outer parts away. You can clip by putting a div around it that has overflow:hidden, and then position things correctly using margins.
Attachment #8667923 - Flags: review?(mstange) → review+
Comment on attachment 8668202 [details] [diff] [review]
Part 2: Box shadow reftest with large border radii

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

Is the bug only reproducible with fractional values?

The leftClip/rightClip elements aren't really clips, they're covers. I hadn't thought of just covering up the edges, but it's a good idea. You should probably rename clip to cover everywhere, and you can remove the overflow:hidden because it doesn't actually do anything.
https://hg.mozilla.org/mozilla-central/rev/b03af020cad5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated with feedback from comment 11.

> Is the bug only reproducible with fractional values?

No, but it's the most glaring difference with the bug. I couldn't get the exact "find search bar" look without it. Instead, I'd get an extra thick border. This test case is from the CSS specified in Firefox's front end in the focused find bar.
Attachment #8668202 - Attachment is obsolete: true
Attachment #8668202 - Flags: review?(mstange)
Attachment #8668460 - Flags: review?(mstange)
Duplicate of this bug: 1210619
Attachment #8668460 - Flags: review?(mstange) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9516a8f8f5 - we haven't yet gotten around to running your test on 10.10 opt, but on 10.10 debug, https://treeherder.mozilla.org/logviewer.html#?job_id=15679163&repo=mozilla-inbound

And no, it doesn't run on try unless you specifically ask for 10.10, and yes, that sucks.
I marked the reftest as fuzzy on OS X 10.10. It's a 1 pixel color difference off.
Given that this is fixed already, I don't see a reason to track this for 44 anymore.
You need to log in before you can comment on or make changes to this bug.