[animation-trigger] Propagate triggers up the fragment tree. [chromium/src : main]
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 12 comments![]( "Open in Gerrit")
So I think you want to do something like:
`LocalFrameView::ExecutePendingSnapUpdates` or
`LocalFrameView::ExecutePendingStickyUpdates`
The animation-trigger map generated within the fragment-tree should be idempotent, e.g. a layout with the same style should produce the same result, and not have any side-effects.
Currently it relies on the previous map, which introduces hysterisis. E.g. within a layout we may visit the same layout-object multiple times, with different styles, currently all those old animation triggers would accumulate which i suspect isn't what we want.
The map produced should just rely on the current style, and the child fragments, then after layout we can do a step like snap/sticky to update the animations, and compare against the old map produced.
Member<AnimationTrigger> trigger;Can this be const?
struct AnimationTriggerAssociationThis likely is better in its own file.
node_.GetDOMNode()->GetTriggerMap();Accessing the existing_map here isn't great.
Can you do something similar to how sticky works? See LocalFrameView::ExecutePendingStickyUpdates
E.g. layout may get called multiple times for a node, and potentially with different styles, etc. You likely want to compare the start vs. end maps after layout has occurred, rather than have side effects like this within layout.
if (Document* document = DynamicTo<Document>(layout_object_->GetNode())) {why this?
if (Member<const ScopedCSSName> name = trigger_names[i]) {if (!name) {
continue;
}(reduce nesting).
AnimationTrigger* trigger = CSSAnimations::ComputeTimelineTrigger(can this be const?
for (const auto& entry : *existing_map) {Accessing the previous state here introduces hysteresis which we should avoid.
entry.value->trigger->RemoveAnimations();This is a side effect which we'd want to avoid if possible, this should be done after layout.
return true;just add `invalidation: ["layout"]` instead.
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
So I think you want to do something like:
`LocalFrameView::ExecutePendingSnapUpdates` or
`LocalFrameView::ExecutePendingStickyUpdates`The animation-trigger map generated within the fragment-tree should be idempotent, e.g. a layout with the same style should produce the same result, and not have any side-effects.
Currently it relies on the previous map, which introduces hysterisis. E.g. within a layout we may visit the same layout-object multiple times, with different styles, currently all those old animation triggers would accumulate which i suspect isn't what we want.
The map produced should just rely on the current style, and the child fragments, then after layout we can do a step like snap/sticky to update the animations, and compare against the old map produced.
Generally speaking as well, the map should be as "const" as possible, so that we guarantee that nobody is messing with it.
David Awogbemila (Gerrit)
David Awogbemila added 11 comments![]( "Open in Gerrit")
Member<AnimationTrigger> trigger;David AwogbemilaCan this be const?
Done
This likely is better in its own file.
Done
node_.GetDOMNode()->GetTriggerMap();Accessing the existing_map here isn't great.
Can you do something similar to how sticky works? See LocalFrameView::ExecutePendingStickyUpdates
E.g. layout may get called multiple times for a node, and potentially with different styles, etc. You likely want to compare the start vs. end maps after layout has occurred, rather than have side effects like this within layout.
Okay, I've switched to a different approach: propagate just the names (and the associated LayoutObjects) up the fragment tree and create & compare the triggers in LayoutBox::UpdateAfterLayout. I think that's about the same timing ExecutePendingStickyUpdates which comes at the end of layout.
if (Document* document = DynamicTo<Document>(layout_object_->GetNode())) {David Awogbemilawhy this?
I thought we needed some special handling for the document but I don't think there's a way to declare a trigger on the document so I've removed this.
if (Member<const ScopedCSSName> name = trigger_names[i]) {if (!name) {
continue;
}(reduce nesting).
Done
AnimationTrigger* trigger = CSSAnimations::ComputeTimelineTrigger(David Awogbemilacan this be const?
Acknowledged. It's been removed.
Accessing the previous state here introduces hysteresis which we should avoid.
Done. Checking the previous state is now done after layout in LayoutBox::UpdateAfterLayout.
This is a side effect which we'd want to avoid if possible, this should be done after layout.
Done. Now done after layout in UpdateAfterLayout.
just add `invalidation: ["layout"]` instead.
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
node_.GetDOMNode()->GetTriggerMap();David AwogbemilaAccessing the existing_map here isn't great.
Can you do something similar to how sticky works? See LocalFrameView::ExecutePendingStickyUpdates
E.g. layout may get called multiple times for a node, and potentially with different styles, etc. You likely want to compare the start vs. end maps after layout has occurred, rather than have side effects like this within layout.
Okay, I've switched to a different approach: propagate just the names (and the associated LayoutObjects) up the fragment tree and create & compare the triggers in LayoutBox::UpdateAfterLayout. I think that's about the same timing ExecutePendingStickyUpdates which comes at the end of layout.
So LayoutBox::UpdateAfterLayout is likely incorrect.
E.g. a LayoutBox may get destroyed & recreated again in the middle of layout, where-as a Node will remain stable.
Likely you don't want to cancel animations in this case?
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila added 1 comment![]( "Open in Gerrit")
if (Element* target =
GetDocument().getElementById(AtomicString("target"))) {
// TODO(crbug.com/429392773): We shouldn't need to force layout here.
target->GetLayoutBox()->SetNeedsLayout("");
}This seems to be needed despite setting `invalidates: ["layout"]` on all the longhands. @ikilp...@chromium.org, would you know how to address this?
Basically, the problem is that some of the tests change an element's style and expect to see the updated trigger which the layout phase should create in `LayoutBox::UpdateAfterLayout`, but sadly, layout seems to be skipped.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
`LocalFrameView::ExecutePendingSnapUpdates` or
`LocalFrameView::ExecutePendingStickyUpdates`
The animation-trigger map generated within the fragment-tree should be idempotent, e.g. a layout with the same style should produce the same result, and not have any side-effects.
Currently it relies on the previous map, which introduces hysterisis. E.g. within a layout we may visit the same layout-object multiple times, with different styles, currently all those old animation triggers would accumulate which i suspect isn't what we want.
The map produced should just rely on the current style, and the child fragments, then after layout we can do a step like snap/sticky to update the animations, and compare against the old map produced.
Generally speaking as well, the map should be as "const" as possible, so that we guarantee that nobody is messing with it.
Acknowledged
if (Element* target =
GetDocument().getElementById(AtomicString("target"))) {
// TODO(crbug.com/429392773): We shouldn't need to force layout here.
target->GetLayoutBox()->SetNeedsLayout("");
}This seems to be needed despite setting `invalidates: ["layout"]` on all the longhands. @ikilp...@chromium.org, would you know how to address this?
Basically, the problem is that some of the tests change an element's style and expect to see the updated trigger which the layout phase should create in `LayoutBox::UpdateAfterLayout`, but sadly, layout seems to be skipped.
Moved to crrev.com/c/6778863. Done.
node_.GetDOMNode()->GetTriggerMap();David AwogbemilaAccessing the existing_map here isn't great.
Can you do something similar to how sticky works? See LocalFrameView::ExecutePendingStickyUpdates
E.g. layout may get called multiple times for a node, and potentially with different styles, etc. You likely want to compare the start vs. end maps after layout has occurred, rather than have side effects like this within layout.
Ian KilpatrickOkay, I've switched to a different approach: propagate just the names (and the associated LayoutObjects) up the fragment tree and create & compare the triggers in LayoutBox::UpdateAfterLayout. I think that's about the same timing ExecutePendingStickyUpdates which comes at the end of layout.
So LayoutBox::UpdateAfterLayout is likely incorrect.
E.g. a LayoutBox may get destroyed & recreated again in the middle of layout, where-as a Node will remain stable.
Likely you don't want to cancel animations in this case?
As discussed offline, I've moved the trigger creatino stuff to crrev.com/c/6778863 and left just the name propagation here.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 6 comments![]( "Open in Gerrit")
GCedHeapHashMap<Member<const ScopedCSSName>, Member<const Element>>;Can optionally move this definition into a header like:
third_party/blink/renderer/core/style/named_grid_lines_map.h
using NameToElementMap =TriggerNameToElementMap ? (NameToElementMap a bit too generic).
Member<GCedHeapDeque<Member<LayoutBox>>>Member<Element>
EnsureTriggerNames().Set(entry.key, entry.value);Something like:
<div id="a" style="trigger-name: a">
<div id="b" style="trigger-name: a"></div>
</div>
will return "b" is this desired?
(e.g. CreateTriggerNames should occur when finalized).
if (!node_ || !node_.GetDOMNode()) {This check is redundant (DynamicTo will pass through nullptr)
Element* element = DynamicTo<Element>(node_.GetDOMNode());.nit const
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
invalidate: ["layout"],only this should need to invalidate layout ?
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
TriggerNameToElementMap& EnsureTriggerNames();This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters. The `Ensure` prefix violates the 'bare words for getters' rule. Please rename to `TriggerNames()` to match the member variable `trigger_names_`.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
const TriggerNameToElementMap* TriggerNames() const {This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters. This rule states getter names should match the variable being accessed. The getter `TriggerNames()` accesses the `trigger_names_map` member, so it should be renamed to `TriggerNamesMap()`.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'
If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila added 9 comments![]( "Open in Gerrit")
only this should need to invalidate layout ?
Acknowledged. Obsolete now.
GCedHeapHashMap<Member<const ScopedCSSName>, Member<const Element>>;Can optionally move this definition into a header like:
third_party/blink/renderer/core/style/named_grid_lines_map.h
Done. I created a file named named_animation_trigger_map.h which defines a NamedAnimationTriggerMap.
TriggerNameToElementMap ? (NameToElementMap a bit too generic).
Acknowledged. Obsolete now.
Member<GCedHeapDeque<Member<LayoutBox>>>David AwogbemilaMember<Element>
Done
This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters. The `Ensure` prefix violates the 'bare words for getters' rule. Please rename to `TriggerNames()` to match the member variable `trigger_names_`.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
Won't fix: This isn't just a setter and follows the existing pattern.
Something like:
<div id="a" style="trigger-name: a">
<div id="b" style="trigger-name: a"></div>
</div>will return "b" is this desired?
(e.g. CreateTriggerNames should occur when finalized).
Good point, done.
This check is redundant (DynamicTo will pass through nullptr)
Done, thanks!
Element* element = DynamicTo<Element>(node_.GetDOMNode());David Awogbemila.nit const
Done
const TriggerNameToElementMap* TriggerNames() const {This comment was generated by Blink C++ Code Review Agent. See go/blink-c++-code-review-agent for more details.
Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters. This rule states getter names should match the variable being accessed. The getter `TriggerNames()` accesses the `trigger_names_map` member, so it should be renamed to `TriggerNamesMap()`.
Please respond with one of these three options:
'Done' OR 'b/<bug_id>' OR 'Won't fix: <reason>'If you have feedback or would like to get involved in this effort, please file a bug here: go/blink-c++-code-review-agent-feedback
#code-review-agent-blink-c++
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila added 1 comment![]( "Open in Gerrit")
+dgrogan as it looks like Ian is OOO this week. Ptal David, thanks.
Related details
- David Grogan
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Awogbemila
| Commit-Queue | +1 |
1 comment
Thinking about it a bit more, it might be better to wait for Ian to return as we've met to discuss this work and he already has a lot of the context.
Sorry for the churn @dgr...@chromium.org!
Related details
- David Grogan
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
Thinking about it a bit more, it might be better to wait for Ian to return as we've met to discuss this work and he already has a lot of the context.
Sorry for the churn @dgr...@chromium.org!
NP, I was going to suggest that also, for the same reason 😊
Related details
- David Awogbemila
- Ian Kilpatrick
David Awogbemila (Gerrit)
David Awogbemila removed awogb...@google.com from this change![]( "Open in Gerrit")
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 4 comments![]( "Open in Gerrit")
NamedAnimationTriggerMap new_trigger_map = NamedAnimationTriggerMap();.nit no need to initialize.
}this might be easier to read as:
```
const bool enqueue = ([&]() {
if (NamedTriggers()) {
return true;
}
for (const auto& fragment : layout_box->PhysicalFragments()) {
if (fragment.NamedTriggers()) {
return true;
}
}
}
return false;
});
```
also comments for why enqueuing in each scenario would be good.
NamedAnimationTriggerMap named_triggers_;Make this a GCedHeapMap
const NamedAnimationTriggerMap named_triggers;Member<const GCedHeapMap<>>
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila removed awogb...@google.com from this change![]( "Open in Gerrit")
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila voted and added 4 comments![]( "Open in Gerrit")
Votes added by David Awogbemila
| Commit-Queue | +1 |
4 comments
NamedAnimationTriggerMap new_trigger_map = NamedAnimationTriggerMap();.nit no need to initialize.
Done
this might be easier to read as:
```
const bool enqueue = ([&]() {
if (NamedTriggers()) {
return true;
}
for (const auto& fragment : layout_box->PhysicalFragments()) {
if (fragment.NamedTriggers()) {
return true;
}
}
}
return false;
});
```also comments for why enqueuing in each scenario would be good.
Sounds good. Done.
NamedAnimationTriggerMap named_triggers_;David AwogbemilaMake this a GCedHeapMap
Sounds good. Done.
const NamedAnimationTriggerMap named_triggers;David AwogbemilaMember<const GCedHeapMap<>>
Sounds good. Done.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 2 comments![]( "Open in Gerrit")
void Element::EnqueueNamedTriggersUpdateIfNeeded() {So because this isn't transforming the triggers now, we can remove this entirely, and just access the trigger map of the layout object.
Previously we couldn't do this but now we can.
EnsureNamedTriggers().Set(entry.key, entry.value);.nit extract this to outside of loop:
```
auto& named_triggers = EnsureNamedTriggers();
for () {
named_triggers.Set();
}
```
Related details
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
CreateNamedTriggers();Maybe something like `CreatedNamedTriggersForSelf`
David Awogbemila (Gerrit)
David Awogbemila voted and added 4 comments![]( "Open in Gerrit")
Votes added by David Awogbemila
| Commit-Queue | +1 |
4 comments
style->HasAnimationTrigger()) {I didn't find a problem with reading the triggers off the LayoutBox, but relying on LayoutBox exposed, through a failing test[1], a potential issue.
In the test[1], script would start running (`Element.getAnimations()`), expecting to see a side effect of the triggers' having been attached to the animation, but because layout hadn't happened yet, there was no trigger in the LayoutBox and the expected side effect was absent.
I addressed this by making `HasAnimationTrigger` one of the conditions that indicates style is affected by layout. Seems reasonable to me given that we need layout to happen to find the animation to attach to. Happy to work out any concerns though.
void Element::EnqueueNamedTriggersUpdateIfNeeded() {So because this isn't transforming the triggers now, we can remove this entirely, and just access the trigger map of the layout object.
Previously we couldn't do this but now we can.
Sounds good. Done.
CreateNamedTriggers();Maybe something like `CreatedNamedTriggersForSelf`
Sounds good. Done.
.nit extract this to outside of loop:
```
auto& named_triggers = EnsureNamedTriggers();
for () {
named_triggers.Set();
}
```
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted Code-Review+1![]( "Open in Gerrit")
- David Awogbemila
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| 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] Propagate triggers up the fragment tree.
The new proposal[1] for animation-trigger is that it declares a name (a
dashed ident) which is to be matched against the names declared by
trigger-instantiating properties, i.e.timeline-trigger, event-trigger
properties. The visibility of the names declared by
trigger-instantiating properties follows a model similar to anchor-name
- global by default, with the option of limiting the visibility with
a (yet to be implemented) scope property.
We are implementing this in 3 phases:
1. Compute AnimationTriggers from timeline-trigger declarations
(crrev.com/c/6813829 ).
2. Propagate the AnimationTrigger declarations from step1 up through the
fragment tree. (This patch)
3. Attach the AnimationTrigger objects to the associated Animations
after step 2 is done in layout (crrev.com/c/6775141 ).
This patch achieves the visibility of CSS trigger names by following the
Blink's anchor-name handling pattern - propagating the relevant
information up the fragment tree.
[1] https://github.com/w3c/csswg-drafts/issues/12336
- M third_party/blink/renderer/core/animation/css/css_animations.cc
- M third_party/blink/renderer/core/animation/css/css_animations.h
- M third_party/blink/renderer/core/animation/css/css_animations_test.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/dom/named_animation_trigger_map.h
- M third_party/blink/renderer/core/layout/fragment_builder.cc
- M third_party/blink/renderer/core/layout/fragment_builder.h
- M third_party/blink/renderer/core/layout/physical_fragment.cc
- M third_party/blink/renderer/core/layout/physical_fragment.h
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/renderer/core/style/computed_style.h
Code-Review: +1 by Ian Kilpatrick
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |