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)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
Attachments
(1 file)
1.58 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•7 years ago
|
Assignee: nobody → matt.woodrow
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
Does calling `RequestRestyle(EffectCompositor::RestyleType::Layer)` in place of `SchedulePaint` work?
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
(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.
Updated•7 years ago
|
Attachment #8904767 -
Flags: review?(bbirtles) → review+
Comment 6•7 years ago
|
||
(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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/610fbd30a6a3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•