Bug 172300 – Transform misplaces element 50% of the time

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Transform misplaces element 50% of the time

[Dean Jackson]

Created attachment 310529 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 310529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review

> Source/WebCore/ChangeLog:11
> +        animation. The "50% of the time" comes in to play because

I wonder if this fixes any existing flaky tests!

> LayoutTests/animations/needs-layout.html:50
> +    setTimeout(test, 100);

Why is the 100 ms timeout needed? Such short timeouts tend to make flaky tests.

[Simon Fraser (smfr)]

Comment on attachment 310529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review

> Source/WebCore/page/animation/CSSAnimationController.cpp:662
> +            frameView.setNeedsLayout();

Why isn't this renderer.setNeedsLayout()?

> Source/WebCore/page/animation/CSSAnimationController.cpp:663
> +            frameView.scheduleRelayout();

Is this needed as well as setNeedsLayout()?

> Source/WebCore/page/animation/KeyframeAnimation.cpp:84
> +    if (!m_keyframes.containsProperty(CSSPropertyTransform))

Is this a linear walk just like your loop below?

> Source/WebCore/page/animation/KeyframeAnimation.cpp:99
> +                    if (translation->x().isPercent() || translation->y().isPercent() || translation->z().isPercent()) {

translation->z() cannot be a percentage.

[Dean Jackson]

Comment on attachment 310529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review

>> Source/WebCore/page/animation/CSSAnimationController.cpp:662
>> +            frameView.setNeedsLayout();
> 
> Why isn't this renderer.setNeedsLayout()?

Fixed.

>> Source/WebCore/page/animation/KeyframeAnimation.cpp:84
>> +    if (!m_keyframes.containsProperty(CSSPropertyTransform))
> 
> Is this a linear walk just like your loop below?

Nope. This is a hash set lookup.

>> Source/WebCore/page/animation/KeyframeAnimation.cpp:99
>> +                    if (translation->x().isPercent() || translation->y().isPercent() || translation->z().isPercent()) {
> 
> translation->z() cannot be a percentage.

Ah, I forgot. It's still a length, but not percentage.

>> LayoutTests/animations/needs-layout.html:50
>> +    setTimeout(test, 100);
> 
> Why is the 100 ms timeout needed? Such short timeouts tend to make flaky tests.

It's not. I've reduced it to nothing. I just need a timeout.

[Dean Jackson]

Comment on attachment 310529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310529&action=review

>>> Source/WebCore/page/animation/CSSAnimationController.cpp:662
>>> +            frameView.setNeedsLayout();
>> 
>> Why isn't this renderer.setNeedsLayout()?
> 
> Fixed.

Actually, it has to be the frameView, otherwise the scheduleLayout() on the frameView returns early without actually scheduling a layout.

[Simon Fraser (smfr)]

Comment on attachment 310529 [details]
Patch

smfr_: it’s just about the order of the 2 timers
smfr_: i think there’s another fix
smfr_: if the animation timer fires and there’s a layout pending, do a forced layout

[Dean Jackson]

Created attachment 310569 [details]
Patch

[Dean Jackson]

Committed r217075: <http://trac.webkit.org/changeset/217075>