1227477 - Polish the way the timeline time graduations are calculated

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: keyframes.html

The graduations displayed in the timeline header of the animation-inspector could be better.

STR:
- open the attached test case
- open the animation inspector while the animation is playing
- try resizing the sidebar and then refreshing the page (to restart the animation)

Expected (with a sidebar at around 500px):
0s 1s 2s 3s 4s 5s 6s 7s 8s 9s 10s

Actual:
0s 1.6s 3.2s 4.8s 6.4s 8s 9.6s

Comment 1

[Patrick Brosset <:pbro>]

As discussed. Let me know if you have questions.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1227477.patch

Hi there. 
I made a few changes to the codebase in order to address this problem. 
I did not launch the tests nor edited them as I wanted to know if I'm heading into the right direction. 
The main idea is to compute an optimal number of intervals rather than directly an interval length, in order to ensure that it shows the max time ( and that the ticks are even, or sort of ).
I did some minor change to the CSS to get some space and display the max time ( the last tick ) in its full length. 
I also changed the function that draw the background ticks but as I'm not very familiar with canvas, and even less with bitwise operators, I don't know if I'm doing a good job here.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8708568 [details] [diff] [review]
Bug_1227477.patch

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

Thanks Nicolas for working on this.
The intervals look better than today for sure.
I think we could do something even better though.
Here are some cases I noticed with your patch applied:

- making the timeline really small results in just one NaN interval,
- on jsbin.com (there's an animation when you click on the logo, top left), making the timeline quite big (~700px), I got: 0, 24, 48, 72, 96, etc. It's not too bad, but it would make even more sense to users if this was 0, 25, 50, 75, 100, etc. Easier to relate to. I know that the animation on that page lasts for 240ms, so cutting it in 25ms intervals would mean that the last interval be smaller. And I think that's what we can improve with your solution: accepting the last interval to be cut off.
- on http://tympanus.net/Development/DialogEffects/ricky.html (click the button to the see the animation), and making the timeline sidebar be ~625px, I got: 0, 138, 275, 412, etc. Again, I have a feeling it would be easier for users if we showed 0, 100, 200, 300, ... or (if that doesn't fit), 0, 250, 500, 750, ...

In short, I think we should stick to showing intervals multiple of 1, 2.5, 5 only. Because these are more logical amounts of time, they're easier to reason about when looking at the timeline.
So, for instance, if you had a total length of 283ms, then (depending on how much width there is), you'd cut it in 0, 50, 100, 150, 200, 250, 300 (which means that the last interval would be somewhat smaller than the other, partly hidden in fact).

I made this to illustrate what I mean: http://jsbin.com/ciyemizasu/2/edit?js,output

::: devtools/client/themes/animationinspector.css
@@ +20,5 @@
>    --timeline-sidebar-width: 200px;
>    /* How high should animations displayed in the timeline be */
>    --timeline-animation-height: 20px;
>    /* The size of a keyframe marker in the keyframes diagram */
> +  --timeline-animation-offset: 40px;

Not sure how I feel about this. On one hand it's nice to be able to see the last interval's label on the right, on the other hand that means we're using more horizontal space now, and because the animation inspector is in a sidebar panel for now, it's usually going to be very narrow. So 40px to the right just to display a time label sounds like a waster of space.
Especially because the left-hand panel (that contains the list of DOM nodes) is already pretty wide.
If you try and make the sidebar width be ~300px on, say, http://tympanus.net/Development/DialogEffects/ricky.html (which I assume would be a usual width for users), then the animations are barely visible.

I don't think loosing this information (the last time interval) is a big deal either, knowing that the current time is always displayed in the toolbar.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1227477.patch

Hi Patrick,

Thank you for all your comments.
I edited my patch with your suggestions ( the jsbin was very helpful ), and I think I got something quite good (at least, it looks good with all the examples you gave in Comment 3).
There are obviously some tests that needs to be rewritten ( devtools/client/animationinspector/test/unit/test_findOptimalTimeInterval.js and devtools/client/animationinspector/test/browser_animation_timeline_header.js ) but I wanted to make sure that you are happy with those changes before doing it.

> I don't think loosing this information (the last time interval) is a big deal either, knowing that the current time is always displayed in the toolbar.

I put a title attribute on the time intervals, so when it is cut, the user can see the whole text hovering the label. What do you think about this ?

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8711902 [details] [diff] [review]
Bug_1227477.patch

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

This looks great. Thanks Nicolas!

::: devtools/client/animationinspector/components/animation-timeline.js
@@ +411,5 @@
>    drawHeaderAndBackground: function() {
>      let width = this.timeHeaderEl.offsetWidth;
> +    let animationDuration = (TimeScale.maxEndTime - TimeScale.minStartTime);
> +    let minTimeInterval = (
> +      TIME_GRADUATION_MIN_SPACING * animationDuration / width);

nit: no need for parenthesis around the values of the last 2 variables.

@@ +431,5 @@
>          nodeType: "span",
>          attributes: {
>            "class": "time-tick",
> +          "style": `left:${pos}%`,
> +          "title": tickLabel

I like the idea, but I'm not sure we should do it this way. Indeed, tick labels have a certain width, so you could in theory, have your mouse positioned at 205ms from the start, in the timeline, but since the label is right below your mouse, then you'd see, say, the "200ms" label.
I think if we wanted to do something like this, we should have a mouseover listener to shows a label continuously when you move your mouse over the timeline graduations, with a small vertical line that indicates exactly your position.
Or something like that anyway.
So I'd suggest removing this for now.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1227477.patch

Hi there,

I removed the title attribute and edited the two tests that needed it because of the changes in the findOptimalTimeInterval function.

I ran the whole test suite and everything is fine.
I hope it looks good to you.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8712788 [details] [diff] [review]
Bug_1227477.patch

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

This looks great!
I've pushed to TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b831139d7b21

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

I'm glad it is good.

> I've pushed to TRY:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b831139d7b21

Oh, I was looking forward to do it as I now have access to TRY :) . Maybe on the next bug thus.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #8)
> I'm glad it is good.
> 
> > I've pushed to TRY:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b831139d7b21
> 
> Oh, I was looking forward to do it as I now have access to TRY :) . Maybe on
> the next bug thus.
Oh sorry about that, I forgot you did. I'm reviewing bug 1219611 now, I'll let you push to try for that one :)

Comment 10

[Patrick Brosset <:pbro>]

Try push went really fast today! And it's green, so I'll land this now. Thanks a lot Nicolas!

Comment 11

[Pulsebot]

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

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/0ae6c7e277af

Comment 13

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 45.0a1(Build Id:20151124030553)
on windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 47.0b3(Build Id:20160505125249)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

[testday-20160506]