1429411 - Inset box-shadows still render incorrectly in some cases

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Markus Stange [:mstange]]

Attachment: testcase

In this testcase, the shadow is too light at the top and left edges of the box. If the height of the box is reduced to something smaller than 120px, the shadow starts rendering correctly.

Comment 1

[Ethan Lin[:ethlin]]

It's a box shadow offset problem. I just upload a PR 2285[1]. Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5291c42ee1c846566c98e6a33b6b9e1270bea3c2&selectedJob=155567986
The bug 1423633 attachment 8940252 [details] is also fixed by this patch.

[1] https://github.com/servo/webrender/pull/2285

Comment 2

[Markus Stange [:mstange]]

Thanks Ethan!

You can use the tool at http://tests.themasta.com/box-shadow-playground/#transparent_red_inset_3_10_200_200_12x13_8x12_9x11_12x12_4_12_7_0
to check if there are more undiscovered box-shadow bugs.

Comment 3

[Ethan Lin[:ethlin]]

(In reply to Markus Stange [:mstange] from comment #2)
> Thanks Ethan!
> 
> You can use the tool at
> http://tests.themasta.com/box-shadow-playground/
> #transparent_red_inset_3_10_200_200_12x13_8x12_9x11_12x12_4_12_7_0
> to check if there are more undiscovered box-shadow bugs.

Okay, I will check each combination on the website.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Bug 1429411 - Add a reftest for the inset box shadow problem.

Review commit: https://reviewboard.mozilla.org/r/212240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212240/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8942032 [details]
Bug 1429411 - Add a reftest for the inset box shadow problem.

https://reviewboard.mozilla.org/r/212240/#review218130

I'll be moving this patch to the wr update bug. But it would be good to add markus' original test case in this bug as a new reftest in gecko to catch this problem if it happens again in the future.

Comment 6

[Ethan Lin[:ethlin]]

Comment on attachment 8942032 [details]
Bug 1429411 - Add a reftest for the inset box shadow problem.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212240/diff/1-2/

Comment 7

[Ethan Lin[:ethlin]]

Comment on attachment 8942032 [details]
Bug 1429411 - Add a reftest for the inset box shadow problem.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212240/diff/2-3/

Comment 8

[Ethan Lin[:ethlin]]

Actually I tried to add a reftest at first. But gecko seems to be lack of testcases which are really comparing box-shadow blur results because it's hard to generate the reference. So I didn't find a good example to follow. I guess that's why we have some bugs are about the correctness of box-shadow.
Today I tried to use a similar box-shadow as the reference since this problem is about offset. So the test html has a box-shadow with an offset and the reference's box shadow doesn't have the offset. The test can detect the problem and the try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23ee5cf5be3ff74bb271ed93fcf389ee90078fd1&selectedJob=156309060

Comment 9

[Ethan Lin[:ethlin]]

Comment on attachment 8942032 [details]
Bug 1429411 - Add a reftest for the inset box shadow problem.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212240/diff/3-4/

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8942032 [details]
Bug 1429411 - Add a reftest for the inset box shadow problem.

https://reviewboard.mozilla.org/r/212240/#review218594

LGTM

Comment 11

[Darkspirit]

(In reply to Ethan Lin[:ethlin] from comment #1)
> [1] https://github.com/servo/webrender/pull/2285

Thank you!

First tab (https://bugzilla.mozilla.org/attachment.cgi?id=8941412 from comment 0):
It's bad when it's just white on the top and left at 300% zoom.
It's good when it looks like non-WR, especially at 300% zoom.

Second tab (https://bugzilla.mozilla.org/attachment.cgi?id=8940252 from comment 1):
It's bad when it's just white on the right and bottom. There should be a thin grey line (which should be a shadow at 300% zoom).

mozregression --find-fix --bad 2018-01-15 --good 2018-01-26 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8941412|https://bugzilla.mozilla.org/attachment.cgi?id=8940252"
> 9:00.43 INFO: First good revision: 243b7c8aa2e705832288658bd4bfe544cf173262
> 9:00.43 INFO: Last bad revision: c2239403f21ae4c0df28d5457c69b51b966f65cc
> 9:00.43 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c2239403f21ae4c0df28d5457c69b51b966f65cc&tochange=243b7c8aa2e705832288658bd4bfe544cf173262

As expected, fixed by:
> bb1da388dea0	Kartikaya Gupta — Bug 1429806 - Update webrender to commit eb9e1702df4b6dc036b649b3dd32ccc4bfbe43bf. r=jrmuizel


(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
Would you push the reftest? Because ... :(

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46b3e588c06493fb418638eb2a01266c8863a495

Green

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11230e686cee
Add a reftest for the inset box shadow problem. r=kats

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/11230e686cee

Comment 15

[Gary [:streetwolf52]]

According to my build number I should have this patch on my Nightly.  However, the testcase still displays the same way as before the patch.

Comment 16

[Darkspirit]

(In reply to Gary [:streetwolf] from comment #15)
> According to my build number I should have this patch on my Nightly. 
> However, the testcase still displays the same way as before the patch.

The actual fix landed on 2018-01-16 (with bug 1429806).

(from comment #11)
> First tab (https://bugzilla.mozilla.org/attachment.cgi?id=8941412 from  comment 0):
> It's bad when it's just white on the top and left at 300% zoom.
> It's good when it looks like non-WR, especially at 300% zoom.
> 
> Second tab (https://bugzilla.mozilla.org/attachment.cgi?id=8940252 from comment 1):
> It's bad when it's just white on the right and bottom. There should be a thin grey line (which should be a shadow at 300% zoom).

https://mozilla.github.io/mozregression/install.html

Before the fix:
mozregression --repo autoland --launch c2239403f21ae4c0df28d5457c69b51b966f65cc --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8941412|https://bugzilla.mozilla.org/attachment.cgi?id=8940252"

After the fix:
mozregression --repo autoland --launch 243b7c8aa2e705832288658bd4bfe544cf173262 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8941412|https://bugzilla.mozilla.org/attachment.cgi?id=8940252"