1501111 - Wrong clipping with clip-path and a nested clip.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

This is a recent regression.

Expected: Green background with text.

Actual: A bullet.

The relevant bit is that a bullet is a nested clip.

This affects: https://christopher-tierney.com/apotheke-la/

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This regressed with https://github.com/servo/webrender/commit/47242e13a6f88c17186afeaf2545796670e2d078.

Glenn, any chance you can take a look? Otherwise let me know needinfo?ing me back and I'll take a look, though will be probably next week since I'm at TPAC.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So, I took a look and talked a bit with Glenn about this.

So this test-case breaks WebRender's assumption that any clip with spatial node index < stacking context's spatial node.

WebRender only creates spatial nodes for sticky frames, reference frames, and scroll frames. But it also creates a surface for clip-path, for example:

  https://searchfox.org/mozilla-central/rev/eac6295c397133b7346822ad31867197e30d7e94/gfx/webrender/src/display_list_flattener.rs#1013

This is sound for transforms and such because they create a fixed-pos containing block. I think it's fine for scroll frames because we don't create separate surfaces for them, but this is not true for masks, so this falls apart in that case.

So potential fixes for this are:

 (a) Make WR not create separate surfaces for anything that isn't a reference frame (i.e. make clip-path not generate a surface).
 (b) Make position: fixed its own spatial node.

Given we want to put more stuff in surfaces in the future, (b) sounds better.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

> So this test-case breaks WebRender's assumption that any clip with spatial node index < stacking context's spatial node.

This should read "that any clip with spatial node index < stacking context's spatial node is safe to apply to the whole stacking context".

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1501111 - [WIP] Make fixed pos display items create a WR spatial node.

Still a couple things to iron out, since I need to dig into some of
the async-scrolling reftests that are failing, but this should hopefully be on
the right track.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9021984 [details]
Bug 1501111 - [WIP] Make fixed pos display items create a WR spatial node.

Kats, Glenn, does this look on the right track to you?

I need to figure out some async-scrolling test failures, and write a test for this of course, but it'd be great confirming that this looks roughly right.

Comment 6

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

Comment on attachment 9021984 [details]
Bug 1501111 - [WIP] Make fixed pos display items create a WR spatial node.

The gecko side looks reasonable to me.

Comment 7

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

Also just a heads-up in case you weren't aware of servo/webrender#3251, there will likely be some rebasing involved for whoever lands second.

Comment 8

[Glenn Watson [:gw]]

We briefly discussed in IRC that I was initially thinking this could be done without WR patches - I'm not sure if we worked out if that was the case or not?

Comment 9

[Glenn Watson [:gw]]

Comment on attachment 9021984 [details]
Bug 1501111 - [WIP] Make fixed pos display items create a WR spatial node.

Did you have a chance to look into whether this can be done without WR changes?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Emilio, what's the status of this?

Comment 11

[Glenn Watson [:gw]]

FWIW, I _think_ we can probably actually solve this by essentially reverting the commit that originally created the regression. That change is, I believe, no longer necessary with the way the picture caching code has ended up working. Emilio, perhaps we can discuss this next week and give it a try?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Emilio, what's the status of this?

The status is that it was not clear this was the right way to fix it. We could fix it that way, but the issue is that essentially the assumption bug 1494898 introduced doesn't hold.

(In reply to Glenn Watson [:gw] from comment #11)
> FWIW, I _think_ we can probably actually solve this by essentially reverting
> the commit that originally created the regression. That change is, I
> believe, no longer necessary with the way the picture caching code has ended
> up working. Emilio, perhaps we can discuss this next week and give it a try?

Sure, that'd be great, I did notice the WR clip node collector stuff changing recently, so will do. Sorry for the lag with all these bugs btw, had some unexpected PTO time this week...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: GitHub Pull Request

Will add a reftest patch shortly.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1501111 - Add a reftest for this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1babeef937f
Add a reftest for this. r=jrmuizel

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14749 for changes under testing/web-platform/tests

Comment 17

[Daniel Varga [:dvarga]]

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

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged