Closed Bug 1169563 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox43 fixed, firefox44 verified)

VERIFIED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed
firefox44 --- verified

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 7 obsolete files)

40 bytes, text/x-review-board-request
jfong
: review+
Details
40 bytes, text/x-review-board-request
tromey
: review+
Details
40 bytes, text/x-review-board-request
tromey
: review+
Details
4.88 KB, image/png
Details
7.14 KB, patch
miker
: review+
Details | Diff | Splinter Review
4.07 KB, patch
miker
: review+
Details | Diff | Splinter Review
9.84 KB, patch
miker
: review+
Details | Diff | Splinter Review
79.68 KB, image/gif
Details
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.
Depends on: 1155663
Blocks: 1153271
Bug 1169563 - 1 - Minor ESLint code cleanup
Attachment #8648037 - Flags: review?(jfong)
Bug 1169563 - 2 - Add a scrubber element to timeline animation UI
Attachment #8648038 - Flags: review?(mratcliffe)
Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time
Attachment #8648039 - Flags: review?(mratcliffe)
Bug 1169563 - 4 - Resize animations in the timeline UI according to their playback rates
Attachment #8648040 - Flags: review?(mratcliffe)
(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.
(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.
(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.
Mike, let me know if you need information about how to test this locally and to know exactly what each patch changes.
Attachment #8648037 - Flags: review?(jfong) → review+
Keywords: leave-open
Bug 1169563 - 2 - Add a scrubber element to timeline animation UI
Attachment #8650928 - Flags: review?(mratcliffe)
Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time
Attachment #8650929 - Flags: review?(mratcliffe)
Bug 1169563 - 4 - Resize animations in the timeline UI according to their playback rates
Attachment #8650930 - Flags: review?(mratcliffe)
Bug 1169563 - 5 - Animate the scrubber when animations are playing
Attachment #8650931 - Flags: review?(mratcliffe)
Bug 1169563 - 6 - Make the animation inspector UI look better
Attachment #8650932 - Flags: review?(mratcliffe)
Attachment #8648038 - Attachment is obsolete: true
Attachment #8648038 - Flags: review?(mratcliffe)
Attachment #8648039 - Attachment is obsolete: true
Attachment #8648039 - Flags: review?(mratcliffe)
Attachment #8648040 - Attachment is obsolete: true
Attachment #8648040 - Flags: review?(mratcliffe)
Blocks: 1180134
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.
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.
Attachment #8650928 - Flags: review?(mratcliffe) → review+
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.
Assignee: nobody → pbrosset
Blocks: 1197192
(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.
(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.
Attached image 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 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
Attachment #8650928 - Attachment description: MozReview Request: Bug 1169563 - 2 - Add a scrubber element to timeline animation UI → MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time
Attachment #8650928 - Flags: review+ → review?(mratcliffe)
Comment on attachment 8650929 [details]
MozReview Request: Bug 116953 - 4 - Move traits out of controller

Bug 116953 - 4 - Move traits out of controller
Attachment #8650929 - Attachment description: MozReview Request: Bug 1169563 - 3 - Move the scrubber in the animation timeline to set the time → MozReview Request: Bug 116953 - 4 - Move traits out of controller
Attachment #8650930 - Attachment description: MozReview Request: Bug 1169563 - 4 - Resize animations in the timeline UI according to their playback rates → MozReview Request: Bug 1169563 - 5 - Resize animations in the timeline UI according to their playback rates
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 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
Attachment #8650931 - Attachment description: MozReview Request: Bug 1169563 - 5 - Animate the scrubber when animations are playing → MozReview Request: Bug 1169563 - 6 - Animate the scrubber when animations are playing
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
Attachment #8650932 - Attachment description: MozReview Request: Bug 1169563 - 6 - Make the animation inspector UI look better → MozReview Request: Bug 1169563 - 7 - Make the animation inspector UI look better
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 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+
Attachment #8650928 - Flags: review+
Comment on attachment 8650929 [details]
MozReview Request: Bug 116953 - 4 - Move traits out of controller

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

Looks good.
Attachment #8650929 - Flags: review+
Attachment #8650928 - Flags: review?(mratcliffe)
Attachment #8650929 - Flags: review?(mratcliffe)
Try push for part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f817eda6793
Thanks Tom for the review.
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.
(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.
Rebased, and fixed tear-down exception in test.
Attachment #8650930 - Attachment is obsolete: true
Attachment #8650931 - Attachment is obsolete: true
Attachment #8650932 - Attachment is obsolete: true
Attachment #8650930 - Flags: review?(mratcliffe)
Attachment #8650931 - Flags: review?(mratcliffe)
Attachment #8650932 - Flags: review?(mratcliffe)
Blocks: 1170159
Attachment #8652759 - Flags: review?(mratcliffe)
Attachment #8652760 - Flags: review?(mratcliffe)
Attachment #8652751 - Flags: review?(mratcliffe)
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.
Flags: needinfo?(mratcliffe)
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.
Attachment #8652759 - Attachment is obsolete: true
Attachment #8652759 - Flags: review?(mratcliffe)
Attachment #8652824 - Flags: review?(mratcliffe)
Attachment #8652751 - Flags: review?(mratcliffe) → review+
Attachment #8652760 - Flags: review?(mratcliffe) → review+
Attachment #8652824 - Flags: review?(mratcliffe) → review+
Try push for the last 3 patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41ceb564c1f
Flags: needinfo?(mratcliffe)
(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.
Blocks: 1199589
Blocks: 1211886
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
(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.
Flags: needinfo?(pbrosset)
According to Comment 49 and Comment 51, I'm marking this issue Verified Fixed, since the found issues are tracked separately.
Status: RESOLVED → VERIFIED
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.