1209649 - Bad box shadow

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: out.html

Test attached

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Screenshot

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Bad test based on find search bar

Screenshot - https://www.dropbox.com/s/gzbnp5zy6x6dde5/Screenshot%202015-09-29%2020.50.36.png?dl=0

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Take into account border radius size when calculating min inset box shadow

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.

Comment 4

[Mason Chang [Inactive] [:mchang]]

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.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Successful reftest try:

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

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Please add a test for this case.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Visual regression

Comment 8

[Markus Stange [:mstange]]

(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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b03af020cad5

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Box shadow reftest with large border radii

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3d3ff1b8e1d

Comment 11

[Markus Stange [:mstange]]

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.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b03af020cad5

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Box shadow reftest with large border radii

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.

Comment 15

[Mason Chang [Inactive] [:mchang]]

Try for the reftest - https://treeherder.mozilla.org/#/jobs?repo=try&revision=17d1f8d4ffd0

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0ffa0fc4d8

Comment 17

[Phil Ringnalda (:philor)]

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.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e8c075bcad8

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36a4f5a15c1e

Comment 20

[Mason Chang [Inactive] [:mchang]]

I marked the reftest as fuzzy on OS X 10.10. It's a 1 pixel color difference off.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36a4f5a15c1e

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that this is fixed already, I don't see a reason to track this for 44 anymore.