[animation-trigger] Ensure first frame reflects triggered animation [chromium/src : main]
David Awogbemila (Gerrit)
David Awogbemila voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 2 comments![]( "Open in Gerrit")
if (timeline_->IsMonotonicallyIncreasing()) {Worth adding a comment here. Historically, OnValidateSnapshot was for scroll timelines, and now we have separate logic for scroll triggering with a monotonic timeline (though most of the logic is consistent).
Documentation for this method in the header file only mentions scroll timelines. Suggesting that we update the comment in the header file, and add a note here where we have the branching logic.
animations.erase(animation);We are effectively forcing the hashset to be copied just so we can remove elements from the map. AnimationTimeline::GetAnimations returns a const ref.
An SDA animation will be doing extra work here. because of the potential for animation triggers. Instead can we do the following:
```
const HeapHashSet<WeakMember<Animation>>& animations = GetAnimations();
if (Runtime...) {
...
for (auto& [animation, behaviors] : trigger-BehaviorMap()) {
DCHECK(...);
// Avoid superfluous snapshot validation, by skipping the call if it will
// be invoked in the loop below.
if (!animations.Contains(animation)) {
animation->OnValidateSnapshot(true);
}
}
}
```
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 voted Commit-Queue+1![]( "Open in Gerrit")
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")
if (timeline_->IsMonotonicallyIncreasing()) {Worth adding a comment here. Historically, OnValidateSnapshot was for scroll timelines, and now we have separate logic for scroll triggering with a monotonic timeline (though most of the logic is consistent).
Documentation for this method in the header file only mentions scroll timelines. Suggesting that we update the comment in the header file, and add a note here where we have the branching logic.
Done
animations.erase(animation);We are effectively forcing the hashset to be copied just so we can remove elements from the map. AnimationTimeline::GetAnimations returns a const ref.
An SDA animation will be doing extra work here. because of the potential for animation triggers. Instead can we do the following:
```
const HeapHashSet<WeakMember<Animation>>& animations = GetAnimations();
if (Runtime...) {
...
for (auto& [animation, behaviors] : trigger-BehaviorMap()) {
DCHECK(...);
// Avoid superfluous snapshot validation, by skipping the call if it will
// be invoked in the loop below.
if (!animations.Contains(animation)) {
animation->OnValidateSnapshot(true);
}
}
}
```
- Kevin Ellis
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. |
Kevin Ellis (Gerrit)
Kevin Ellis voted and added 1 comment![]( "Open in Gerrit")
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Awogbemila
| Code-Review | +1 |
| Commit-Queue | +2 |
2 comments
if (timeline_->IsMonotonicallyIncreasing()) {David AwogbemilaWorth adding a comment here. Historically, OnValidateSnapshot was for scroll timelines, and now we have separate logic for scroll triggering with a monotonic timeline (though most of the logic is consistent).
Documentation for this method in the header file only mentions scroll timelines. Suggesting that we update the comment in the header file, and add a note here where we have the branching logic.
David AwogbemilaDone
Done
animations.erase(animation);David AwogbemilaWe are effectively forcing the hashset to be copied just so we can remove elements from the map. AnimationTimeline::GetAnimations returns a const ref.
An SDA animation will be doing extra work here. because of the potential for animation triggers. Instead can we do the following:
```
const HeapHashSet<WeakMember<Animation>>& animations = GetAnimations();
if (Runtime...) {
...
for (auto& [animation, behaviors] : trigger-BehaviorMap()) {
DCHECK(...);
// Avoid superfluous snapshot validation, by skipping the call if it will
// be invoked in the loop below.
if (!animations.Contains(animation)) {
animation->OnValidateSnapshot(true);
}
}
}
```
Done
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[animation-trigger] Ensure first frame reflects triggered animation
This change allows Blink to not only detect that an animation is
triggered based on the output of the style and layout phases but also
ensure that the triggered animation is reflected in the computed style
of the animating element in the same frame as the trigger detection.
As the trigger state update and the animations update are now done at
the same time, there is no need to separate the UpdateState and
UpdateAnimation functions, so they are combined.
- M third_party/blink/renderer/core/animation/animation.cc
- M third_party/blink/renderer/core/animation/animation.h
- M third_party/blink/renderer/core/animation/animation_timeline.cc
- M third_party/blink/renderer/core/animation/animation_timeline.h
- M third_party/blink/renderer/core/animation/animation_trigger.h
- M third_party/blink/renderer/core/animation/document_animations.cc
- M third_party/blink/renderer/core/animation/scroll_snapshot_timeline.cc
- M third_party/blink/renderer/core/animation/timeline_trigger.cc
- M third_party/blink/renderer/core/animation/timeline_trigger.h
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/timeline-trigger-source-starts-in-trigger-range.tentative.html
Code-Review: +1 by Kevin Ellis, +1 by David Awogbemila
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57126
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |