1120852 - The animations panel doesn't work when there is an animation-delay (timeline is de-synchronized)

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

STR:

- open https://bug1117603.bugzilla.mozilla.org/attachment.cgi?id=8543711
- open the inspector
- switch to the animations sidebar panel
- select the animated node

you might have to reload the page once you've selected the node as the animation may have already ended by then (the node will remain selected when you reload the page).

ER: the timeline progress is synchronized with the animation itself
AR: the timeline starts right away, not waiting for the 2s delay, and therefore finishes before the animation actually finishes.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1120852-1-expose-delay-on-actor.patch

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1120852-2-dont-start-animation-timeline-before-delay.patch

Comment 3

[Patrick Brosset <:pbro>]

Try build for both patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4230d999543e

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8548229 [details] [diff] [review]
bug1120852-1-expose-delay-on-actor.patch

Review of attachment 8548229 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/animation.js
@@ +126,5 @@
> +    } else {
> +      return 0;
> +    }
> +
> +    if (delayText.indexOf(",") !== -1) {

We need test coverage for this case (multiple animation / transition delays separated by commas).

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1120852-1-expose-delay-on-actor v2.patch

Thanks Brian for the review. This new version includes a new test file to ensure multiple delays, duration, counts were retrieved correctly by the actor.
New try build for this 1st part: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4a7e0b1ea6e

Comment 6

[Patrick Brosset <:pbro>]

part 1 in fx-team: https://hg.mozilla.org/integration/fx-team/rev/5ff5e6e6623b

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8548230 [details] [diff] [review]
bug1120852-2-dont-start-animation-timeline-before-delay.patch

Review of attachment 8548230 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/animationinspector/test/browser_animation_time_formatting.js
@@ +6,5 @@
> +
> +// Test the time formatting helper function.
> +
> +add_task(function*() {
> +  yield addTab(TEST_URL_ROOT + "doc_simple_animation.html");

This getFormattedTime function seems like something that could be decoupled from the UI and tested with a unit test instead of a mochitest (possibly moved into viewhelpers or exported separately in one of the animationinspector files rather than attached to the widget instance).  Usually we just end up doing it in a mochitest like this so I wouldn't stop it from landing because of this, but it would speed up our test harness and allow cross-tool reuse if things like this were moved.

@@ +13,5 @@
> +  let widget = panel.playerWidgets[0];
> +
> +  is(widget.getFormattedTime(0), "0.00");
> +  is(widget.getFormattedTime(1000), "1.00");
> +  is(widget.getFormattedTime(5.123765827635872635), "0.00");

Nit: Add test cases with a number (plus decimal) in the 10s and 100s

Comment 8

[Patrick Brosset <:pbro>]

You're right, I should have moved this to a unit test. Maybe what I'll do is do this in a separate patch so that I can land part 2 quickly (since try is green), and then land this new unit test separately.
In any case, thanks for the reviews.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ff5e6e6623b

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> This getFormattedTime function seems like something that could be decoupled
> from the UI and tested with a unit test instead of a mochitest (possibly
> moved into viewhelpers or exported separately in one of the
> animationinspector files rather than attached to the widget instance). 
> Usually we just end up doing it in a mochitest like this so I wouldn't stop
> it from landing because of this, but it would speed up our test harness and
> allow cross-tool reuse if things like this were moved.
In fact, I just came across toLocaleString (which now seems to be fully implemented, that didn't seem to be the case some time ago), and using it simplifies massively the time formatting function:

  getFormattedTime: function(time) {
    return (time/1000).toLocaleString(undefined, {
      minimumFractionDigits: 2,
      maximumFractionDigits: 2
    });
  },

In fact, now that only really use this native function, there's no real need to test it alone with a unit test.
What I'm going to do though is file a separate bug to refactor ViewHelpers.L10N.numberWithDecimals which has its own implementation for now.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug1120852-2-dont-start-animation-timeline-before-delay v2.patch

Part 2 in fx-team: https://hg.mozilla.org/integration/fx-team/rev/d0b17fe68e83

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0b17fe68e83