1169563 - Add a timeline scrubber to the list of animations to visualize the current time and set it

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Bug 1155663 introduces a new display for the animation-panel, with animation timelines and a time scale header.

In this bug, we should add a vertical scrubber line that moves when animations progress and that can be moved (drag/drop) to set the current time of all the current animations.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 1 - Minor ESLint code cleanup

Bug 1169563 - 1 - Minor ESLint code cleanup

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 2 - Add a scrubber element to timeline animation UI

Bug 1169563 - 2 - Add a scrubber element to timeline animation UI

Comment 3

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

Comment 4

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 4 - Resize animations in the timeline UI according to their playback rates

Bug 1169563 - 4 - Resize animations in the timeline UI according to their playback rates

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2)
> Created attachment 8648038 [details]
> MozReview Request: Bug 1169563 - 2 - Add a scrubber element to timeline
> animation UI
> 
> Bug 1169563 - 2 - Add a scrubber element to timeline animation UI
For info, this part only adds a scrubber that you can move by dragging, but that scrubber doesn't move on its own, nor does dragging it does anything useful.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3)
> Created attachment 8648039 [details]
> MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation
> timeline to set the time
> 
> Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time
This part however makes dragging the scrubber actually set the time of current animations.
The scrubber doesn't move through time when animations progress though, that's for a later patch that I need more work on.

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4)
> Created attachment 8648040 [details]
> MozReview Request: Bug 1169563 - 4 - Resize animations in the timeline UI
> according to their playback rates
> 
> Bug 1169563 - 4 - Resize animations in the timeline UI according to their
> playback rates
And this final part is indirectly related to this bug, but was needed for correctness. Indeed, now that all animations are shown side by side, on the same time scale, we need to make sure their durations/delays take their playbackRate into account.
For instance, if you have 2 animations, both starting at the same time and lasting 2 seconds, and if one of them has a playbackRate of 2, then it'll be twice as fast, and so should be displayed twice shorter than the other animation in the timeline. That's all this patch does.

Comment 8

[Patrick Brosset <:pbro>]

Mike, let me know if you need information about how to test this locally and to know exactly what each patch changes.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4cde80b886d2

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/4cde80b886d2

Comment 11

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

Bug 1169563 - 2 - Add a scrubber element to timeline animation UI

Comment 12

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 116953 - 4 - Move traits out of controller

Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

Comment 13

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 5 - Resize animations in the timeline UI according to their playback rates

Bug 1169563 - 4 - Resize animations in the timeline UI according to their playback rates

Comment 14

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 6 - Animate the scrubber when animations are playing

Bug 1169563 - 5 - Animate the scrubber when animations are playing

Comment 15

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1169563 - 7 - Make the animation inspector UI look better

Bug 1169563 - 6 - Make the animation inspector UI look better

Comment 16

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

https://reviewboard.mozilla.org/r/16801/#review14989

::: browser/devtools/animationinspector/components.js:746
(Diff revision 1)
> +    this.win.addEventListener("mouseup", this.onTimeHeaderMouseUp);

This probably won't work if you're releasing the button when you're not on `this.win`, and therefore you will end up to having the element still dragging, once you go back to the animation panel.
I tried releasing the button once over the web console, and it seems the case.
Maybe one possible solution could be removing the listeners also when the `mouseout` event is fired, for the animation panel.

::: browser/devtools/animationinspector/components.js:759
(Diff revision 1)
> +  onCurrentTimeChanged: function(pageX) {

I would rename the `onCurrentTimeChanged` in `onTimeHeaderPositionChanged`/`Updated` or similar. From a method / listener that is called `onCurrentTimeChanged` I'm expecting the time as argument, not a position in pixel, so in my opinion it could be misleading.

::: browser/devtools/animationinspector/test/browser_animation_timeline_scrubber_movable.js:31
(Diff revision 1)
> +  info("Release the mouse and move again and verify that the scrubber stays");

Probably you could also add a test for the issue of releasing the button on a different windows; but if it wasn't for the web console I would say that is an edge case.

::: browser/themes/shared/devtools/animationinspector.css:153
(Diff revision 1)
> +  right: -5px;

In this way the arrow's point is not really aligned with the vertical line. You want to set probably `width: 1px` and `right: -6px`.

Looks good to me! Just an additional couple of notes:

1. Maybe using `col-resize` instead of `pointer` it would be more explicit as cursor; as user I would also expect to be able to drag the whole line, not just from the arrow.
2. The CSS is not HiDPI complaint, it means that on retina display the vertical line will be 2px width actually. I don't think it's a big issue 'cause most of our devtools aren't HiDPI complaint currently; but I think was worthy to mention.

Comment 17

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

https://reviewboard.mozilla.org/r/16801/#review14989

> This probably won't work if you're releasing the button when you're not on `this.win`, and therefore you will end up to having the element still dragging, once you go back to the animation panel.
> I tried releasing the button once over the web console, and it seems the case.
> Maybe one possible solution could be removing the listeners also when the `mouseout` event is fired, for the animation panel.

Sorry, I meant the markup view, not the web console.
To be more precise, you should be able to reproduce this behavior just dragging the scrubber and then going to the markup view – with still the button pressed –, get an element highlighted, and then release the button over such element. Then if you're going back to the animation panel you're still dragging the scrubber.

Comment 18

[Patrick Brosset <:pbro>]

Thanks for the review Matteo.
I'll take a look at your comments shortly.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #17)
> To be more precise, you should be able to reproduce this behavior just
> dragging the scrubber and then going to the markup view – with still the
> button pressed –, get an element highlighted, and then release the button
> over such element. Then if you're going back to the animation panel you're
> still dragging the scrubber.
Good catch. For some reasons, I remember taking care of this issue because I saw it too, but I clearly not done it. So I'll take a look at that.

As discussed on IRC, part 3 isn't quite ready yet, it will work as expected for animations that start on page load, but the time can't be set for animations that start later. I have a fix for this in a later patch, I will move it to part 3 and upload it again.

Comment 19

[Patrick Brosset <:pbro>]

Try push for part 2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2eb1cec182
Try push for part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30646200cc73

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #16)
> https://reviewboard.mozilla.org/r/16801/#review14989
> 
> ::: browser/devtools/animationinspector/components.js:746
> (Diff revision 1)
> > +    this.win.addEventListener("mouseup", this.onTimeHeaderMouseUp);
> 
> This probably won't work if you're releasing the button when you're not on
> `this.win`, and therefore you will end up to having the element still
> dragging, once you go back to the animation panel.
> I tried releasing the button once over the web console, and it seems the
> case.
> Maybe one possible solution could be removing the listeners also when the
> `mouseout` event is fired, for the animation panel.
Fixed
> ::: browser/devtools/animationinspector/components.js:759
> (Diff revision 1)
> > +  onCurrentTimeChanged: function(pageX) {
> 
> I would rename the `onCurrentTimeChanged` in
> `onTimeHeaderPositionChanged`/`Updated` or similar. From a method / listener
> that is called `onCurrentTimeChanged` I'm expecting the time as argument,
> not a position in pixel, so in my opinion it could be misleading.
You're right, the name isn't great. I've chosen moveScrubberTo(px)
> :::
> browser/devtools/animationinspector/test/
> browser_animation_timeline_scrubber_movable.js:31
> (Diff revision 1)
> > +  info("Release the mouse and move again and verify that the scrubber stays");
> 
> Probably you could also add a test for the issue of releasing the button on
> a different windows; but if it wasn't for the web console I would say that
> is an edge case.
I tried adding a test for this, but while I was doing it, I realized I would have to simulate all events in the right order, including simulating the mouseout of the animation panel window, making the test pretty useless to my opinion. So I've left it out of this patch for now.
> ::: browser/themes/shared/devtools/animationinspector.css:153
> (Diff revision 1)
> > +  right: -5px;
> 
> In this way the arrow's point is not really aligned with the vertical line.
> You want to set probably `width: 1px` and `right: -6px`.
Fixed.
> Looks good to me! Just an additional couple of notes:
> 
> 1. Maybe using `col-resize` instead of `pointer` it would be more explicit
> as cursor; as user I would also expect to be able to drag the whole line,
> not just from the arrow.
Changed to col-resize, and filed bug 1197192 for the part about dragging the whole line.
> 2. The CSS is not HiDPI complaint, it means that on retina display the
> vertical line will be 2px width actually. I don't think it's a big issue
> 'cause most of our devtools aren't HiDPI complaint currently; but I think
> was worthy to mention.
I'm building on Mac right now and will test there. I will either file a new bug or add a patch here.

The previous try push was green and the corrections I've made are minor enough that I'll push this to fx-team in a bit.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ccbc6bf48ad2

Comment 22

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #20)
> > 2. The CSS is not HiDPI complaint, it means that on retina display the
> > vertical line will be 2px width actually. I don't think it's a big issue
> > 'cause most of our devtools aren't HiDPI complaint currently; but I think
> > was worthy to mention.
> I'm building on Mac right now and will test there. I will either file a new
> bug or add a patch here.
Just tested on a retina screen and I'm not seeing a 2px line.

Comment 23

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

Attachment: screenshot-retina.png

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #22)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #20)
> > > 2. The CSS is not HiDPI complaint, it means that on retina display the
> > > vertical line will be 2px width actually. I don't think it's a big issue
> > > 'cause most of our devtools aren't HiDPI complaint currently; but I think
> > > was worthy to mention.
> > I'm building on Mac right now and will test there. I will either file a new
> > bug or add a patch here.
> Just tested on a retina screen and I'm not seeing a 2px line.

What I meant is that it takes double the space. So 1px line in HiDPI will 2px line; that unless we do something explicit in the CSS – or in JS – to handle that, and so far we do nothing in most of our tools.

In the attachment it's possible to see the differences: the first image is a non retina display, the second one is a retina display with 2px line, and the last one is a retina display with 1px line.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8650928 [details]
MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

Comment 26

[Patrick Brosset <:pbro>]

Comment on attachment 8650929 [details]
MozReview Request: Bug 116953 - 4 - Move traits out of controller

Bug 116953 - 4 - Move traits out of controller

Comment 27

[Patrick Brosset <:pbro>]

Comment on attachment 8650930 [details]
MozReview Request: Bug 1169563 - 5 - Resize animations in the timeline UI according to their playback rates

Bug 1169563 - 5 - Resize animations in the timeline UI according to their playback rates

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8650931 [details]
MozReview Request: Bug 1169563 - 6 - Animate the scrubber when animations are playing

Bug 1169563 - 6 - Animate the scrubber when animations are playing

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8650932 [details]
MozReview Request: Bug 1169563 - 7 - Make the animation inspector UI look better

Bug 1169563 - 7 - Make the animation inspector UI look better

Comment 30

[Tom Tromey :tromey]

https://reviewboard.mozilla.org/r/16801/#review15111

Looks good, just one nit.

::: toolkit/devtools/server/tests/browser/browser_animation_actors_11.js:66
(Diff revision 2)
> +  info("Retrieve mutliple animated nodes and their animation players");

Typo in "multiple"

Comment 31

[Tom Tromey :tromey]

Comment on attachment 8650928 [details]
MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time

https://reviewboard.mozilla.org/r/16801/#review15115

Another review to add the r+

Comment 32

[Tom Tromey :tromey]

Comment on attachment 8650929 [details]
MozReview Request: Bug 116953 - 4 - Move traits out of controller

https://reviewboard.mozilla.org/r/16803/#review15127

Looks good.

Comment 33

[Patrick Brosset <:pbro>]

Try push for part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f817eda6793
Thanks Tom for the review.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/babc0182fe4b

Comment 35

[Patrick Brosset <:pbro>]

Try push for part 4: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1663ac7c81e3

Comment 36

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=3bcdcfa7f9be

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/babc0182fe4b
https://hg.mozilla.org/mozilla-central/rev/3bcdcfa7f9be

Part 4 landed with the wrong bug number in the commit message. Please be more careful in the future.

Comment 38

[Patrick Brosset <:pbro>]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37)
> https://hg.mozilla.org/mozilla-central/rev/babc0182fe4b
> https://hg.mozilla.org/mozilla-central/rev/3bcdcfa7f9be
> 
> Part 4 landed with the wrong bug number in the commit message. Please be
> more careful in the future.
Really sorry about that, especially that I had made note to change it before landing ... I ended up changing the message to add the reviewer, but forgot to change the bug number.

Comment 39

[Patrick Brosset <:pbro>]

Attachment: bug1169563-5-resize-with-rate.patch

Rebased, and fixed tear-down exception in test.

Comment 40

[Patrick Brosset <:pbro>]

Attachment: bug1169563-6-animate-scrubber.patch

Comment 41

[Patrick Brosset <:pbro>]

Attachment: bug1169563-7-minor-ui-fixes.patch

Comment 42

[Patrick Brosset <:pbro>]

Mike, I've just flagged you for the last 3 reviews of this bug. The first parts have been reviewed by Matteo and Tom and have landed already.
reviewboard was doing weird stuff when I uploaded a second set of commits last time, so for these 3 new (rebased) patches, I have just uploaded them to bugzilla instead, so you can use splinter to review them.

Comment 43

[Patrick Brosset <:pbro>]

Attachment: bug1169563-6-animate-scrubber.patch

Sorry I just realized I had forgotten to add the test file to source control before committing this change.
The rest of the patch is unchanged.

Comment 44

[Patrick Brosset <:pbro>]

Try push for the last 3 patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41ceb564c1f

Comment 45

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #44)
> Try push for the last 3 patches:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41ceb564c1f
There are a couple of tests failures I didn't see locally. I'll get them fixed and push to try again when it opens up.

Comment 46

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e7ff20e8ee

Comment 47

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/41e0d45d4e26
https://hg.mozilla.org/integration/fx-team/rev/77fc00e04e19
https://hg.mozilla.org/integration/fx-team/rev/453640fcbf71

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41e0d45d4e26
https://hg.mozilla.org/mozilla-central/rev/77fc00e04e19
https://hg.mozilla.org/mozilla-central/rev/453640fcbf71

Comment 49

[Mihai Boldan, Desktop QA [:mboldan]]

I've performed Exploratory Testing around the scrubber implementation on Nightly 44.0a1 (2015-10-21) across platforms [1] and I confirm that:
- the scrubber works as expected if playing/ pausing/ rewinding an animation;
- it can be manually moved;
- it stops at the end of the Animation tab when the animation is finished;
- it's displayed above all other elements from the Animation tab;

Also, I've observed some issues that I would like to be confirmed:
- only on Windows platform, if an animation is finished, the scrubber can be manually moved beyond the end limit (see the 'manually move the scrubber' attached video);
- on Mac and Ubuntu platforms the scrubber is deselected if the mouse cursor is took outside the Nightly browser without releasing the Click button, and on Windows platform it's deselected only if the mouse cursor is took outside the Nightly browser and not moved for about 2 seconds, otherwise the scrubber is selected.
- the scrubber has a delay if it's manually moved.

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

Comment 50

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: manually move the scrubb.gif

Comment 51

[Patrick Brosset <:pbro>]

(In reply to Mihai Boldan, QA [:mboldan] from comment #49)
> Also, I've observed some issues that I would like to be confirmed:
> - only on Windows platform, if an animation is finished, the scrubber can be
> manually moved beyond the end limit (see the 'manually move the scrubber'
> attached video);
I don't think that this is a big problem worth filing a bug for. If you do move the scrubber past the edge, the animation should do what's right: either remain at its finished position if it's finished, or continue showing the right state if its an infinite animation.
> - on Mac and Ubuntu platforms the scrubber is deselected if the mouse cursor
> is took outside the Nightly browser without releasing the Click button, and
> on Windows platform it's deselected only if the mouse cursor is took outside
> the Nightly browser and not moved for about 2 seconds, otherwise the
> scrubber is selected.
There's some logic in the code that tries to do the right thing when the user releases the mouse button outside of the animation tool. In an earlier version, if you started dragging and then moved your mouse to over another panel and released there, then the scrubber would still be in drag mode when your mouse came back.
Maybe we should revisit this code a bit to at least make it consistent across platforms: bug 1218348.

Comment 52

[Mihai Boldan, Desktop QA [:mboldan]]

According to Comment 49 and Comment 51, I'm marking this issue Verified Fixed, since the found issues are tracked separately.