Closed Bug 1205681 Opened 5 years ago Closed 4 years ago

Add a rewind button to the timeline toolbar

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox44 verified)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 2 obsolete files)

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.
Blocks: 1199589
Part 1 - WIP
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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.
Blocks: 1211801
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.
Attachment #8662435 - Attachment is obsolete: true
Attachment #8671856 - Flags: review?(zer0)
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.
Attachment #8662436 - Attachment is obsolete: true
Attachment #8671861 - Flags: review?(ttromey)
Part 3 - And this is a new test for this new feature.
Attachment #8671862 - Flags: review?(ttromey)
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.
Attachment #8671861 - Flags: review?(ttromey) → review+
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.
Attachment #8671862 - Flags: review?(ttromey) → review+
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 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!
Attachment #8671856 - Flags: review?(zer0) → review+
Thanks Matteo.
New try build after addressing Tom's feedback: https://treeherder.mozilla.org/#/jobs?repo=try&revision=071fdfbd87dd
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
Status: RESOLVED → VERIFIED
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.
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
(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.
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.