1205681 - Add a rewind button to the timeline toolbar

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Now that we have a timeline toolbar (bug 1155661), we should add a rewind button to it. It's useful when you start to play around with the pause button and the scrubber, to quickly move the animations back to their initial positions.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1205681_-_1_-_Add_a_timeline_rewind_button_to_.diff

Part 1 - WIP

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1205681_-_2_-_Implement_the_rewind_timeline_bu.diff

Part 2 - WIP

The missing parts are:
- tests
- the scrubber doesn't go back to 0 and doesn't stop moving when rewind is pressed.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1205681_-_1_-_Add_a_timeline_rewind_button_to_.diff

Rebased, fixed the missing button icon image, and also removed a lot of CSS code that I had forgotten to clean up when moving the UI from v2 to v3.

Note that this patch is really small, it only adds the button, not the logic behind the button. This comes in part 2.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1205681_-_2_-_Implement_the_rewind_timeline_bu.diff

Part 2 - The logic for the button!

In this patch, I introduced a small DOM querySelector helper ($) to reduce the amount of code.
I also changed a little bit how the animations' states and the UI are refreshed after an action like pause, play or rewind.
Finally, I had to make some changes to the logic used to display/move the scrubber element. Indeed, so far we could use the document's currentTime to position it, but that's not possible anymore with the rewind feature. So the timeline component needs to detect if animations have been rewound, and if yes position the scrubber at the beginning.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug_1205681_-_3_-_Tests_for_the_timeline_rewind_bu.diff

Part 3 - And this is a new test for this new feature.

Comment 6

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85606c14799c

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8671861 [details] [diff] [review]
Bug_1205681_-_2_-_Implement_the_rewind_timeline_bu.diff

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

Looks good.

::: devtools/client/animationinspector/animation-panel.js
@@ +9,5 @@
>  "use strict";
>  
>  const {AnimationsTimeline} = require("devtools/client/animationinspector/components");
>  
> +let $ = (selector, target = document) => target.querySelector(selector);

I think we're supposed to use "var" at top-level now.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8671862 [details] [diff] [review]
Bug_1205681_-_3_-_Tests_for_the_timeline_rewind_bu.diff

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

Looks good, thanks.

::: devtools/client/animationinspector/test/head.js
@@ +470,5 @@
> +    // for some time. A relatively long timeout is used because the test page
> +    // has long running animations, so the scrubber doesn't move that quickly.
> +    let startOffset = scrubberEl.offsetLeft;
> +    yield new Promise(r => setTimeout(r, 2000));
> +    let endOffset = scrubberEl.offsetLeft;

If the scrubber can come back to the same position during the course of an animation, then this is susceptible to (unlikely) random failures if the timeout happens to end at just the right moment.  It would be more robust to add a one-time listener for timeline-data-changed and arrange for the test to fail if it is fired.

I think it's fine if you'd rather not do this now.  I normally wouldn't even mention it, since this is pre-existing code, but it occurred to me that moving it to head.js means that it will be used in more scenarios than previously.

Comment 9

[Patrick Brosset <:pbro>]

Thanks Tom for the reviews. I have updated parts 2 and 3 according to your feedback.
Last try was green, just waiting for a review on part 1 and I will then land this.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8671856 [details] [diff] [review]
Bug_1205681_-_1_-_Add_a_timeline_rewind_button_to_.diff

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

Looks good to me!

Comment 11

[Patrick Brosset <:pbro>]

Thanks Matteo.
New try build after addressing Tom's feedback: https://treeherder.mozilla.org/#/jobs?repo=try&revision=071fdfbd87dd

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6d016e7c8780
https://hg.mozilla.org/integration/fx-team/rev/84e437888ab4
https://hg.mozilla.org/integration/fx-team/rev/d42f330611b5

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d016e7c8780
https://hg.mozilla.org/mozilla-central/rev/84e437888ab4
https://hg.mozilla.org/mozilla-central/rev/d42f330611b5

Comment 14

[Mihai Boldan, Desktop QA [:mboldan]]

I've performed Exploratory Testing around this feature on Nightly 44.0a1 (2015-10-20) across platforms [1] and managed to confirm the following:
- the Rewind button is displayed only if an animation is available in the sidebar;
- no matter in witch stage the animation is (playing, paused, finished or rewinded), if clicking on the Rewind button will move the animations back to their original state and also will reset the counter and the scrubber. 

[1]Windows 10 x86, Ubuntu 13.10 x64 and Mac OS X 10.10.4

Comment 15

[Will Bamberg [:wbamberg]]

I've added a section on Firefox 44: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Work_with_animations#Firefox_44

I wasn't sure how to handle versions here: what I've chosen is to list extra Firefox 44 features as increments to the more fundamental changes introduced in Firefox 43. The difficulty with that is that you need to read the Firefox 43 section as well, to get the full effect.

Alternatives:
- omit any discussion of Firefox 43, but that might confuse people who are not on Developer Edition
- duplicate the Firefox 43 stuff under the Firefox 44 heading. But I think this would read very weirdly.

Hopefully, eventually this will settle down, and when Firefox 44 is in the release channel I'll be able to retire the old stuff.

I wish we had a way to do versioning in MDN pages.

Comment 16

[Patrick Brosset <:pbro>]

Yeah, it's unfortunately hard to document, sorry about that, but I guess that's expected for new tools. They tend to change quickly at first, and then, hopefully, settle down.
Thanks for baring with me and documenting so much of what has changed, this is awesome.

How about this though: we move the pre-43 docs to another place, flagged as obsolete or something, with a reminder to delete it some time from now.
And instead, on the "work with animations" page, we only document all the features of the timeline UI.
We don't show 1 section for 43 and another one for 44, we just list all features together but flag them with geckoVersionNote thing "New in Firefox 44".

I'm happy either way, as you said, we can revisit this in a little while when 44 is released.

Comment 17

[Will Bamberg [:wbamberg]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #16)
> Yeah, it's unfortunately hard to document, sorry about that, but I guess
> that's expected for new tools. They tend to change quickly at first, and
> then, hopefully, settle down.
> Thanks for baring with me and documenting so much of what has changed, this
> is awesome.
> 
> How about this though: we move the pre-43 docs to another place, flagged as
> obsolete or something, with a reminder to delete it some time from now.
> And instead, on the "work with animations" page, we only document all the
> features of the timeline UI.
> We don't show 1 section for 43 and another one for 44, we just list all
> features together but flag them with geckoVersionNote thing "New in Firefox
> 44".
> 

This sounds like a good idea, and I've now done that, more or less.

I'll mark this and the related bugs dev-doc-complete, but of course please let me know if you spot anything else that needs work.