[animation-trigger] Don't update attachments if layout is needed [chromium/src : main]
David Awogbemila (Gerrit)
David Awogbemila added 1 comment![]( "Open in Gerrit")
Hey Anders, ptal. I've tried to incorporate `PostLayoutSnapShotClient` into `TimelineTrigger` so we now get that second chance to update an element's style during the same lifecycle update where the attachment is done.
Seems to work fine when testing manually but when writing tests we seem to sometimes have style updates without lifecycle updates (unless I'm judging the situation wrong) so we sometimes need to force the lifecycle update before checking for the style change side effect in the test.
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 3 comments![]( "Open in Gerrit")
: PostLayoutSnapshotClient(timeline ? timeline->GetDocument()->GetFrame()I was wondering: Timelines seem to be aware of their associated triggers (`AnimationTimeline::triggers_`). It could maybe be an option to let `ScrollSnapshotTimelines` update the states of their triggers as well?
DynamicTo<ScrollSnapshotTimeline>(timeline_.Get())->UpdateSnapshot();This would cause the `ScrollSnapshotTimeline` to be updated _twice_, in a random order (`ScrollSnapshotTimeline` are also `PostLayoutSnapshotClient`s). We need to guarantee that a second update within the same snapshot round has no effect, somehow.
return Update();1. If we update the state here (and we should), we should not _also_ update the state in `UpdateAnimationTiming`.
2. Doesn't that mean we would also _trip_ the trigger here? I don't have the full picture, but deciding to start an animation may be weird as a "final action" after layout is done already? With the caveat that I don't have the full picture of how everything works, it seems more natural to do this part in `UpdateAnimationTiming` as we do currently. So basically, I think we should split update-state and perform-action into two things that can happen at two different times.
2b. By default, I think we should mark the trigger-attached animations for needs-animation-recalc, and then have `CalculateAnimationUpdate` (or similar) queue the triggers actions on the `CSSAnimationUpdate`, but that's another story.
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila added 3 comments![]( "Open in Gerrit")
: PostLayoutSnapshotClient(timeline ? timeline->GetDocument()->GetFrame()I was wondering: Timelines seem to be aware of their associated triggers (`AnimationTimeline::triggers_`). It could maybe be an option to let `ScrollSnapshotTimelines` update the states of their triggers as well?
Done
DynamicTo<ScrollSnapshotTimeline>(timeline_.Get())->UpdateSnapshot();This would cause the `ScrollSnapshotTimeline` to be updated _twice_, in a random order (`ScrollSnapshotTimeline` are also `PostLayoutSnapshotClient`s). We need to guarantee that a second update within the same snapshot round has no effect, somehow.
Acknowledged
return Update();1. If we update the state here (and we should), we should not _also_ update the state in `UpdateAnimationTiming`.
2. Doesn't that mean we would also _trip_ the trigger here? I don't have the full picture, but deciding to start an animation may be weird as a "final action" after layout is done already? With the caveat that I don't have the full picture of how everything works, it seems more natural to do this part in `UpdateAnimationTiming` as we do currently. So basically, I think we should split update-state and perform-action into two things that can happen at two different times.
2b. By default, I think we should mark the trigger-attached animations for needs-animation-recalc, and then have `CalculateAnimationUpdate` (or similar) queue the triggers actions on the `CSSAnimationUpdate`, but that's another story.
Okay, I've split the logic up into `UpdateState` and `UpdateAnimations`.
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 6 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
Hey Anders, ptal. I've tried to incorporate `PostLayoutSnapShotClient` into `TimelineTrigger` so we now get that second chance to update an element's style during the same lifecycle update where the attachment is done.
Seems to work fine when testing manually but when writing tests we seem to sometimes have style updates without lifecycle updates (unless I'm judging the situation wrong) so we sometimes need to force the lifecycle update before checking for the style change side effect in the test.
Seems to work fine when testing manually but when writing tests we seem to sometimes have style updates without lifecycle updates (unless I'm judging the situation wrong) so we sometimes need to force the lifecycle update before checking for the style change side effect in the test.
Yeah, I don't think we can get gCS to work consistently in all cases until we switch to the approach we discussed (off Gerrit) yesterday.
I don't fully get why this fixes the problem, but this looks like a step in the right direction anyway considering recent discussions.
std::optional<State> new_state = ComputeState();
if (!new_state) {Not changed in this CL, but I find it weird that `ComputeState()` returning `nullopt` means "no state change" even when we're holding a non-`nullopt` state.
return Update();David Awogbemila1. If we update the state here (and we should), we should not _also_ update the state in `UpdateAnimationTiming`.
2. Doesn't that mean we would also _trip_ the trigger here? I don't have the full picture, but deciding to start an animation may be weird as a "final action" after layout is done already? With the caveat that I don't have the full picture of how everything works, it seems more natural to do this part in `UpdateAnimationTiming` as we do currently. So basically, I think we should split update-state and perform-action into two things that can happen at two different times.
2b. By default, I think we should mark the trigger-attached animations for needs-animation-recalc, and then have `CalculateAnimationUpdate` (or similar) queue the triggers actions on the `CSSAnimationUpdate`, but that's another story.
Okay, I've split the logic up into `UpdateState` and `UpdateAnimations`.
Acknowledged
!document_.View()->NeedsLayout()) {You should probably document why we have this check.
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila added 2 comments![]( "Open in Gerrit")
You should probably document why we have this check.
Done
nit: Avoid touching this file?
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |