Skip to content
Permalink
Browse files
Layout Test animations/needs-layout.html is a flaky Image Failure.
https://bugs.webkit.org/show_bug.cgi?id=172397

Reviewed by Dean Jackson.

Source/WebCore:

Animations that animate a transform and uses a relative value for either the x or y components
require a layout before starting, which CSSAnimationController would perform in the call to
CSSAnimationControllerPrivate::animationTimerFired() made immediately after a CSS animation was
created.

We now perform a similar task where upon setting new blending keyframes we compute a flag indicating
if the keyframe effect is animating a transform with relative x or y components. Then, when we perform
the first invalidation task, which runs in the next run loop after a change to the timing model has
been made, such as a call to play() on a CSSAnimation made in the TreeResolver::createAnimatedElementUpdate()
where the CSSAnimation was created, we call forceLayout() on this element's FrameView. We also ensure
we commit animations on the compositor immediately after that too, instead of waiting until the next
DisplayRefreshMonitor callback.

* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::performInvalidationTask):
(WebCore::DocumentTimeline::updateAnimations):
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::forceLayoutIfNeeded):
(WebCore::KeyframeEffectReadOnly::setBlendingKeyframes):
(WebCore::KeyframeEffectReadOnly::computedNeedsForcedLayout):
(WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions):
* animation/KeyframeEffectReadOnly.h:

LayoutTests:

No longer mark this test as flaky.

* platform/ios-wk2/TestExpectations:
* platform/mac-wk1/TestExpectations:
* platform/mac-wk2/TestExpectations:

Canonical link: https://commits.webkit.org/200192@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230703 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
graouts committed Apr 17, 2018
1 parent e3f5df4 commit 1971cead6e22832e8ad85ae032e12003ab307a5d
Showing 8 changed files with 109 additions and 6 deletions.
@@ -1,3 +1,16 @@
2018-04-16 Antoine Quint <graouts@apple.com>

Layout Test animations/needs-layout.html is a flaky Image Failure.
https://bugs.webkit.org/show_bug.cgi?id=172397

Reviewed by Dean Jackson.

No longer mark this test as flaky.

* platform/ios-wk2/TestExpectations:
* platform/mac-wk1/TestExpectations:
* platform/mac-wk2/TestExpectations:

2018-04-16 Keith Rollin <krollin@apple.com>

REGRESSION: [mac-wk2 release] LayoutTest http/tests/security/contentSecurityPolicy/script-src-blocked-error-event.html is flaky
@@ -1319,7 +1319,6 @@ webkit.org/b/173608 webrtc/video-replace-muted-track.html [ Skip ]

webkit.org/b/177501 webrtc/video-mute.html [ Pass Timeout ]

webkit.org/b/172397 [ Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
webkit.org/b/172397 [ Debug ] legacy-animation-engine/animations/needs-layout.html [ Pass ImageOnlyFailure ]

webkit.org/b/179176 [ Release ] svg/wicd/test-rightsizing-a.xhtml [ Pass Failure ]
@@ -492,7 +492,6 @@ webkit.org/b/175886 svg/animations/smil-leak-element-instances.svg [ Pass Failur
[ HighSierra+ ] fast/forms/textarea-maxlength.html [ Crash ]
[ HighSierra+ ] fast/text/system-font-fallback-emoji.html [ Crash ]

webkit.org/b/172397 [ Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
webkit.org/b/172397 [ Debug ] legacy-animation-engine/animations/needs-layout.html [ Pass ImageOnlyFailure ]

webkit.org/b/179775 imported/w3c/web-platform-tests/XMLHttpRequest/firing-events-http-no-content-length.html [ Pass Failure ]
@@ -730,7 +730,6 @@ webkit.org/b/172201 webaudio/silent-audio-interrupted-in-background.html [ Pass

webkit.org/b/167757 workers/bomb.html [ Pass Timeout ]

webkit.org/b/172397 [ Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
webkit.org/b/172397 [ Debug ] legacy-animation-engine/animations/needs-layout.html [ Pass ImageOnlyFailure ]

# Touch events are not available on open source bots, thus only tested on Mac.
@@ -1,3 +1,33 @@
2018-04-16 Antoine Quint <graouts@apple.com>

Layout Test animations/needs-layout.html is a flaky Image Failure.
https://bugs.webkit.org/show_bug.cgi?id=172397

Reviewed by Dean Jackson.

Animations that animate a transform and uses a relative value for either the x or y components
require a layout before starting, which CSSAnimationController would perform in the call to
CSSAnimationControllerPrivate::animationTimerFired() made immediately after a CSS animation was
created.

We now perform a similar task where upon setting new blending keyframes we compute a flag indicating
if the keyframe effect is animating a transform with relative x or y components. Then, when we perform
the first invalidation task, which runs in the next run loop after a change to the timing model has
been made, such as a call to play() on a CSSAnimation made in the TreeResolver::createAnimatedElementUpdate()
where the CSSAnimation was created, we call forceLayout() on this element's FrameView. We also ensure
we commit animations on the compositor immediately after that too, instead of waiting until the next
DisplayRefreshMonitor callback.

* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::performInvalidationTask):
(WebCore::DocumentTimeline::updateAnimations):
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::forceLayoutIfNeeded):
(WebCore::KeyframeEffectReadOnly::setBlendingKeyframes):
(WebCore::KeyframeEffectReadOnly::computedNeedsForcedLayout):
(WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions):
* animation/KeyframeEffectReadOnly.h:

2018-04-16 Pablo Saavedra <psaavedra@igalia.com>

Inconsistent EGL defines in ImageBufferCairo
@@ -176,6 +176,8 @@ void DocumentTimeline::performInvalidationTask()
downcast<DeclarativeAnimation>(*animation).invalidateDOMEvents();
}

applyPendingAcceleratedAnimations();

updateAnimationSchedule();
m_cachedCurrentTime = std::nullopt;
}
@@ -244,8 +246,6 @@ void DocumentTimeline::updateAnimations()
m_document->updateStyleIfNeeded();
}

applyPendingAcceleratedAnimations();

// Time has advanced, the timing model requires invalidation now.
timingModelDidChange();
}
@@ -332,8 +332,16 @@ void DocumentTimeline::animationAcceleratedRunningStateDidChange(WebAnimation& a

void DocumentTimeline::applyPendingAcceleratedAnimations()
{
for (auto& animation : m_acceleratedAnimationsPendingRunningStateChange)
bool hasForcedLayout = false;
for (auto& animation : m_acceleratedAnimationsPendingRunningStateChange) {
if (!hasForcedLayout) {
auto* effect = animation->effect();
if (is<KeyframeEffectReadOnly>(effect))
hasForcedLayout |= downcast<KeyframeEffectReadOnly>(effect)->forceLayoutIfNeeded();
}
animation->applyPendingAcceleratedActions();
}

m_acceleratedAnimationsPendingRunningStateChange.clear();
}

@@ -46,6 +46,7 @@
#include "StylePendingResources.h"
#include "StyleResolver.h"
#include "TimingFunction.h"
#include "TranslateTransformOperation.h"
#include "WillChangeData.h"
#include <wtf/UUID.h>

@@ -693,10 +694,28 @@ void KeyframeEffectReadOnly::updateBlendingKeyframes()
setBlendingKeyframes(keyframeList);
}

bool KeyframeEffectReadOnly::forceLayoutIfNeeded()
{
if (!m_needsForcedLayout || !m_target)
return false;

auto* renderer = m_target->renderer();
if (!renderer || !renderer->parent())
return false;

auto* frameView = m_target->document().view();
if (!frameView)
return false;

frameView->forceLayout();
return true;
}

void KeyframeEffectReadOnly::setBlendingKeyframes(KeyframeList& blendingKeyframes)
{
m_blendingKeyframes = WTFMove(blendingKeyframes);

computedNeedsForcedLayout();
computeStackingContextImpact();
computeShouldRunAccelerated();

@@ -896,6 +915,34 @@ bool KeyframeEffectReadOnly::stylesWouldYieldNewCSSTransitionsBlendingKeyframes(
return !CSSPropertyAnimation::propertiesEqual(property, m_blendingKeyframes[1].style(), &newStyle);
}

void KeyframeEffectReadOnly::computedNeedsForcedLayout()
{
m_needsForcedLayout = false;
if (is<CSSTransition>(animation()) || !m_blendingKeyframes.containsProperty(CSSPropertyTransform))
return;

size_t numberOfKeyframes = m_blendingKeyframes.size();
for (size_t i = 0; i < numberOfKeyframes; i++) {
auto* keyframeStyle = m_blendingKeyframes[i].style();
if (!keyframeStyle) {
ASSERT_NOT_REACHED();
continue;
}
if (keyframeStyle->hasTransform()) {
auto& transformOperations = keyframeStyle->transform();
for (auto operation : transformOperations.operations()) {
if (operation->isTranslateTransformOperationType()) {
auto translation = downcast<TranslateTransformOperation>(operation.get());
if (translation->x().isPercent() || translation->y().isPercent()) {
m_needsForcedLayout = true;
return;
}
}
}
}
}
}

void KeyframeEffectReadOnly::computeStackingContextImpact()
{
m_triggersStackingContext = false;
@@ -1180,6 +1227,11 @@ void KeyframeEffectReadOnly::addPendingAcceleratedAction(AcceleratedAction actio

void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
{
// Once an accelerated animation has been committed, we no longer want to force a layout.
// This should have been performed by a call to forceLayoutIfNeeded() prior to applying
// pending accelerated actions.
m_needsForcedLayout = false;

auto* renderer = this->renderer();
if (!renderer || !renderer->isComposited())
return;
@@ -120,6 +120,7 @@ class KeyframeEffectReadOnly : public AnimationEffectReadOnly
bool computeExtentOfTransformAnimation(LayoutRect&) const;
bool computeTransformedExtentViaTransformList(const FloatRect&, const RenderStyle&, LayoutRect&) const;
bool computeTransformedExtentViaMatrix(const FloatRect&, const RenderStyle&, LayoutRect&) const;
bool forceLayoutIfNeeded();

protected:
void copyPropertiesFromSource(Ref<KeyframeEffectReadOnly>&&);
@@ -136,6 +137,7 @@ class KeyframeEffectReadOnly : public AnimationEffectReadOnly
void setAnimatedPropertiesInStyle(RenderStyle&, double);
TimingFunction* timingFunctionForKeyframeAtIndex(size_t);
Ref<const Animation> backingAnimationForCompositedRenderer() const;
void computedNeedsForcedLayout();
void computeStackingContextImpact();
void updateBlendingKeyframes();
void computeCSSAnimationBlendingKeyframes();
@@ -149,6 +151,7 @@ class KeyframeEffectReadOnly : public AnimationEffectReadOnly
#endif

bool m_shouldRunAccelerated { false };
bool m_needsForcedLayout { false };
bool m_triggersStackingContext { false };
bool m_started { false };
bool m_startedAccelerated { false };

0 comments on commit 1971cea

Please sign in to comment.