Closed Bug 1199589 Opened 5 years ago Closed 4 years ago

Display current timeline time on the scrubber


(DevTools :: Inspector: Animations, defect)

Not set


(firefox44 fixed)

Firefox 44
Tracking Status
firefox44 --- fixed


(Reporter: pbro, Assigned: pbro)



(Keywords: dev-doc-complete)


(2 files, 2 obsolete files)

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.
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.
Depends on: 1205681
Assignee: nobody → pbrosset
Attachment #8662437 - Attachment is obsolete: true
Attachment #8671875 - Flags: review?(bgrinstead)
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.
Attachment #8672573 - Flags: review?(bgrinstead)
I forgot to update another test.
Attachment #8672573 - Attachment is obsolete: true
Attachment #8672573 - Flags: review?(bgrinstead)
Attachment #8672575 - Flags: review?(bgrinstead)
Comment on attachment 8672575 [details] [diff] [review]

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?
Attachment #8672575 - Flags: review?(bgrinstead) → review+
Comment on attachment 8671875 [details] [diff] [review]

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

I believe the two patches should be folded before landing
Attachment #8671875 - Flags: review?(bgrinstead) → review+
Thanks Brian. Changed innerHTML to textContent and folded the 2 patches.
Blocks: 985861
No longer blocks: 1153271
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
I've added a section on Firefox 44:

Also see bug 1205681, comment 15 for more on why I've chosen to present it like this.
Flags: needinfo?(pbrosset)
LGTM, thanks Will.
Flags: needinfo?(pbrosset)
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.