Bug 186129

Created attachment 341648 [details] Testcase

Started with https://trac.webkit.org/r232186. Perhaps an intentional change?

r232186 should not affect WebKit unless you have DEFAULT_EXPERIMENTAL_FEATURES_ENABLED set to TRUE. Also, the timing function code is not affected by the Web Animations flags.

Looks like a bug with the new CSS Animations implementation on top of Web Animations. This does not reproduce with the Web Animations CSS Integration flag off.

<rdar://problem/40694333>

To be clear, I positively bisected this regression to r232186. That's not a guess.

Created attachment 341744 [details] Testcase This is an extended testcase that logs the getComputedStyle() of the square, showing that the transform is correctly updated in both cases.

Animating margin-left instead of transform behaves as expected. This is an issue with hardware-accelerated animations only.

(In reply to Antoine Quint from comment #8) > Animating margin-left instead of transform behaves as expected. This is an > issue with hardware-accelerated animations only. Also, note that GraphicsLayerCA::animationCanBeAccelerated returns false for the steps() timing function (which is the case failing in the attached testcase). So maybe the implementation with webAnimationCSSIntegration enabled does not handle transform for non-accelerated animations?

Created attachment 342943 [details] Testcase Testcase, including a CSS propertie that cannot be accelerated.

(In reply to Frédéric Wang (:fredw) from comment #9) > (In reply to Antoine Quint from comment #8) > > Animating margin-left instead of transform behaves as expected. This is an > > issue with hardware-accelerated animations only. > > Also, note that GraphicsLayerCA::animationCanBeAccelerated returns false for > the steps() timing function (which is the case failing in the attached > testcase). So maybe the implementation with webAnimationCSSIntegration > enabled does not handle transform for non-accelerated animations? Comparing the stack traces, webAnimationCSSIntegration disabled gives: WebCore::GraphicsLayerCA::animationCanBeAccelerated WebCore::GraphicsLayerCA::addAnimation WebCore::RenderLayerBacking::startAnimation WebCore::RenderBoxModelObject::startAnimation WebCore::KeyframeAnimation::startAnimation WebCore::AnimationBase::updateStateMachine ... while webAnimationCSSIntegration enabled gives: WebCore::GraphicsLayerCA::animationCanBeAccelerated WebCore::GraphicsLayerCA::addAnimation WebCore::RenderLayerBacking::startAnimation WebCore::RenderBoxModelObject::startAnimation WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions WebCore::WebAnimation::applyPendingAcceleratedActions ... Note that in the former case, the AnimationBase becomes non-accelerated when startAnimation returns false (e.g. when animationCanBeAccelerated returns false) but in the latter case, the WebAnimation just ignores the boolean returned by startAnimation. It looks like it should somewhat fallback to non-accelerated mode too.

(In reply to Frédéric Wang (:fredw) from comment #11) > Note that in the former case, the AnimationBase becomes non-accelerated when > startAnimation returns false (e.g. when animationCanBeAccelerated returns > false) but in the latter case, the WebAnimation just ignores the boolean > returned by startAnimation. It looks like it should somewhat fallback to > non-accelerated mode too. OK, just checked again this morning and indeed this seems to be enough: --- a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp +++ b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp @@ -1256,3 +1256,6 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions() case AcceleratedAction::Play: - compositedRenderer->startAnimation(timeOffset, backingAnimationForCompositedRenderer().ptr(), m_blendingKeyframes); + if (!compositedRenderer->startAnimation(timeOffset, backingAnimationForCompositedRenderer().ptr(), m_blendingKeyframes)) { + m_shouldRunAccelerated = false; + m_lastRecordedAcceleratedAction = AcceleratedAction::Stop; + } break;

Created attachment 344178 [details] Patch

Comment on attachment 344178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344178&action=review > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1259 > + m_lastRecordedAcceleratedAction = AcceleratedAction::Stop; I think we should clear the list of pending accelerated actions and return here since we already know we can't actually run accelerated.

Comment on attachment 344178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344178&action=review >> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1259 >> + m_lastRecordedAcceleratedAction = AcceleratedAction::Stop; > > I think we should clear the list of pending accelerated actions and return here since we already know we can't actually run accelerated. Right, we can just return immediately. m_pendingAcceleratedActions.clear(); is already called at the beginning of the function.

Created attachment 344186 [details] Patch for landing

Comment on attachment 344186 [details] Patch for landing Clearing flags on attachment: 344186 Committed r233462: <https://trac.webkit.org/changeset/233462>

All reviewed patches have been landed. Closing bug.