Closed Bug 1383998 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

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"
Per hiro's check, timeout of dom/base/test_domwindowutils.html may have the same cause as this bug.
See Also: → 1355758
Assignee: nobody → boris.chiou
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)
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.
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.
Attachment #8892734 - Flags: review?(cam)
Depends on: 1355758, 1378190
See Also: 1355758
Priority: -- → P2
Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

https://reviewboard.mozilla.org/r/163722/#review169576
Attachment #8892733 - Flags: review?(cam) → review+
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 on attachment 8892734 [details]
Bug 1383998 - Enable dom/animation/test/css-transitions/test_element-get-animations.html.

https://reviewboard.mozilla.org/r/163724/#review169582
Attachment #8892734 - Flags: review?(cam) → review+
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.
Pushed by bchiou@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/864d977c8bc4
https://hg.mozilla.org/mozilla-central/rev/6ac13bd22480
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
Flags: needinfo?(boris.chiou)
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.
Flags: needinfo?(boris.chiou)
Attachment #8892733 - Flags: approval-mozilla-beta?
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.
Attachment #8892734 - Flags: approval-mozilla-beta?
Comment on attachment 8892733 [details]
Bug 1383998 - Increase animation generation if attributes are changed.

Fix a regression. Beta56+. Should be in 54 beta 2.
Attachment #8892733 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8892734 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.