Closed Bug 1523776 Opened 7 months ago Closed 7 months ago

Filters and clips interact poorly with WR.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached file testcase

+++ This bug was initially created as a clone of Bug #1514383 +++

Cloning from bug 1514383 comment 7. The attached testcase is rendering wrong.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e4554a91e5ffc51bd4fbaba22a8d997b276813

Hopefully this one will be green. My previous try push (bug 1514383 comment 12) had one failure due to the clip chain for a sticky item not getting applied to the contents of the sticky after I broke the clip chain parent links. But this new try push has a patch that fixes that problem for me locally, by explicitly setting the clip chain on the stacking context that holds the contents of the sticky item.

Attachment #9039934 - Attachment mime type: text/plain → text/html

Previous try push had a typo. This one is better:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b00a066354c76f63e7f7e0fe3dd34ffa2da4f155

But there's some stupid fuzzing needed. I'll fix some of it via bug 1515310 which has the same problem.

The API to create a sticky spatial node doesn't allow us to set a clip
chain that will apply to all the contents of the sticky node. This means
that when the ClipManager sets up the clip state for the sticky node,
the clip chain for it is dropped. Everything still works currently
because the contents of the sticky node have their own clip chains whose
parent link will include the sticky node's clip chain. However, in the
next patch we're going to sever that parent link to fix other issues,
and so we will break this mechanism. This patch fixes it up by
explicitly applying the sticky node's clip chain to the stacking context
that contains all the sticky contents. This ensures all those items pick
up the clip chain.

It turns out that setting the parent link on a clip chain is no longer
needed (and probably hasn't been since WR started applying a stacking
context's clip to the SC's contents). In fact it can produce incorrect
behaviour in some cases, because it doesn't match the semantics of
Gecko's clip chains. This removes the parent link on the Gecko side and
adds a test for this scenario.

Depends on D18095

The API to create a sticky spatial node doesn't allow us to set a clip
chain that will apply to all the contents of the sticky node. This means
that when the ClipManager sets up the clip state for the sticky node,
the clip chain for it is dropped. Everything still works currently
because the contents of the sticky node have their own clip chains whose
parent link will include the sticky node's clip chain. However, in the
next patch we're going to sever that parent link to fix other issues,
and so we will break this mechanism. This patch fixes it up by
explicitly applying the sticky node's clip chain to the stacking context
that contains all the sticky contents. This ensures all those items pick
up the clip chain.

It turns out that setting the parent link on a clip chain is no longer
needed (and probably hasn't been since WR started applying a stacking
context's clip to the SC's contents). In fact it can produce incorrect
behaviour in some cases, because it doesn't match the semantics of
Gecko's clip chains. This removes the parent link on the Gecko side and
adds a test for this scenario.

Depends on D18100

Attachment #9040147 - Attachment is obsolete: true
Attachment #9040148 - Attachment is obsolete: true

Sorry for the phabricator noise, I ran into a moz-phab bug, filed 1523994 for it.

Also https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b1565844fe3e2ae0e5de6c20ea8dbc888f16420a is the latest try push.

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2aaf3468dd57
Set a clipchain on sticky contents. r=kvark
https://hg.mozilla.org/integration/autoland/rev/92684cd249a1
Don't set the parent link when creating a new clip chain. r=kvark
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1524261

I'm going to back this out for now as the regressions seem to be quite visible and the fix (see bug 1524261) involves fuzzing a lot of reftests which will take time to do properly.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla67 → ---

https://bugzilla.mozilla.org/show_bug.cgi?id=1524385 has landed now, so I think that this bug can be considered fixed (assuming that patch doesn't get backed out / cause regressions). Is that right?

Flags: needinfo?(kats)

Yes but I want to reland the reftest that I wrote for this bug. Working on that now.

Flags: needinfo?(kats)

Try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b40b7ab99195c597ff3273007bdf7ec629ddca59

Since :kvark already reviewed the test from the first time I landed it, I'll just reland on inbound. The test is the same as before, including the fuzz numbers.

Status: REOPENED → RESOLVED
Closed: 7 months ago7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.