1199589 - Display current timeline time on the scrubber

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Somewhere next to the scrubber (added in bug 1169563), we should display the exact current time of the document timeline.
This would help because the time graduations shown in the background are often not enough to know what's the current time.

Comment 1

[Patrick Brosset <:pbro>]

I think a better place for this will be in the timeline toolbar, next to the rewind and pause/play buttons.
I'm marking this as blocked by bug 1205681 because for that bug, I have patches that revamp a bit a way the toolbar looks, and I need this to display the time in there.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1199589_-_Display_the_current_timeline_time_in.diff

WIP

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1199589_-_Display_the_current_timeline_time_in.diff

Comment 4

[Patrick Brosset <:pbro>]

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

Comment 5

[Patrick Brosset <:pbro>]

Hm, I think I should move the time formatting function to utils.js in the same dir and create a unit test for it. I did not think of this, but after re-reviewing my patch, it seems that the browser test I added does not exercise this enough. In any case, I'll do this as a separate patch.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug_1199589_-_Unit_test_for_time_formatting.diff

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug_1199589_-_Unit_test_for_time_formatting.diff

I forgot to update another test.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8672575 [details] [diff] [review]
Bug_1199589_-_Unit_test_for_time_formatting.diff

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

Happy to see this move into a util / unit test

::: devtools/client/animationinspector/animation-panel.js
@@ +200,5 @@
>    displayTimelineCurrentTime: function() {
>      let {isMoving, isPaused, time} = this.timelineData;
>  
>      if (isMoving || isPaused) {
> +      this.timelineCurrentTimeEl.innerHTML = formatStopwatchTime(time);

Unrelated really, but can we use textContent instead?

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8671875 [details] [diff] [review]
Bug_1199589_-_Display_the_current_timeline_time_in.diff

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

I believe the two patches should be folded before landing

Comment 10

[Patrick Brosset <:pbro>]

Thanks Brian. Changed innerHTML to textContent and folded the 2 patches.

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[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

Also see bug 1205681, comment 15 for more on why I've chosen to present it like this.

Comment 14

[Patrick Brosset <:pbro>]

LGTM, thanks Will.