1383998 - stylo: dom/animation/test/css-transitions/test_element-get-animations.html fails

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Brian Birtles (:birtles)]

According to bug 1383980 comment 4 and https://treeherder.mozilla.org/#/jobs?repo=try&revision=860fc37406abaf5b93afb24d1a43c25632ade89a&selectedJob=117288465 this now fails with:
 
  TEST-UNEXPECTED-FAIL | dom/animation/test/css-transitions/test_element-get-animations.html | getAnimations sorts transitions by when they were generated - getAnimations sorts transitions by when they were generated: assert_equals: expected "transform" but got "opacity"

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Per hiro's check, timeout of dom/base/test_domwindowutils.html may have the same cause as this bug.

Comment 2

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #0)
> According to bug 1383980 comment 4 and
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=860fc37406abaf5b93afb24d1a43c25632ade89a&selectedJob=1
> 17288465 this now fails with:
>  
>   TEST-UNEXPECTED-FAIL |
> dom/animation/test/css-transitions/test_element-get-animations.html |
> getAnimations sorts transitions by when they were generated - getAnimations
> sorts transitions by when they were generated: assert_equals: expected
> "transform" but got "opacity"

OK, so this is caused by bug 1383980? I thought bug 1355758 already fixed this before. Looks like we should check the animation generation again because I remember we use that as the order.

Comment 3

[Boris Chiou [:boris]]

My log of this test case. OK, the animation generation is not added after creating the first transition.

GECKO(44343) | create transition: [transform] (translate3d(0px, 0px, 0px) -> translate3d(100px, 0px, 0px)), (order: 0)
GECKO(44343) | create transition: [opacity] (0 -> 1), (order: 0)

Comment 4

[Boris Chiou [:boris]]

Looks like we don't call PostRestyleEvent() after changing the style, so mHaveNonAnimationRestyles is always false. Therefore, we don't update mAnimationGeneration in this case.

Comment 5

[Boris Chiou [:boris]]

OK. I think this is a regression of Bug 1378190. In Bug 1378190, we change the function call stack for AttributeChanged(), and therefore we don't make mHaveNonAnimationRestyles to be true any more if we change attribute.

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1383998 - Increase animation generation if attributes are changed.

We have to set mHaveNonAnimationRestyles if we have attributes changed
(note: we increase the animation generation only if mHaveNonAnimationRestyles
is set). Attributes changed may create a new transition, and we use the
animation generation as the order of the transition, so
Element::GetAnimations() can return transitions with correct order.

Besides, I think ContentStateChanged() will not trigger a new
transition, so we don't need to make mHaveNonAnimationRestyles there.

Review commit: https://reviewboard.mozilla.org/r/163722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163722/

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1383998 - Enable dom/animation/test/css-transitions/test_element-get-animations.html.

Review commit: https://reviewboard.mozilla.org/r/163724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163724/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

https://reviewboard.mozilla.org/r/163722/#review169576

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

https://reviewboard.mozilla.org/r/163722/#review169580

::: layout/base/ServoRestyleManager.cpp:1232
(Diff revision 1)
> +    // If we change attributes, we have to mark this to be true, so we will
> +    // increase the animation generation for the new created transition if any.
> +    mHaveNonAnimationRestyles = true;

Actually, why don't we need to do this in TakeSnapshotForAttributeChange too, where we also call IncrementUndisplayedRestyledGeneration() for an attribute change?  The change here in AttributeChanged will only take care of attributes that have special restyle hint generation behavior (such as mapped attributes).  So if some other random attribute changes, and we had some transition that started based of that attribute change, e.g.

  p { color: blue; transition: color 1s; }
  p[data-something] { color: red; }

where the data-something="" attribute is changing, or even

  p { color: blue; transition: color 1s; }
  p.x { color: red; }

where the class="" attribute is changing, do we need to increment the animation generation too?

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8892734 [details]
Bug 1383998 - Enable dom/animation/test/css-transitions/test_element-get-animations.html.

https://reviewboard.mozilla.org/r/163724/#review169582

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

https://reviewboard.mozilla.org/r/163722/#review169580

> Actually, why don't we need to do this in TakeSnapshotForAttributeChange too, where we also call IncrementUndisplayedRestyledGeneration() for an attribute change?  The change here in AttributeChanged will only take care of attributes that have special restyle hint generation behavior (such as mapped attributes).  So if some other random attribute changes, and we had some transition that started based of that attribute change, e.g.
> 
>   p { color: blue; transition: color 1s; }
>   p[data-something] { color: red; }
> 
> where the data-something="" attribute is changing, or even
> 
>   p { color: blue; transition: color 1s; }
>   p.x { color: red; }
> 
> where the class="" attribute is changing, do we need to increment the animation generation too?

You are right. We also need do this in TakeSnapshotForAttributeChange too. I will add this line in TakeSnapshotForAttributeChange too. Thanks for the review.

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163722/diff/1-2/

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8892734 [details]
Bug 1383998 - Enable dom/animation/test/css-transitions/test_element-get-animations.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163724/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/864d977c8bc4
Increase animation generation if attributes are changed. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6ac13bd22480
Enable dom/animation/test/css-transitions/test_element-get-animations.html. r=heycam

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/864d977c8bc4
https://hg.mozilla.org/mozilla-central/rev/6ac13bd22480

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

We need this because the first Beta 56 builds will be released next week and we will start our Stylo experiment for some Beta 56 users

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1378190
[User impact if declined]: This makes Element.getAnimations() return incorrect order of animations and a test is failed.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 2 in this bug. (Attachment 8892734 [details])
[Is the change risky?]: No.
[Why is the change risky/not risky?]: No. This set only one more flag to make sure we get the correct order of a specific web animation API for stylo.
[String changes made/needed]: No.

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8892734 [details]
Bug 1383998 - Enable dom/animation/test/css-transitions/test_element-get-animations.html.

Some reason as the part 1 patch.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

Fix a regression. Beta56+. Should be in 54 beta 2.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5c06aca00aae
https://hg.mozilla.org/releases/mozilla-beta/rev/9bc271101512

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Boris Chiou [:boris] from comment #17)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Boris's assessment on manual testing needs and the fact that this fix has automated coverage.