[webkit-changes] [WebKit/WebKit] 5a8f69: Fix reset to baseVal after animation end for SVGTr...

[Nikolas Zimmermann]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 5a8f69dc67ce414cca0c9575080c73d858a603bc
      https://github.com/WebKit/WebKit/commit/5a8f69dc67ce414cca0c9575080c73d858a603bc
  Author: Nikolas Zimmermann <nzimmermann at igalia.com>
  Date:   2023-10-06 (Fri, 06 Oct 2023)

  Changed paths:
    M LayoutTests/platform/mac-ventura-wk2-lbse-text/TestExpectations
    M Source/WebCore/svg/properties/SVGAnimatedPropertyAnimator.h

  Log Message:
  -----------
  Fix reset to baseVal after animation end for SVGTransformList
https://bugs.webkit.org/show_bug.cgi?id=249140

Reviewed by Rob Buis.

Partly revert the fix from webkit.org/b/198576 (REGRESSION (r243121): Load event
should not be fired while animating the 'externalResourcesRequired' attribute),
that landed in https://commits.webkit.org/212621@main.

First of all, externalResourcesRequired support is gone from WebKit, therefore
SMIL animations of that property are no longer possible: the workaround is obsolete.

More importantly, the applied workaround is harmful for SVGTransformList animations,
which go through the same SVGAnimatedPropertyAnimator code paths to apply changes,
when <animateTransform> animations are active.

The now-reverted patch changed the order of two calls, when applying property
animations. Before the patch, stopAnimation() was called first, then
applyAnimatedPropertyChange() (which calls svgAttributeChanged()).

SVGAnimatedPropertyList::stopAnimation() first calls SVGAnimatedProperty::stopAnimation(),
and then resets m_animVal to m_baseVal (#). The SVGAnimatedProperty::stopAnimation() code
removes the 'SVGAttributeAnimator' object from the m_animators hash set. If there was
only one animation running, isAnimating() would now return false. Thus when calling
stopAnimation() first and applyAnimatedPropertyChanged() afterwards, one cannot query
anymore if the svgAttributeChanged() origin is a SMIL animation or not (that kind of
information was needed by SVGExternalResourcesRequired support, and nowhere else).

However it is guaranteed that the "m_animVal" is reset to the "m_baseVal" _BEFORE_
calling svgAttributeChanged(), which potentially triggers repaints / relayouts etc.

For the legacy SVG engine this imposes no problem: svgAttributeChanged() _marks_
the renderer for a transform update (renderer()->setNeedsTransformUpdate()) and triggers
an async relayout. Next time layout is updated, the correct values are used to update
and render the scene (property now reflect 'baseVal' visually, animVal was reset).

For LBSE we only want to update the layer transform and repaint, avoiding any relayout.
Therefore the current behaviour in ToT, first calling applyAnimatedPropertyChange()
(which calls svgAttributeChanged()) and then stopAnimation() is harmful.
When svgAttributeChanged() is called, the SVGTransformList state reflects the _LAST_
animation progress value that was calculated. That state is used to update the layer
transform and trigger a repaint. Just afterwards stopAnimation() reset the animVal
to the baseVal (e.g. back to identity matrix, ...) -- but since we no longer trigger
any async relayout from svgAttributeChanged(), nobody is going to pick up that change
and redraw the scene.

The fix is simple: revert the order again, and things are fine.
Solves the recently induced regression in svg/animations/list-wrapper-assertion.svg.

* LayoutTests/platform/mac-ventura-wk2-lbse-text/TestExpectations: Remove svg/animations/list-wrapper-assertion.svg failure expectation.
* Source/WebCore/svg/properties/SVGAnimatedPropertyAnimator.h: Call stopAnimation() before applyAnimatedPropertyChange().

Canonical link: https://commits.webkit.org/268987@main