Wrong clipping with clip-path and a nested clip.

RESOLVED FIXED in Firefox 66

Status

()

P2
normal
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks: 1 bug, {regression})

unspecified
mozilla66
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 disabled, firefox65 disabled, firefox66 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 months ago
Posted file 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/
(Assignee)

Updated

5 months ago
Assignee: nobody → emilio
Flags: needinfo?(emilio)
(Assignee)

Comment 1

5 months ago
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.
Blocks: 1494898
Flags: needinfo?(emilio) → needinfo?(gwatson)
Keywords: regression
Blocks: 1386669
Priority: -- → P2
(Assignee)

Updated

5 months ago
Flags: needinfo?(emilio)
(Assignee)

Comment 2

5 months ago
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.
(Assignee)

Comment 3

5 months ago
> 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".

Updated

5 months ago
Flags: needinfo?(gwatson)
(Assignee)

Comment 4

5 months ago
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.
(Assignee)

Comment 5

5 months ago
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.
Attachment #9021984 - Flags: feedback?(kats)
Attachment #9021984 - Flags: feedback?(gwatson)
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.
Attachment #9021984 - Flags: feedback?(kats) → feedback+
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

4 months ago
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

4 months ago
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?
Attachment #9021984 - Flags: feedback?(gwatson) → feedback+
Emilio, what's the status of this?
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?
(Assignee)

Comment 12

3 months ago
(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...
(Assignee)

Comment 13

3 months ago
Posted file GitHub Pull Request

Will add a reftest patch shortly.

Flags: needinfo?(emilio)
Attachment #9021984 - Attachment is obsolete: true

Comment 15

3 months ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1babeef937f
Add a reftest for this. r=jrmuizel
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14749 for changes under testing/web-platform/tests

Comment 17

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Upstream PR merged
status-firefox64: --- → disabled
status-firefox65: --- → disabled
status-firefox-esr60: --- → unaffected
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.