1211363 - box-shadow with border-radius is broken recently

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Alfred Kayser]

Attachment: boxshadow.html

See attachment for example.

Comment 1

[Alfred Kayser]

In Firefox 41.0.1 and 42.0b3 it works correctly.

Comment 2

[Alfred Kayser]

Narrowed range:
43.0a1 (2015-09-15): Good
43.0a1 (2015-09-18): Good
43.0a1 (2015-09-19): Bad
43.0a1 (2015-09-20): Bad
44.0a1 (2015-09-27): Bad

So, just somewhere before 2015-09-19 release.

Bugs 1097464, 1161913 and 1152298 are examples of checkins that could have caused this

Comment 3

[Markus Stange [:mstange]]

Probably bug 1188075.

Comment 4

[Markus Stange [:mstange]]

I don't think bug 1097464 caused this.

Comment 5

[Alfred Kayser]

Bug 1188075 is the rootcause.

Comment 6

[Markus Stange [:mstange]]

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

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Calculate min inset blur size with max border radius

Thanks for the test case!

The main bug with this was that the border-radius was much larger than the inset blur radius. When we copied the min box shadow to the destination box shadow, we tried to only copy the part from the outer rect. We also miscalculated the margins we should copy to the destination since we didn't always take into account the actual blur extended size versus pre-calculating it based on the blur radius. I think this didn't work in all situations. I rewrote how we calculate the min rects and how we copy the blur to be the same as the outer blur. 

When we copy the blur, we copy the whole blur by creating an extendDestBy and aSlice like with the outer blur. We then calculate the src inner and dest rects with these margins, just like the outer blur. This approach was a lot cleaner.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Looking good reftest try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f07b8c30f4

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8673455 [details] [diff] [review]
Calculate min inset blur size with max border radius

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

r+ assuming this comes with a test

::: gfx/thebes/gfxBlur.cpp
@@ +808,5 @@
>    }
>  
> +  // Create the inner rect to be the smallest possible size based on
> +  // blur / spread / corner radii
> +  IntMargin innerMargin = IntMargin(ceil(cornerHeight) + rectBufferSize.height,

You could remove " = IntMargin" if you want to.

::: layout/base/nsCSSRendering.cpp
@@ +5543,5 @@
>    transformedSkipRect.RoundIn();
>  
>    for (size_t i = 0; i < 4; i++) {
> +    aInnerClipRectRadii[i].width = std::floor(scale.width * aInnerClipRectRadii[i].width);
> +    aInnerClipRectRadii[i].height = std::floor(scale.height * aInnerClipRectRadii[i].height);

Does this match what we did before your patches? Have we always used floor?

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Reftest failure with backed out inset patches

Test case with floating point border radius from bug 1209649 on a build without the inset border patches with this bug. This is a reftest log with the == in reftest.list set to !=. They are equal, but we get a screenshot from what it looks like.

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: Reftest failure with floor border radii + patches

This is the reftest log with the patch in this bug. I just made the reftest != so we'd get a screenshot.

Comment 13

[Mason Chang [Inactive] [:mchang]]

From irc: I talked with mstange about using floor and the data URIs are the same, so we'll go with floor. Will land after creating a test case.

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: reftest based on reporter's test case

Comment 15

[Mason Chang [Inactive] [:mchang]]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a422004b0e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3df3d4856542

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8a422004b0e2
https://hg.mozilla.org/mozilla-central/rev/3df3d4856542

Comment 18

[Alfred Kayser]

Attachment: boxshadow2.html

I am sorry, but the problem is not fixed.
See attached example.

Comment 19

[Alfred Kayser]

Attachment: Screenshot of the problem (nice pattern though!)

Comment 20

[Mason Chang [Inactive] [:mchang]]

(In reply to Alfred Kayser from comment #18)
> Created attachment 8676197 [details]
> boxshadow2.html
> 
> I am sorry, but the problem is not fixed.
> See attached example.

Might be a dupe of 1216200.

Comment 21

[Mason Chang [Inactive] [:mchang]]

(In reply to Alfred Kayser from comment #18)
> Created attachment 8676197 [details]
> boxshadow2.html
> 
> I am sorry, but the problem is not fixed.
> See attached example.

This test case is fixed with the patch in bug 1216200.

Comment 22

[Alfred Kayser]

Attachment: boxshadow3.png

It is not just themes that suffer from this. 
An example from the banking site from the largest bank in the Netherlands.

Comment 23

[Mason Chang [Inactive] [:mchang]]

(In reply to Alfred Kayser from comment #22)
> Created attachment 8677916 [details]
> boxshadow3.png
> 
> It is not just themes that suffer from this. 
> An example from the banking site from the largest bank in the Netherlands.

Which site is this? I'll test again. Thanks for finding test cases. I found other banking sites had this problem as well, such as vanguard.com in the US. Vanguard was fixed by bug 1216200. It hasn't hit nightly yet, so try again tomorrow? Thanks!

Comment 24

[Alfred Kayser]

The site is "https://bankieren.mijn.ing.nl/"
You need an account to login.
But on "https://www.ing.nl/particulier/index.html" (same bank), the search box also had this problem.
But with the currently nightly it is no longer an issue.
The fix from 1216200 does seems to have solved this issue.

Comment 25

[Mason Chang [Inactive] [:mchang]]

From comment 24, resolving as WFM. Thanks for testing!