Closed Bug 1397057 Opened 7 years ago Closed 7 years ago

Make sure we trigger an invalidation when we change animation state.

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file)

When we toggle from not having an animation (of opacity/transform), to having one in delayed state, we want to start rendering as a stacking context, but we don't actually update the style data for the frame.

The animation state change triggers a paint, but nothing marks the actual animated frame as changed.

When we rebuild the entire display list, this doesn't matter, as we recompute stacking context information for the entire tree.
When we're retaining the display list, we decide to retain the old display items (without stacking context), and the rendering can be different (we have a few tests that catch this).
Attachment #8904767 - Flags: review?(bbirtles)
Assignee: nobody → matt.woodrow
Is there a test case that fails without this?

I'm just wondering if `RequestRestyle(EffectCompositor::RestyleType::Layer)` would work here in place of SchedulePaint since, if it does, that might be more consistent with the surrounding code.
web-animations/stacking-context-opacity-changing-target-in-delay.html (and the transform equivalent) fails with retained-dl and this patch backed out.

Going into the delay phase means that we should re-render the element as a stacking context, but nothing tells retained-dl, and we just use the old display list.
Does calling `RequestRestyle(EffectCompositor::RestyleType::Layer)` in place of `SchedulePaint` work?
(In reply to Brian Birtles (:birtles) from comment #3)
> Does calling `RequestRestyle(EffectCompositor::RestyleType::Layer)` in place
> of `SchedulePaint` work?

In the case where we have no EffectSet in KeyframeEffectReadOnly::UnregisterTarget, RequestRestyle(Layer) might not work since we can't store the animation generation in EffectSet.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> (In reply to Brian Birtles (:birtles) from comment #3)
> > Does calling `RequestRestyle(EffectCompositor::RestyleType::Layer)` in place
> > of `SchedulePaint` work?
> 
> In the case where we have no EffectSet in
> KeyframeEffectReadOnly::UnregisterTarget, RequestRestyle(Layer) might not
> work since we can't store the animation generation in EffectSet.

Yeah, fair enough. It might still work (since the default 0 generation would not match the generation we put on the layer) but I'm not sure. I'd prefer to see if that works but since I can't easily test retained-dl it's probably ok to just accept this and try to replace it with RequestRestyle later, once retained-dl has landed.
Attachment #8904767 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > (In reply to Brian Birtles (:birtles) from comment #3)
> > > Does calling `RequestRestyle(EffectCompositor::RestyleType::Layer)` in place
> > > of `SchedulePaint` work?
> > 
> > In the case where we have no EffectSet in
> > KeyframeEffectReadOnly::UnregisterTarget, RequestRestyle(Layer) might not
> > work since we can't store the animation generation in EffectSet.
> 
> Yeah, fair enough. It might still work (since the default 0 generation would
> not match the generation we put on the layer) but I'm not sure. I'd prefer
> to see if that works but since I can't easily test retained-dl it's probably
> ok to just accept this and try to replace it with RequestRestyle later, once
> retained-dl has landed.

Yeah, I am having the same thing in mind. We can later consider it after landing. I am also looking forward to see the retained display list and run animation tests with it. :)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/610fbd30a6a3
Invalidate frames whenever we toggle an animation on the corresponding Element. r=birtles
https://hg.mozilla.org/mozilla-central/rev/610fbd30a6a3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1399667
Depends on: 1400022
You need to log in before you can comment on or make changes to this bug.