1514384 - drop-shadow() filter is clipped if its size changes in response to off-main-thread animations

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Testcases: attachment 8972679 [details] and attachment 8972680 [details]

The animated elements get clipped during the animation. They should always be completely visible.

I think this happens if the size of the filter changes because a property animation affects the bounds which are covered by the items inside the filter.

Comment 1

[Darkspirit]

mozregression --good 2018-06-15 --bad 2018-10-01 --pref gfx.webrender.all:true -a https://bug1458661.bmoattachments.org/attachment.cgi?id=8972679 -a https://bug1458661.bmoattachments.org/attachment.cgi?id=8972680
> 9:31.53 INFO: Last good revision: febddb5a4dc21c7b09b02efc39568a03588d893b
> 9:31.53 INFO: First bad revision: 0192efea1b664db8d20a691401ec5d3f9d3a2122
> 9:31.53 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=febddb5a4dc21c7b09b02efc39568a03588d893b&tochange=0192efea1b664db8d20a691401ec5d3f9d3a2122

the relevant one:
> f4739ab0ece1	Emilio Cobos Álvarez — Bug 1459065 - Clip filter effects at the stacking context level. r=mstange

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Well, sure, before that we wouldn't clip it at all :)

But sure I'll take a look.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Are you still looking at this Emilio?

Clipping to the current bounds of the filter item does seem wrong, since they can change asynchronously.

Do we just need a binding.rs api to get the WrClipId from DisplayListBuilder::mCurrentSpaceAndClipChain, so that we can define the stacking context as being clipped by that?

Comment 4

[Emilio Cobos Álvarez [:emilio]]

I think we can just push a max clip rect. The clip id we pass to webrender is really just a hint so that WR properly puts it on its own surface.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

I.e, if I recall correctly, the right clip should already be on the clip stack, we really just need to hit 1.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Oh, I hadn't seen that. Isolation as a side effect of being clipped feels a bit weird!

Still, seems somewhat cleaner to just use the existing clip we have, rather than defining a no-op new one.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

yeah, just confirmed that max clip rect works, will try to rejigger the API to allow passing the existing clip chain.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1514384 - Properly make CSS clip property clip filters as well.

This fixes a bug that my previous patch uncovers, and that's tested by
css/css-masking/clip/clip-filter-order.html.

We weren't setting up the clips in a way that the clip property clipped filters
and such. This test works in Gecko because Gecko won't draw outside of the
effect bounds, that account for clip().

I moved the mask bit there as well because I ran a bunch of SVG masking
reftests and they were green, but if it fails by any reason I can put it back
where it was.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

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

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1519073 - Pass the current clip chain id instead of clipping to the filter bounds. r?mattwoodrow

We could also instead, maybe, force isolation when we have a stacking context
with blurs or such, and remove this clip altogether... That might be cleaner
actually.

In any case this works.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b61f3ad2ea3
Pass the current clip chain id instead of clipping to the filter bounds. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/0026b863c437
Properly make CSS clip property clip filters as well. r=mstange

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8b61f3ad2ea3
https://hg.mozilla.org/mozilla-central/rev/0026b863c437