Closed Bug 1199589 Opened 5 years ago Closed 4 years ago

Display current timeline time on the scrubber

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Keywords: dev-doc-complete)

Attachments

(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
WIP
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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]
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?
Attachment #8672575 - Flags: review?(bgrinstead) → review+
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
Attachment #8671875 - Flags: review?(bgrinstead) → review+
Thanks Brian. Changed innerHTML to textContent and folded the 2 patches.
Blocks: 985861
No longer blocks: 1153271
https://hg.mozilla.org/mozilla-central/rev/ec2bb2866393
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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.
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.