[Blink] Reset finished SVG image animations on reload [chromium/src : main]
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
Hi, could you take a look at this? thanks!
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
void ImageLoader::ResetAnimationOrDeferUntilAttach() {I don't think there's supposed to be any connection between resetting an animation and whether the animation is visible at all, so I suspect what you want to do here is rather to separate resetting the animation and potentially triggering invalidation (if needed).
Related details
- Kevin Ellis
- zhenbang Jiang
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 added 2 comments![]( "Open in Gerrit")
if (timeline->HasAnimations()) {Not sure we need another HasAnimations method. We already have Element::HasAnimations(), Document::getAnimations(), Element::getAnimations(), and Element::GetElementAnimations().
This method would return true if any element in the document has an animation, or even if it recently had an animation that simply hasn't been garbage collected yet. Methinks Element::GetElementAnimations() provides what you need.
document.GetDocumentAnimations().HasAnimations();Element already has a HasAnimations method.
root_element->HasAnimations()
Related details
- zhenbang Jiang
zhenbang Jiang (Gerrit)
zhenbang Jiang added 4 comments![]( "Open in Gerrit")
Not sure we need another HasAnimations method. We already have Element::HasAnimations(), Document::getAnimations(), Element::getAnimations(), and Element::GetElementAnimations().
This method would return true if any element in the document has an animation, or even if it recently had an animation that simply hasn't been garbage collected yet. Methinks Element::GetElementAnimations() provides what you need.
Thanks, I removed the extra DocumentAnimations::HasAnimations() helper.
The current check now uses existing element-level animation state via Element::HasAnimations() instead. I did not use root_element->GetElementAnimations() alone because the CSS animations we need to detect may live on descendant elements inside the isolated SVG document, not on the outer <svg> root itself. Restricting the check to the root element would miss those cases and skip reuse/reset again.
I also avoided Document::getAnimations()/Element::getAnimations() here because those paths update style/layout, while this check needs to stay lightweight during SVG image reuse.
void ImageLoader::ResetAnimationOrDeferUntilAttach() {I don't think there's supposed to be any connection between resetting an animation and whether the animation is visible at all, so I suspect what you want to do here is rather to separate resetting the animation and potentially triggering invalidation (if needed).
Thanks, I split reset from invalidation.
The reused SVG image is now reset independently of visibility or layout attachment. Paint invalidation is triggered separately only when a LayoutImageResource already exists, so reset no longer depends on whether the animation is currently visible, while attached layout objects still repaint when needed.
Element already has a HasAnimations method.
root_element->HasAnimations()
- Fredrik Söderquist
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 13 comments![]( "Open in Gerrit")
- Defer image animation reset until the LayoutImageResource is attached whenBreak at 72.
Verification:
- Open https://cdn.svgator.com/testing/reload-issue/index.html
- Wait for the animation to finish
- Refresh the page
- Verify that the animation starts again from the initial frame instead of
remaining on the final frameThis belongs in the bug.
!animation->effect() || !DynamicTo<CSSAnimation>(animation.Get())) {Use `IsA<>`.
Compositor().BeginFrame();Why remove the `BeginFrame()` calls - I think this test wants them?
void InvalidatePaintForAnimationReset();I think that `ResetAnimation()` is now dead, so you can remove it. (If there's not going to be a need for a flag, then you may need to keep it.)
Also, I think this could just be named `InvalidatePaint()`. It should probably also do nothing if there's no `ImageResourceContent` attached. There's also the case where the `LayoutImageResource` is holding on to a different `ImageResourceContent` than the one currently active in the `ImageLoader`, in which case it ought to do nothing I think.
if (!image_content || !image_content->HasImage()) {
return false;
}
auto* svg_image = DynamicTo<SVGImage>(image_content->GetImage());
return svg_image && svg_image->ShouldResetForReuse();I don't think that this belongs here. `ImageLoader` should just call `ResetAnimation` and leave any details to the `Image`.
void ImageLoader::ResetAnimationAndMaybeInvalidate() {Just call this `ResetAnimation`.
Vector<Persistent<CSSAnimation>> css_animations_pending_resume_;This is not great. It's better to reverse the order here (i.e have a `Persistent<>` that holds a `GCedHeapVector<>`).
// Once this SVGImage has painted with animation content, future reload-like
// reuse should continue to rewind it back to the initial frame.We shouldn't care about users of our interface. Phrase this so that it makes sense from a local perspective.
was_ever_animated_ = was_ever_animated_ || IsCurrentlyAnimated();So if the SVG document does not contain any animations, then on each paint we're going to traverse the entire document to look for them? Not great.
for (CSSAnimation* animation : animations_to_resume) {
if (!animation || animation->ReplaceStateRemoved() ||
!animation->effect()) {
continue;
}
animation->Unpause();This looks like knowledge that would be better to keep on the animation-side (probably together with the `Prepare...` call). Could probably wrap this up into a little helper class with a simple interface.
for (Node& node : NodeTraversal::DescendantsOf(*root_element)) {
if (auto* element = DynamicTo<Element>(node);
element && element->HasAnimations()) {`ElementTraversal`?
if (timer->Value().IsActive()) {
test::RunDelayedTasks(base::Milliseconds(1) +
timer->Value().NextFireInterval());
PumpFrame();
}This looks like bodge. We should know the state of the timer here.
Related details
- Kevin Ellis
- zhenbang Jiang
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. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 14 comments![]( "Open in Gerrit")
- Defer image animation reset until the LayoutImageResource is attached whenzhenbang JiangBreak at 72.
Done
Verification:
- Open https://cdn.svgator.com/testing/reload-issue/index.html
- Wait for the animation to finish
- Refresh the page
- Verify that the animation starts again from the initial frame instead of
remaining on the final frameThis belongs in the bug.
Done
!animation->effect() || !DynamicTo<CSSAnimation>(animation.Get())) {zhenbang JiangUse `IsA<>`.
Done
Compositor().BeginFrame();Why remove the `BeginFrame()` calls - I think this test wants them?
My understanding is that the ordinary image path no longer goes through layout_object_->SetShouldDoFullPaintInvalidation() , so after switching loading from lazy to eager there is no longer a naturally pending frame at this point. If we keep the first BeginFrame() here, it now hits the DCHECK .
It seems that this test previously consumed a frame here only because it was relying on an old shared post-update side effect: image updates would also trigger a full paint invalidation. With this change, that reset/invalidation behavior is intentionally narrowed to SVG images that actually need to be rewound on reuse/reload, so that unrelated image resources are not also reset/invalidated and the fix stays scoped to the SVG animation issue itself.
So my understanding is that keeping the first BeginFrame() here would effectively preserve a dependency on an old shared side effect, rather than on behavior that is essential to the core intent of this test.
I think that `ResetAnimation()` is now dead, so you can remove it. (If there's not going to be a need for a flag, then you may need to keep it.)
Also, I think this could just be named `InvalidatePaint()`. It should probably also do nothing if there's no `ImageResourceContent` attached. There's also the case where the `LayoutImageResource` is holding on to a different `ImageResourceContent` than the one currently active in the `ImageLoader`, in which case it ought to do nothing I think.
Done
if (!image_content || !image_content->HasImage()) {
return false;
}
auto* svg_image = DynamicTo<SVGImage>(image_content->GetImage());
return svg_image && svg_image->ShouldResetForReuse();I don't think that this belongs here. `ImageLoader` should just call `ResetAnimation` and leave any details to the `Image`.
Done
void ImageLoader::ResetAnimationAndMaybeInvalidate() {Just call this `ResetAnimation`.
Done
Vector<Persistent<CSSAnimation>> css_animations_pending_resume_;This is not great. It's better to reverse the order here (i.e have a `Persistent<>` that holds a `GCedHeapVector<>`).
Done
// Once this SVGImage has painted with animation content, future reload-like
// reuse should continue to rewind it back to the initial frame.We shouldn't care about users of our interface. Phrase this so that it makes sense from a local perspective.
Done
was_ever_animated_ = was_ever_animated_ || IsCurrentlyAnimated();So if the SVG document does not contain any animations, then on each paint we're going to traverse the entire document to look for them? Not great.
Done
for (CSSAnimation* animation : animations_to_resume) {
if (!animation || animation->ReplaceStateRemoved() ||
!animation->effect()) {
continue;
}
animation->Unpause();This looks like knowledge that would be better to keep on the animation-side (probably together with the `Prepare...` call). Could probably wrap this up into a little helper class with a simple interface.
Done
for (Node& node : NodeTraversal::DescendantsOf(*root_element)) {
if (auto* element = DynamicTo<Element>(node);
element && element->HasAnimations()) {zhenbang Jiang`ElementTraversal`?
Done
if (timer->Value().IsActive()) {
test::RunDelayedTasks(base::Milliseconds(1) +
timer->Value().NextFireInterval());
PumpFrame();
}This looks like bodge. We should know the state of the timer here.
- Fredrik Söderquist
- 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 3 comments![]( "Open in Gerrit")
!animation->effect() || !IsA<CSSAnimation>(animation.Get())) {Insufficient check. A CSSAnimation without an owning element is treated as a programatic animation. This happens for example if you grab a reference to a CSS animation from JavaScript and call play() on it once no longer associated with a style rule. You need to also check that OwningElement is not null.
animation->SetCurrentTimeInternal(AnimationTimeDelta());This is also affecting CSS animations on elements which have no SVG content. This is also assuming the animation has a monotonic (time-based) timeline, and is problematic for animations using scroll-timelines.
TEST_F(DocumentAnimationsTest, PrepareAnimationsForSVGImageResetAndResume) {No SVG in the test body. This seems to suggest a broader effect on animationed content.
Related details
- Fredrik Söderquist
- zhenbang Jiang
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 10 comments![]( "Open in Gerrit")
class CORE_EXPORT SVGImageAnimationReset finalThis is a weird name. Maybe it should be something like `AnimationsToRewind`, `AnimationsToReset` or something like that?
// SVGImage rewind happens on the main thread without a JS context. Rewind CSS
// animations before the next paint and defer resuming them until after that
// paint so the first sampled frame stays at time=0. Explicitly pausedTalking about paint here seems a bit weird - this is a concept for the user of this function/class.
Compositor().BeginFrame();zhenbang JiangWhy remove the `BeginFrame()` calls - I think this test wants them?
My understanding is that the ordinary image path no longer goes through layout_object_->SetShouldDoFullPaintInvalidation() , so after switching loading from lazy to eager there is no longer a naturally pending frame at this point. If we keep the first BeginFrame() here, it now hits the DCHECK .
It seems that this test previously consumed a frame here only because it was relying on an old shared post-update side effect: image updates would also trigger a full paint invalidation. With this change, that reset/invalidation behavior is intentionally narrowed to SVG images that actually need to be rewound on reuse/reload, so that unrelated image resources are not also reset/invalidated and the fix stays scoped to the SVG animation issue itself.
So my understanding is that keeping the first BeginFrame() here would effectively preserve a dependency on an old shared side effect, rather than on behavior that is essential to the core intent of this test.
Other images will need the paint invalidation as well - `BitmapImage::ResetAnimation()` for example has side-effects and would need to repaint to signal the "rewind".
if (!image->ShouldResetAnimationForReuse()) {
return;
}This shouldn't be here. Let the `ResetAnimation()` implementations handle this.
bool was_ever_animated_ = false;
mutable std::optional<bool> contains_animations_;
bool contains_animations_cache_initialized_ = false;This looks complicated - and related to each other. Could we do a small state machine enum instead? `has_pending_timeline_rewind_` might fit in that as well.
bool ShouldResetForReuse() const;Shouldn't be public. (If it should exist at all.)
css_animation_reset_(MakeGarbageCollected<SVGImageAnimationReset>()),Do we need to allocate this up-front? Doesn't seem so.
.GetDocumentAnimations()
.PrepareAnimationsForSVGImageReset(*css_animation_reset_);
}
has_pending_timeline_rewind_ = false;
}
void SVGImage::StartAnimation() {
SVGSVGElement* root_element = RootElement();
if (!root_element)
return;
css_animation_reset_->Resume();Remind me again why we can't just rewind in `FlushPendingTimelineRewind()`. Are there side-effects?
bool contains_animations = false;
SVGSVGElement* root_element = RootElement();
if (root_element) {
const Document& document = root_element->GetDocument();
if (HasSmilAnimations(document) || root_element->HasAnimations()) {
contains_animations = true;
} else {
for (Element& element : ElementTraversal::DescendantsOf(*root_element)) {
if (element.HasAnimations()) {
contains_animations = true;
break;
}
}
}
}
if (contains_animations || contains_animations_cache_initialized_) {
contains_animations_ = contains_animations;
}It might be better to separate the cached version from the uncached version. It may also be worth considering to handle this in `ServiceAnimations` (where we should have a good idea about what animations are going to run).
contains_animations_.reset();
contains_animations_cache_initialized_ = false;
was_ever_animated_ = false;
has_pending_timeline_rewind_ = false;Use default initializers.
Related details
- zhenbang Jiang
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
!animation->effect() || !IsA<CSSAnimation>(animation.Get())) {Insufficient check. A CSSAnimation without an owning element is treated as a programatic animation. This happens for example if you grab a reference to a CSS animation from JavaScript and call play() on it once no longer associated with a style rule. You need to also check that OwningElement is not null.
SVG images don't support javascript, so is this an issue?
animation->SetCurrentTimeInternal(AnimationTimeDelta());This is also affecting CSS animations on elements which have no SVG content. This is also assuming the animation has a monotonic (time-based) timeline, and is problematic for animations using scroll-timelines.
Can you have scroll timeline animations in SVG? SVG images aren't interactive, so I think this isn't an issue.
zhenbang Jiang (Gerrit)
zhenbang Jiang added 14 comments![]( "Open in Gerrit")
PTAL, thanks~
This is a weird name. Maybe it should be something like `AnimationsToRewind`, `AnimationsToReset` or something like that?
Done
// SVGImage rewind happens on the main thread without a JS context. Rewind CSS
// animations before the next paint and defer resuming them until after that
// paint so the first sampled frame stays at time=0. Explicitly pausedTalking about paint here seems a bit weird - this is a concept for the user of this function/class.
Done
!animation->effect() || !IsA<CSSAnimation>(animation.Get())) {Philip RogersInsufficient check. A CSSAnimation without an owning element is treated as a programatic animation. This happens for example if you grab a reference to a CSS animation from JavaScript and call play() on it once no longer associated with a style rule. You need to also check that OwningElement is not null.
SVG images don't support javascript, so is this an issue?
Even though SVG images do not support JavaScript, this helper lives on DocumentAnimations and should only operate on CSS animations that are still style-owned.
A CSSAnimation without an OwningElement is no longer associated with a style rule and should not be rewound/resumed by this SVG image reset path. I added the OwningElement() check so those animations are skipped.
animation->SetCurrentTimeInternal(AnimationTimeDelta());Philip RogersThis is also affecting CSS animations on elements which have no SVG content. This is also assuming the animation has a monotonic (time-based) timeline, and is problematic for animations using scroll-timelines.
Can you have scroll timeline animations in SVG? SVG images aren't interactive, so I think this isn't an issue.
I added two filters to guard against unintended side effects:
Skip non-monotonic timelines before setting current time to 0, so scroll/view timelines are not affected.
Only handle CSS animations whose KeyframeEffect target is an SVG element, so animations on non-SVG content are not rewound by this SVG image reset path.
Together with the OwningElement() check, this keeps the helper limited to style-owned CSS animations on SVG content using time-based timelines.
TEST_F(DocumentAnimationsTest, PrepareAnimationsForSVGImageResetAndResume) {No SVG in the test body. This seems to suggest a broader effect on animationed content.
Done
Compositor().BeginFrame();zhenbang JiangWhy remove the `BeginFrame()` calls - I think this test wants them?
Fredrik SöderquistMy understanding is that the ordinary image path no longer goes through layout_object_->SetShouldDoFullPaintInvalidation() , so after switching loading from lazy to eager there is no longer a naturally pending frame at this point. If we keep the first BeginFrame() here, it now hits the DCHECK .
It seems that this test previously consumed a frame here only because it was relying on an old shared post-update side effect: image updates would also trigger a full paint invalidation. With this change, that reset/invalidation behavior is intentionally narrowed to SVG images that actually need to be rewound on reuse/reload, so that unrelated image resources are not also reset/invalidated and the fix stays scoped to the SVG animation issue itself.
So my understanding is that keeping the first BeginFrame() here would effectively preserve a dependency on an old shared side effect, rather than on behavior that is essential to the core intent of this test.
Other images will need the paint invalidation as well - `BitmapImage::ResetAnimation()` for example has side-effects and would need to repaint to signal the "rewind".
Thanks, this is a helpful point.
I kept the generic protocol in ImageLoader: it calls ResetAnimation(kForReuse), and invalidates paint only if the implementation returns true. This keeps the type-specific decision inside each Image implementation.
I did try making BitmapImage participate in this reuse reset path, but that regressed image-animation-paused-exact-frame.tentative.html. As I understand it, that test relies on an animated GIF naturally reaching the green frame before image-animation is set to paused; resetting BitmapImage from this hook rewinds the GIF and changes the frame that ends up being paused.
So BitmapImage currently returns false for kForReuse, not because its reset side effects would not need repaint, but because this particular ImageLoader reuse hook should not reset ordinary GIF playback. SVGImage returns true when it actually needs to rewind for the SVG image reuse/reload case. Other Image implementations can still opt in by returning true if their kForReuse reset is appropriate and has repaint-visible side effects.
if (!image->ShouldResetAnimationForReuse()) {
return;
}This shouldn't be here. Let the `ResetAnimation()` implementations handle this.
Done
bool was_ever_animated_ = false;
mutable std::optional<bool> contains_animations_;
bool contains_animations_cache_initialized_ = false;This looks complicated - and related to each other. Could we do a small state machine enum instead? `has_pending_timeline_rewind_` might fit in that as well.
Done
Shouldn't be public. (If it should exist at all.)
Done
css_animation_reset_(MakeGarbageCollected<SVGImageAnimationReset>()),Do we need to allocate this up-front? Doesn't seem so.
Done
.GetDocumentAnimations()
.PrepareAnimationsForSVGImageReset(*css_animation_reset_);
}
has_pending_timeline_rewind_ = false;
}
void SVGImage::StartAnimation() {
SVGSVGElement* root_element = RootElement();
if (!root_element)
return;
css_animation_reset_->Resume();Remind me again why we can't just rewind in `FlushPendingTimelineRewind()`. Are there side-effects?
We shouldn’t only do the timeline rewind within FlushPendingTimelineRewind().If we did, the very first frame rendered after reset could already be past time=0, which would cause the animation to fail to visibly restart from the start.
This happens because the lifecycle update in the same draw pass can advance any still-running CSS animations right before the reset frame is sampled.To address this, I’ve added logic to also rewind and temporarily pause active CSS animations at this point.These animations will be resumed once playback restarts, while any animations that were already paused will stay in their paused state.
bool contains_animations = false;
SVGSVGElement* root_element = RootElement();
if (root_element) {
const Document& document = root_element->GetDocument();
if (HasSmilAnimations(document) || root_element->HasAnimations()) {
contains_animations = true;
} else {
for (Element& element : ElementTraversal::DescendantsOf(*root_element)) {
if (element.HasAnimations()) {
contains_animations = true;
break;
}
}
}
}
if (contains_animations || contains_animations_cache_initialized_) {
contains_animations_ = contains_animations;
}It might be better to separate the cached version from the uncached version. It may also be worth considering to handle this in `ServiceAnimations` (where we should have a good idea about what animations are going to run).
Done
contains_animations_.reset();
contains_animations_cache_initialized_ = false;
was_ever_animated_ = false;
has_pending_timeline_rewind_ = false;zhenbang JiangUse default initializers.
Done
Related details
- Fredrik Söderquist
- Kevin Ellis
- Philip Rogers
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 3 comments![]( "Open in Gerrit")
!animation->effect() || !IsA<CSSAnimation>(animation.Get())) {Philip RogersInsufficient check. A CSSAnimation without an owning element is treated as a programatic animation. This happens for example if you grab a reference to a CSS animation from JavaScript and call play() on it once no longer associated with a style rule. You need to also check that OwningElement is not null.
zhenbang JiangSVG images don't support javascript, so is this an issue?
Even though SVG images do not support JavaScript, this helper lives on DocumentAnimations and should only operate on CSS animations that are still style-owned.
A CSSAnimation without an OwningElement is no longer associated with a style rule and should not be rewound/resumed by this SVG image reset path. I added the OwningElement() check so those animations are skipped.
A CSS animation / transition or programmatic animation can affect an SVG element if the SVG is not embedded in an IMG tag, but rather injected inline. Comments aside, this method is not differentiating how the SVG in being included. It might be as simple as adding a check that the root element is <SVG>.
That said, document_animations_test.cc is using an inline SVG. Should the reset be applied in this case or only in SVG within an IMG case.
If we ensure that this method is only being used for isolated SVG document's then the check for owning element and timeline time, or even the effect target are indeed irrelevant.
animation->SetCurrentTimeInternal(AnimationTimeDelta());Philip RogersThis is also affecting CSS animations on elements which have no SVG content. This is also assuming the animation has a monotonic (time-based) timeline, and is problematic for animations using scroll-timelines.
zhenbang JiangCan you have scroll timeline animations in SVG? SVG images aren't interactive, so I think this isn't an issue.
I added two filters to guard against unintended side effects:
Skip non-monotonic timelines before setting current time to 0, so scroll/view timelines are not affected.
Only handle CSS animations whose KeyframeEffect target is an SVG element, so animations on non-SVG content are not rewound by this SVG image reset path.Together with the OwningElement() check, this keeps the helper limited to style-owned CSS animations on SVG content using time-based timelines.
@pdr: You could have an animation affecting an SVG that is linked to a scroll-timeline attached to the enclosing scroll container.
!To<CSSAnimation>(*animation).OwningElement()) {```
... !animation->IsCSSAnimation() || !animation->OwningElement() ...
```
No need to cast.
Related details
- Fredrik Söderquist
- Philip Rogers
- zhenbang Jiang
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 added 2 comments![]( "Open in Gerrit")
SVGImageAnimationsToReset& animations_to_reset) {To alleviate my concerns over calling the method on the wrong type of document and applying too generally, can you add a check.
```
DCHECK(document_.View()->GetFrame().GetChromeClient().IsIsolatedSVGChromeClient());
```
If we ensure that we are not calling this method on the document associated with the HTML element then all of the safeguards for timeline time and the type of animation are no longer required. My earlier reservations were based on the fact that this could result in non-isolated animations being reset.
!To<CSSAnimation>(*animation).OwningElement()) {```
... !animation->IsCSSAnimation() || !animation->OwningElement() ...
```
No need to cast.
No longer relevant with the proposed check to enforce restriction to SVG documents.
zhenbang Jiang (Gerrit)
zhenbang Jiang added 3 comments![]( "Open in Gerrit")
To alleviate my concerns over calling the method on the wrong type of document and applying too generally, can you add a check.
```
DCHECK(document_.View()->GetFrame().GetChromeClient().IsIsolatedSVGChromeClient());
```If we ensure that we are not calling this method on the document associated with the HTML element then all of the safeguards for timeline time and the type of animation are no longer required. My earlier reservations were based on the fact that this could result in non-isolated animations being reset.
Done
!animation->effect() || !IsA<CSSAnimation>(animation.Get())) {Philip RogersInsufficient check. A CSSAnimation without an owning element is treated as a programatic animation. This happens for example if you grab a reference to a CSS animation from JavaScript and call play() on it once no longer associated with a style rule. You need to also check that OwningElement is not null.
zhenbang JiangSVG images don't support javascript, so is this an issue?
Kevin EllisEven though SVG images do not support JavaScript, this helper lives on DocumentAnimations and should only operate on CSS animations that are still style-owned.
A CSSAnimation without an OwningElement is no longer associated with a style rule and should not be rewound/resumed by this SVG image reset path. I added the OwningElement() check so those animations are skipped.
A CSS animation / transition or programmatic animation can affect an SVG element if the SVG is not embedded in an IMG tag, but rather injected inline. Comments aside, this method is not differentiating how the SVG in being included. It might be as simple as adding a check that the root element is <SVG>.
That said, document_animations_test.cc is using an inline SVG. Should the reset be applied in this case or only in SVG within an IMG case.
If we ensure that this method is only being used for isolated SVG document's then the check for owning element and timeline time, or even the effect target are indeed irrelevant.
Added. I narrowed the contract of this helper to isolated SVG documents and added a DCHECK for IsIsolatedSVGChromeClient() at the entry point.
That makes the intended scope explicit and avoids applying this reset path to regular HTML documents.
Related details
- Fredrik Söderquist
- Kevin Ellis
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 19 comments![]( "Open in Gerrit")
Compositor().BeginFrame();zhenbang JiangWhy remove the `BeginFrame()` calls - I think this test wants them?
Fredrik SöderquistMy understanding is that the ordinary image path no longer goes through layout_object_->SetShouldDoFullPaintInvalidation() , so after switching loading from lazy to eager there is no longer a naturally pending frame at this point. If we keep the first BeginFrame() here, it now hits the DCHECK .
It seems that this test previously consumed a frame here only because it was relying on an old shared post-update side effect: image updates would also trigger a full paint invalidation. With this change, that reset/invalidation behavior is intentionally narrowed to SVG images that actually need to be rewound on reuse/reload, so that unrelated image resources are not also reset/invalidated and the fix stays scoped to the SVG animation issue itself.
So my understanding is that keeping the first BeginFrame() here would effectively preserve a dependency on an old shared side effect, rather than on behavior that is essential to the core intent of this test.
zhenbang JiangOther images will need the paint invalidation as well - `BitmapImage::ResetAnimation()` for example has side-effects and would need to repaint to signal the "rewind".
Thanks, this is a helpful point.
I kept the generic protocol in ImageLoader: it calls ResetAnimation(kForReuse), and invalidates paint only if the implementation returns true. This keeps the type-specific decision inside each Image implementation.
I did try making BitmapImage participate in this reuse reset path, but that regressed image-animation-paused-exact-frame.tentative.html. As I understand it, that test relies on an animated GIF naturally reaching the green frame before image-animation is set to paused; resetting BitmapImage from this hook rewinds the GIF and changes the frame that ends up being paused.
So BitmapImage currently returns false for kForReuse, not because its reset side effects would not need repaint, but because this particular ImageLoader reuse hook should not reset ordinary GIF playback. SVGImage returns true when it actually needs to rewind for the SVG image reuse/reload case. Other Image implementations can still opt in by returning true if their kForReuse reset is appropriate and has repaint-visible side effects.
There shouldn't be a `kForReuse` argument. Please remove it. The `image-animation-paused-exact-frame.tentative.html` test looks very fragile (and I'm not sure it necessarily tests what it sets out to test) - in which way is it failing? It's not impossible that this end up exposing an issue with our "reset animation flag" handling (it should only be done under certain conditions). The answer is however not to stop resetting `BitmapImage`s.
case AnimationState::kAnimatedRewindPending:This (and `kUnknownRewindPending`) could just `break` (stay in the same state).
case AnimationState::kNotAnimated:Why would this switch to the `kUnknownRewindPending` state?
.GetDocumentAnimations()
.PrepareAnimationsForSVGImageReset(*css_animation_reset_);
}
has_pending_timeline_rewind_ = false;
}
void SVGImage::StartAnimation() {
SVGSVGElement* root_element = RootElement();
if (!root_element)
return;
css_animation_reset_->Resume();zhenbang JiangRemind me again why we can't just rewind in `FlushPendingTimelineRewind()`. Are there side-effects?
We shouldn’t only do the timeline rewind within FlushPendingTimelineRewind().If we did, the very first frame rendered after reset could already be past time=0, which would cause the animation to fail to visibly restart from the start.
This happens because the lifecycle update in the same draw pass can advance any still-running CSS animations right before the reset frame is sampled.To address this, I’ve added logic to also rewind and temporarily pause active CSS animations at this point.These animations will be resumed once playback restarts, while any animations that were already paused will stay in their paused state.
Well, `FlushPendingTimelineRewind()` is called right before performing the lifecycle update, so if the animations are not reset after it, the paint that follows _will_ be with an incorrect time for these animations. The `StartAnimation()` call that then follows is intended to restart the timeline for animations. I think the problem here may rather be that the CSS animations aren't following the same timeline control (and thus will always tick when `ServiceAnimations()` is called).
css_animations_to_reset_->Resume();This can drop the object after it's been used.
if (option == ResetAnimationOption::kForReuse && !ShouldResetForReuse()) {I don't see why this would even be needed. If there are no animations, then the below should effectively be a no-op. The term "reuse" makes no sense in this context.
bool SVGImage::ShouldResetForReuse() const {
switch (animation_state_) {
case AnimationState::kAnimated:
case AnimationState::kAnimatedRewindPending:
return true;
case AnimationState::kNotAnimated:
return false;
case AnimationState::kUnknown:
case AnimationState::kUnknownRewindPending:
return IsCurrentlyAnimated();
}
}This is effectively the same as `IsCurrentlyAnimated()`. (Which I guess then transitively means that `IsCurrentlyAnimated()` and `MaybeAnimated()` are identical, and both aren't needed.)
contains_animations = true;`return true`
contains_animations = true;`return true`
return contains_animations;`return false` (variable not needed)
const bool contains_animations = DetectAnimatedContent();
if (contains_animations) {No need to have this local variable
if (animation_state_ == AnimationState::kUnknown) {Don't we want both states to transition to `kNotAnimated`?
animation_state_ = AnimationState::kUnknown;Why set this here?
animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(1));
ASSERT_TRUE(animation->CurrentTimeInternal().has_value());
EXPECT_EQ(animation->CurrentTimeInternal().value(),
ANIMATION_TIME_DELTA_FROM_SECONDS(1));Could we test this by advancing the time within the whole image instead rather than mutating specific animations?
animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(1));
ASSERT_TRUE(animation->CurrentTimeInternal().has_value());
EXPECT_EQ(animation->CurrentTimeInternal().value(),
ANIMATION_TIME_DELTA_FROM_SECONDS(1));Ditto here.
animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(0.5));
ASSERT_TRUE(animation->CurrentTimeInternal().has_value());
EXPECT_EQ(animation->CurrentTimeInternal().value(),
ANIMATION_TIME_DELTA_FROM_SECONDS(0.5));Ditto here.
animation->pause();How will this happen within an `SVGImage`? Can we write the test in such a way instead of using this synthetic method?
first_animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(1));Could we achieve this by advancing the image's timeline instead?
Related details
- Kevin Ellis
- Philip Rogers
- zhenbang Jiang
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 added 1 comment![]( "Open in Gerrit")
Removing myself from the attention set until the unresolved comments have been addressed. Will take a look once ready for re-review.
Related details
- Philip Rogers
- zhenbang Jiang
zhenbang Jiang (Gerrit)
zhenbang Jiang added 20 comments![]( "Open in Gerrit")
Plz take a look again, thanks!
Compositor().BeginFrame();zhenbang JiangWhy remove the `BeginFrame()` calls - I think this test wants them?
Fredrik SöderquistMy understanding is that the ordinary image path no longer goes through layout_object_->SetShouldDoFullPaintInvalidation() , so after switching loading from lazy to eager there is no longer a naturally pending frame at this point. If we keep the first BeginFrame() here, it now hits the DCHECK .
It seems that this test previously consumed a frame here only because it was relying on an old shared post-update side effect: image updates would also trigger a full paint invalidation. With this change, that reset/invalidation behavior is intentionally narrowed to SVG images that actually need to be rewound on reuse/reload, so that unrelated image resources are not also reset/invalidated and the fix stays scoped to the SVG animation issue itself.
So my understanding is that keeping the first BeginFrame() here would effectively preserve a dependency on an old shared side effect, rather than on behavior that is essential to the core intent of this test.
zhenbang JiangOther images will need the paint invalidation as well - `BitmapImage::ResetAnimation()` for example has side-effects and would need to repaint to signal the "rewind".
Fredrik SöderquistThanks, this is a helpful point.
I kept the generic protocol in ImageLoader: it calls ResetAnimation(kForReuse), and invalidates paint only if the implementation returns true. This keeps the type-specific decision inside each Image implementation.
I did try making BitmapImage participate in this reuse reset path, but that regressed image-animation-paused-exact-frame.tentative.html. As I understand it, that test relies on an animated GIF naturally reaching the green frame before image-animation is set to paused; resetting BitmapImage from this hook rewinds the GIF and changes the frame that ends up being paused.
So BitmapImage currently returns false for kForReuse, not because its reset side effects would not need repaint, but because this particular ImageLoader reuse hook should not reset ordinary GIF playback. SVGImage returns true when it actually needs to rewind for the SVG image reuse/reload case. Other Image implementations can still opt in by returning true if their kForReuse reset is appropriate and has repaint-visible side effects.
There shouldn't be a `kForReuse` argument. Please remove it. The `image-animation-paused-exact-frame.tentative.html` test looks very fragile (and I'm not sure it necessarily tests what it sets out to test) - in which way is it failing? It's not impossible that this end up exposing an issue with our "reset animation flag" handling (it should only be done under certain conditions). The answer is however not to stop resetting `BitmapImage`s.
Done. Removed kForReuse and kept ImageLoader generic. BitmapImage still resets and requests repaint for real rewinds, but it now avoids emitting a reset sequence before any cached PaintImage exists. In that initial-load state there is no existing bitmap animation timeline to rewind or painted frame to invalidate; emitting the sequence there caused image-animation-paused-exact-frame.tentative.html to reset the GIF too early. This matches the “reset flag should only be done under certain conditions” point without stopping BitmapImage reset.
This (and `kUnknownRewindPending`) could just `break` (stay in the same state).
Done
Why would this switch to the `kUnknownRewindPending` state?
Done. kNotAnimated no longer transitions to kUnknownRewindPending, and kUnknownRewindPending now just keeps its state. Only kUnknown can schedule an unknown pending rewind.
.GetDocumentAnimations()
.PrepareAnimationsForSVGImageReset(*css_animation_reset_);
}
has_pending_timeline_rewind_ = false;
}
void SVGImage::StartAnimation() {
SVGSVGElement* root_element = RootElement();
if (!root_element)
return;
css_animation_reset_->Resume();zhenbang JiangRemind me again why we can't just rewind in `FlushPendingTimelineRewind()`. Are there side-effects?
Fredrik SöderquistWe shouldn’t only do the timeline rewind within FlushPendingTimelineRewind().If we did, the very first frame rendered after reset could already be past time=0, which would cause the animation to fail to visibly restart from the start.
This happens because the lifecycle update in the same draw pass can advance any still-running CSS animations right before the reset frame is sampled.To address this, I’ve added logic to also rewind and temporarily pause active CSS animations at this point.These animations will be resumed once playback restarts, while any animations that were already paused will stay in their paused state.
Well, `FlushPendingTimelineRewind()` is called right before performing the lifecycle update, so if the animations are not reset after it, the paint that follows _will_ be with an incorrect time for these animations. The `StartAnimation()` call that then follows is intended to restart the timeline for animations. I think the problem here may rather be that the CSS animations aren't following the same timeline control (and thus will always tick when `ServiceAnimations()` is called).
Done. The rewind still happens in FlushPendingTimelineRewind(); ResetAnimation() only schedules it. The extra work there is specifically for CSS animations, since they can tick during ServiceAnimations() and don't fully follow the SVG timeline control. We now reset their current time to 0 and temporarily pause only running CSS animations until StartAnimation() resumes them. Explicitly paused animations remain paused. This helper is restricted to isolated SVG image documents.
This can drop the object after it's been used.
Done. StartAnimation() now copies css_animations_to_reset_ into a local Persistent, clears the member, and then calls Resume() through the local Persistent. This keeps the helper alive during Resume() while avoiding retaining the stale reset list afterwards.
if (option == ResetAnimationOption::kForReuse && !ShouldResetForReuse()) {I don't see why this would even be needed. If there are no animations, then the below should effectively be a no-op. The term "reuse" makes no sense in this context.
one. Removed kForReuse and ShouldResetForReuse(). ResetAnimation() now uses the AnimationState state machine directly; kNotAnimated naturally produces no pending rewind and returns false, while animated/unknown states can schedule one. ImageLoader just calls the parameterless Image::ResetAnimation().
bool SVGImage::ShouldResetForReuse() const {
switch (animation_state_) {
case AnimationState::kAnimated:
case AnimationState::kAnimatedRewindPending:
return true;
case AnimationState::kNotAnimated:
return false;
case AnimationState::kUnknown:
case AnimationState::kUnknownRewindPending:
return IsCurrentlyAnimated();
}
}This is effectively the same as `IsCurrentlyAnimated()`. (Which I guess then transitively means that `IsCurrentlyAnimated()` and `MaybeAnimated()` are identical, and both aren't needed.)
Removed ShouldResetForReuse() and IsCurrentlyAnimated(). MaybeAnimated() now uses AnimationState directly and falls back to DetectAnimatedContent() only for unknown states.
contains_animations = true;zhenbang Jiang`return true`
Done
contains_animations = true;zhenbang Jiang`return true`
Done
`return false` (variable not needed)
Done
const bool contains_animations = DetectAnimatedContent();
if (contains_animations) {No need to have this local variable
Done. Removed the local contains_animations variable and now call DetectAnimatedContent() directly. If no animations are detected, both kUnknown and kUnknownRewindPending transition to kNotAnimated.
if (animation_state_ == AnimationState::kUnknown) {Don't we want both states to transition to `kNotAnimated`?
Done. When DetectAnimatedContent() returns false, both kUnknown and kUnknownRewindPending now fall through to the same kNotAnimated assignment.
animation_state_ = AnimationState::kUnknown;zhenbang JiangWhy set this here?
Removed the explicit assignment from the initialization path and rely on the member default initializer: animation_state_ now defaults to kUnknown in SVGImage.
animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(1));
ASSERT_TRUE(animation->CurrentTimeInternal().has_value());
EXPECT_EQ(animation->CurrentTimeInternal().value(),
ANIMATION_TIME_DELTA_FROM_SECONDS(1));Could we test this by advancing the time within the whole image instead rather than mutating specific animations?
Done
animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(1));
ASSERT_TRUE(animation->CurrentTimeInternal().has_value());
EXPECT_EQ(animation->CurrentTimeInternal().value(),
ANIMATION_TIME_DELTA_FROM_SECONDS(1));zhenbang JiangDitto here.
Done
animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(0.5));
ASSERT_TRUE(animation->CurrentTimeInternal().has_value());
EXPECT_EQ(animation->CurrentTimeInternal().value(),
ANIMATION_TIME_DELTA_FROM_SECONDS(0.5));zhenbang JiangDitto here.
Done
How will this happen within an `SVGImage`? Can we write the test in such a way instead of using this synthetic method?
Done. The test no longer mutates the Animation directly or calls pause() synthetically. It now uses SVG content with a CSS-paused animation (`animation: ... paused`) and verifies that ResetAnimation() preserves that paused state instead of collecting it for resume.
first_animation->SetCurrentTimeInternal(ANIMATION_TIME_DELTA_FROM_SECONDS(1));Could we achieve this by advancing the image's timeline instead?
- Fredrik Söderquist
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 7 comments![]( "Open in Gerrit")
if (!image->ResetAnimation()) {
return;
}Let's move this repaint optimization to a separate CL. This CL is big enough as it is, and we'll be better off keeping the bug fix and the optimization separate.
if (css_animations_to_reset_) {
Persistent<SVGImageAnimationsToReset> animations_to_reset =
css_animations_to_reset_;
css_animations_to_reset_ = nullptr;
animations_to_reset->Resume();
}```suggestion
if (css_animations_to_reset_) {
css_animations_to_reset_.Release()->Resume();
}
```
} else {You can drop this `else` to save on indentation. I'd actually prefer to keep the structure from `MaybeAnimated()` with preferring early returns rather than adding indentation.
Animation* GetBoxAnimation(LocalFrame& local_frame) {Maybe make this take an `id` argument as well. Then you can reuse it for the tests where not all ids are `"box"` as well.
Element* first_box =
first_local_frame->GetDocument()->getElementById(AtomicString("box"));
ASSERT_TRUE(first_box);
HeapVector<Member<Animation>> first_animations = first_box->getAnimations();
ASSERT_EQ(first_animations.size(), 1u);
Animation* first_animation = first_animations[0].Get();Use the helper?
Element* hidden_box =
second_local_frame->GetDocument()->getElementById(AtomicString("box"));
ASSERT_TRUE(hidden_box);
HeapVector<Member<Animation>> hidden_animations = hidden_box->getAnimations();
ASSERT_EQ(hidden_animations.size(), 1u);
Animation* hidden_animation = hidden_animations[0].Get();Ditto
Element* visible_box =
second_local_frame->GetDocument()->getElementById(AtomicString("box"));
ASSERT_TRUE(visible_box);
HeapVector<Member<Animation>> visible_animations =
visible_box->getAnimations();
ASSERT_EQ(visible_animations.size(), 1u);
Animation* visible_animation = visible_animations[0].Get();Ditto
- Philip Rogers
- zhenbang Jiang
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. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 8 comments![]( "Open in Gerrit")
Let's move this repaint optimization to a separate CL. This CL is big enough as it is, and we'll be better off keeping the bug fix and the optimization separate.
Done.
I removed the repaint optimization from this CL. ImageLoader now calls Image::ResetAnimation() as a side-effect-only API and still invalidates paint when the active LayoutImageResource matches the current ImageResourceContent.
I also changed ResetAnimation() back to void, so this CL no longer carries a “can skip repaint” signal. BitmapImage still participates in reset, and SVGImage keeps the state-machine based reset behavior.
The repaint-skipping optimization can be handled in a follow-up CL.
if (css_animations_to_reset_) {
Persistent<SVGImageAnimationsToReset> animations_to_reset =
css_animations_to_reset_;
css_animations_to_reset_ = nullptr;
animations_to_reset->Resume();
}```suggestion
if (css_animations_to_reset_) {
css_animations_to_reset_.Release()->Resume();
}
```
Done
You can drop this `else` to save on indentation. I'd actually prefer to keep the structure from `MaybeAnimated()` with preferring early returns rather than adding indentation.
Done
Animation* GetBoxAnimation(LocalFrame& local_frame) {Maybe make this take an `id` argument as well. Then you can reuse it for the tests where not all ids are `"box"` as well.
Done
Element* first_box =
first_local_frame->GetDocument()->getElementById(AtomicString("box"));
ASSERT_TRUE(first_box);
HeapVector<Member<Animation>> first_animations = first_box->getAnimations();
ASSERT_EQ(first_animations.size(), 1u);
Animation* first_animation = first_animations[0].Get();zhenbang JiangUse the helper?
Done
Element* hidden_box =
second_local_frame->GetDocument()->getElementById(AtomicString("box"));
ASSERT_TRUE(hidden_box);
HeapVector<Member<Animation>> hidden_animations = hidden_box->getAnimations();
ASSERT_EQ(hidden_animations.size(), 1u);
Animation* hidden_animation = hidden_animations[0].Get();zhenbang JiangDitto
Done
Element* visible_box =
second_local_frame->GetDocument()->getElementById(AtomicString("box"));
ASSERT_TRUE(visible_box);
HeapVector<Member<Animation>> visible_animations =
visible_box->getAnimations();
ASSERT_EQ(visible_animations.size(), 1u);
Animation* visible_animation = visible_animations[0].Get();- Fredrik Söderquist
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fredrik Söderquist
| Code-Review | +1 |
1 comment
LGTM for the SVG/layout/loader bits, but perhaps we should add a flag for this just in case?
Related details
- Philip Rogers
- zhenbang Jiang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
LGTM for the SVG/layout/loader bits, but perhaps we should add a flag for this just in case?
I added a Blink runtime-enabled feature for this behavior: SvgImageAnimationReset.
I set it to stable so the fix remains enabled by default, and the flag can work as a kill switch if needed. I'm not fully sure whether stable is the preferred status for this case, so please let me know if you'd prefer a different status/default setup.
The flag is checked at the ImageLoader reset entry point, and SVGImage::ResetAnimation() also checks it to cover direct reset callers. When the flag is disabled, the new SVG image reset/invalidation path is skipped.
I also added a test covering the disabled-flag case to verify that the cached detached SVG image is not rewound when SvgImageAnimationReset is disabled.
Could you please take another look and let me know if this is the right way to add the flag?
Related details
- Fredrik Söderquist
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
if (!RuntimeEnabledFeatures::SvgImageAnimationResetEnabled()) {Now when the flag is off, the old behavior is not maintained. You'll need to keep the old `LayoutImageResource::ResetAnimation()` and essentially all the old code and most likely check the flag in a few other places. For `SVGImage` this will be complicated I think (landing a no-op refactoring there first might make it easier).
So here I'd expect something like:
```
if (RuntimeEnabledFeatures::SvgImageAnimationResetEnabled()) {
...the new code...
} else {
...the old code...
}
```
Related details
- Philip Rogers
- zhenbang Jiang
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. |