Masks incorrectly apply to masked contents individually instead of to the "transparency group"

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: mstange, Assigned: Gankro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected, firefox61 fixed)

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Posted file testcase
The CSS mask property creates a stacking context and a conceptual intermediate surface. The masking operation needs to be performed with that intermediate surface as its input, analogous to what you'd do with an opacity group.

However, with webrender, it seems there is no intermediate surface, and instead the mask gets applied to the contents individually. This is incorrect.
Whiteboard: [wr-mvp] [triage]
Blocks: 1386669
status-firefox57: --- → unaffected
status-firefox58: affected → unaffected
Priority: -- → P3
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][wr-reserve-candidate]
Whiteboard: [wr-mvp] [triage][wr-reserve-candidate] → [wr-reserve] [gfx-noted]
Assignee: nobody → a.beingessner
(Assignee)

Updated

a year ago
Blocks: 1432101
(Assignee)

Comment 1

a year ago
This is being actively discussed in the upstream tracking issue https://github.com/servo/webrender/issues/1957
Comment hidden (mozreview-request)
(Reporter)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8967081 [details]
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.

https://reviewboard.mozilla.org/r/235736/#review241524

Looks good! It'll be interesting to see how our tests react to this.

::: layout/painting/nsDisplayList.cpp:9665
(Diff revision 1)
> -    // otherwise any nested ScrollingLayersHelper instances that are created
> -    // will get confused about which clips are pushed.
> -    aBuilder.PushClip(clipId, GetClipChain());
> +    // Create a new stacking context to attach the mask to, ensuring effects
> +    // like opacity are applied to the aggregate, and not the individual
> +    // elements.

I'd replace "effects like opacity are" with "the mask is".
Attachment #8967081 - Flags: review?(mstange) → review+

Comment 5

11 months ago
mozreview-review
Comment on attachment 8967081 [details]
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.

https://reviewboard.mozilla.org/r/235736/#review241526

::: layout/painting/nsDisplayList.cpp:9664
(Diff revision 1)
>                                                                              bounds);
>    if (mask) {
> +    auto layoutBounds = wr::ToRoundedLayoutRect(bounds);
>      wr::WrClipId clipId = aBuilder.DefineClip(Nothing(), Nothing(),
> -        wr::ToRoundedLayoutRect(bounds), nullptr, mask.ptr());
> -    // Don't record this clip push in aBuilder's internal clip stack, because
> +        layoutBounds, nullptr, mask.ptr());
> +    

nit: some trailing whitespace here
Comment hidden (mozreview-request)
(Assignee)

Comment 7

11 months ago
nits addressed.
(Assignee)

Comment 8

11 months ago
tons of regressions, but also a bunch of new passes!

Applying the layoutRect blindly to the stacking context was wrong. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=166851105edfaa1df2e52e05beb47dc7c116aa9b
Note that the "new passes" are just reductions in fuzziness for tests that are already fuzzy-if.
(Assignee)

Comment 10

11 months ago
Ok the new try looks a lot better. All the failures are just fuzziness increases (seems like we're producing lower quality AA on a bunch of circles?)

No new passes, which means I'll need to writeup a reftest based on markus' test.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

11 months ago
Some further analysis: 

* there's actually unexpected passes in wpt (hooray!)
* markus seems to think the fuzziness changes might actually be a concern, possibly a sign of double-clipping. kats, what do you think?
Flags: needinfo?(bugmail)
(Assignee)

Comment 13

11 months ago
Actually if we were double-clipping wouldn't we get double transparency? This would mean markus' test would still fail, as the you would see the green through the blue (which I don't locally).
Yeah if it were double-clipping I would expect mask-opacity-1a.html to fail, but it looks like that's doing better now (the wpt version is passing, and the non-wpt version is the same as before). Overall this patch looks like an improvement to me - the slight increase in fuzziness I think is basically negligible and the passes in wpt are good.

So I'd be ok with landing it with the appropriate test annotation changes.
Flags: needinfo?(bugmail)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8967138 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 17

11 months ago
mozreview-review
Comment on attachment 8967151 [details]
Bug 1412375 - Add reftest for atomicity of masks.

https://reviewboard.mozilla.org/r/235798/#review241606

Do you have a try push with this? In particular I want to ensure that we make the fuzzy ranges as tight as possible, and that this actually passes on all platforms. A try push with test-verify jobs should run just this test.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

11 months ago
try looks good, expanded the fuzzy range to 1-2 (locally I got 1, try unanimously wants 2).

Comment 25

11 months ago
mozreview-review
Comment on attachment 8967081 [details]
Bug 1412375 - Create a clipped stacking context for nsDisplayMasks.

https://reviewboard.mozilla.org/r/235736/#review241736
Attachment #8967081 - Flags: review+

Comment 26

11 months ago
mozreview-review
Comment on attachment 8967151 [details]
Bug 1412375 - Add reftest for atomicity of masks.

https://reviewboard.mozilla.org/r/235798/#review241740

::: layout/reftests/bugs/reftest.list:2069
(Diff revision 2)
>  fuzzy-if(Android,66,574) fuzzy-if(d2d,89,777) fuzzy-if(!Android&&!d2d,1,31219) == 1425243-2.html 1425243-2-ref.html
>  == 1432541.html 1432541-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,true) == 1446470.html 1035091-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,false) == 1446470-2.html 1035091-ref.html
>  test-pref(layout.css.prefixes.gradients,false) == 1451874.html 1451874-ref.html
> +fuzzy(1-2,17500) == 1412375.html 1412375-ref.html

Let's make it 1-2,17500-17500
Attachment #8967151 - Flags: review?(bugmail) → review+

Comment 27

11 months ago
mozreview-review
Comment on attachment 8967153 [details]
Bug 1412375 - adjust test expectations for better masking

https://reviewboard.mozilla.org/r/235804/#review241742

This looks good, thanks. I added linux-qr reftest jobs to your try push just to make sure those are all green too (I expect that to be the case since the windows ones were, but I like to be safe with stuff like this).
Attachment #8967153 - Flags: review?(bugmail) → review+

Comment 28

11 months ago
mozreview-review
Comment on attachment 8967151 [details]
Bug 1412375 - Add reftest for atomicity of masks.

https://reviewboard.mozilla.org/r/235798/#review241768

::: layout/reftests/bugs/reftest.list:2069
(Diff revision 2)
>  fuzzy-if(Android,66,574) fuzzy-if(d2d,89,777) fuzzy-if(!Android&&!d2d,1,31219) == 1425243-2.html 1425243-2-ref.html
>  == 1432541.html 1432541-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,true) == 1446470.html 1035091-ref.html
>  pref(layout.css.moz-document.url-prefix-hack.enabled,false) == 1446470-2.html 1035091-ref.html
>  test-pref(layout.css.prefixes.gradients,false) == 1451874.html 1451874-ref.html
> +fuzzy(1-2,17500) == 1412375.html 1412375-ref.html

Actually it looks like it's passing perfectly on linux-qr. So we should one of the following:

fuzzy(0-2,0-17500)
fuzzy-if(!(webrender&&gtkWidget),1-2,17500-17500)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 31

11 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edadd89f89fd
Create a clipped stacking context for nsDisplayMasks. r=kats,mstange
https://hg.mozilla.org/integration/autoland/rev/68b719bed991
Add reftest for atomicity of masks. r=kats
https://hg.mozilla.org/integration/autoland/rev/f4ccd608d691
adjust test expectations for better masking r=kats
Keywords: checkin-needed

Comment 32

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edadd89f89fd
https://hg.mozilla.org/mozilla-central/rev/68b719bed991
https://hg.mozilla.org/mozilla-central/rev/f4ccd608d691
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Reporter)

Updated

11 months ago
Depends on: 1454097
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.