Reset image animation on layout removal and avoid storing normal style [chromium/src : main]
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Related details
- Florin Malita
- Rune Lillesveen
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
`BitmapImage` via the `ImageNodeAnimationInfo` struct.Seokho SongIt don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Rune LillesveenAlso, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment? If so, we can't use this approach here for [the same reason here](https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comment/ebcaa4e3_53468901/)...
We might need to find the right place to store this information, perhaps in `ElementRareData` (similar to [this one](https://chromium-review.googlesource.com/c/chromium/src/+/7692355)) to explicitly track the previous style and reattachment.
WDYT @fma...@chromium.org?
Seokho SongAlso, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment?
There will be an old style, but there might be intermediate style changes which are never passed on to the paint code. I've suggested a way this can be observed in the test you're adding.
Rune LillesveenAh got it. Thanks! I've changed the approach to update/store previous image animation.
I've read the spec now, and I got the impression that these timelines are intended to behave like CSS/web animation timelines wrt when they are created and reset on display:none changes, for instance.
It might be a good idea for the spec to refer to the animation specs and timelines and have an animation timeline that runs all the animated images.
Then it would also be natural to run these timelines on ElementAnimations and let the rendering changes affect the timelines in the same way as for other animation timelines.
Disclaimer: I'm not an expert, and I've not been involved in the spec discussions here.
Adding flackr@ for input.
Related details
- Florin Malita
- Robert Flack
- Seokho Song
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. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
`BitmapImage` via the `ImageNodeAnimationInfo` struct.Seokho SongIt don't think it makes sense to track this on ComputedStyle. Also, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Rune LillesveenAlso, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment? If so, we can't use this approach here for [the same reason here](https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comment/ebcaa4e3_53468901/)...
We might need to find the right place to store this information, perhaps in `ElementRareData` (similar to [this one](https://chromium-review.googlesource.com/c/chromium/src/+/7692355)) to explicitly track the previous style and reattachment.
WDYT @fma...@chromium.org?
Seokho SongAlso, you may have an arbitrary number of style change events between paints, so the old image animation may be lost.
Hmm, IIUC, does this mean the old style might not exist for various reasons other than DOM tree reattachment?
There will be an old style, but there might be intermediate style changes which are never passed on to the paint code. I've suggested a way this can be observed in the test you're adding.
Rune LillesveenAh got it. Thanks! I've changed the approach to update/store previous image animation.
I've read the spec now, and I got the impression that these timelines are intended to behave like CSS/web animation timelines wrt when they are created and reset on display:none changes, for instance.
It might be a good idea for the spec to refer to the animation specs and timelines and have an animation timeline that runs all the animated images.
Then it would also be natural to run these timelines on ElementAnimations and let the rendering changes affect the timelines in the same way as for other animation timelines.
Disclaimer: I'm not an expert, and I've not been involved in the spec discussions here.
Adding flackr@ for input.
I think it would make sense to move ImageAnimationData into ElementAnimations and share more of that code path.
Related details
- Florin Malita
- Seokho Song
Seokho Song (Gerrit)
Seokho Song voted and added 1 comment![]( "Open in Gerrit")
Related details
- Florin Malita
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
Removing myself since the style approach is now gone.
Related details
- Florin Malita
- Seokho Song
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. |
Robert Flack (Gerrit)
Robert Flack added 4 comments![]( "Open in Gerrit")
I'm not sure I understand all of indirection going on in this patch. I can see that when we have a value that is not normal, we need to track some information for that element - but the purpose of using ElementAnimations was to have element associated data - it seems like we're storing this data on the bitmap though with a possible risk of never erasing it.
if (RuntimeEnabledFeatures::CSSImageAnimationEnabled()) {These calls to clear don't need to be flag guarded since we would not add any image animations if the flag wasn't enabled so the clear would be a no-op.
image_animation_map_.find(image_node_animation_info->node_id);Why is image_animation_map_ a map of NodeId to ImageAnimationData on BitmapImage rather than storing ImageAnimationData on ElementAnimations? It seems like if we stored the animation data on ElementAnimations then there would be no risk of the data persisting when the element is detached or memory leaks by not removing entries.
image_animation_map_.erase(animation_data_it);It seems like this is the only call to erase the image animation data state, but what happens if the node is detached from the dom or becomes display none, we would never call PaintImageForCurrentFrameWithInfo right?