1319989 - Don't show direction in tooltip when it is 'normal'

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect, P3)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: timing-tooltip.png

In the screenshot, 'direction: normal' is shown despite this being the default. I believe that in the interests of reducing clutter, we normally don't show fields which have a default value (e.g. delay: 0s is not shown)?

(Might be a good first bug if someone is available to mentor it)

Comment 1

[Patrick Brosset <:pbro>]

Thank you Brian for filing.

If someone wants to try working on this good-first bug, I can mentor them.
It's a good opportunity to learn more about the animation tool in DevTools, and the change should be rather simple.

First of all, make sure you know how to get the code and build Firefox: https://wiki.mozilla.org/DevTools/Hacking
Here is also some information to get in touch if needed: https://wiki.mozilla.org/DevTools/GetInvolved#Working_on_the_code

Now, in terms of code, the following file needs to be changed: devtools\client\animationinspector\components\animation-time-block.js
If you search for the `getTooltipText` function in this file, it should be rather self-explanatory what the problem is.

As Brian said, if direction has a value of `normal` then we just want to not show the direction at all inside the tooltip.

Feel free to ask questions and/or attach a first patch here and I'll assign the bug to you and help you.

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Taccio Yamamoto]

Hi Patrick,

I am new to open-source development but I will try to fix the bug tonight.

Comment 4

[y.gravrand]

Attachment: Bug 1319989 - Don't show direction in tooltip when it is 'normal';

Review commit: https://reviewboard.mozilla.org/r/95806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95806/

Comment 5

[y.gravrand]

Attachment: Bug 1319989 - Better test;

Review commit: https://reviewboard.mozilla.org/r/95808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95808/

Comment 6

[y.gravrand]

Hi!
I submitted a patch on MozReview.
I initially wrote a test which could pass even if no Direction was ever output.
The new test should be better.
Do I need to squash these commits in mercurial? I am new to MozReview :)

BTW, maybe we could put ``controller.animationPlayers[i].state`` in a variable on top of the tests...

Comment 7

[y.gravrand]

Attachment: Bug 1319989 - ESlint;

Review commit: https://reviewboard.mozilla.org/r/95814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95814/

Comment 8

[Patrick Brosset <:pbro>]

Thanks! I've assigned the bug to you now.
Yes please do squash these commits together as one, mozreview unfortunately doesn't show the unified changes in just one diff. And you changed the same lines in 2 separate commits, so it would make sense to squash them. You can use mercurial's histedit command for this.

Feel free to declare a local variable to store `controller.animationPlayers[i].state` if it makes the test more readable.

Other than this, this looks quite good. I'll give this a final review when you have combined the commits into one. Thanks!

Comment 9

[y.gravrand]

Comment on attachment 8814673 [details]
Bug 1319989 - Don't show direction in tooltip when it is 'normal';

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95806/diff/1-2/

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8814673 [details]
Bug 1319989 - Don't show direction in tooltip when it is 'normal';

https://reviewboard.mozilla.org/r/95806/#review95952

Great! Only one minor comment that doesn't prevent me from R+'ing this change.

::: devtools/client/animationinspector/test/doc_simple_animation.html:57
(Diff revision 2)
>      .short {
>        top: 500px;
>        left: 10px;
>        background: red;
>  
> -      animation: simple-animation 2s;
> +      animation: simple-animation 2s normal;

Is this change really needed? `normal` is the default value, right?

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8814673 [details]
Bug 1319989 - Don't show direction in tooltip when it is 'normal';

https://reviewboard.mozilla.org/r/95806/#review95954

Comment 12

[Patrick Brosset <:pbro>]

I pushed your commit to TRY (our CI environment) to see if all tests still pass fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=729859c8b00f
Let's wait for this to run and request to land the change after.

Comment 13

[y.gravrand]

Comment on attachment 8814673 [details]
Bug 1319989 - Don't show direction in tooltip when it is 'normal';

https://reviewboard.mozilla.org/r/95806/#review95952

> Is this change really needed? `normal` is the default value, right?

Yes, but if I remember correctly, it is the way to reproduce the bug; there was no ``Direction`` output if we didn't specify ``normal``.
Let me check this evening and get back to you.

Comment 14

[y.gravrand]

Comment on attachment 8814673 [details]
Bug 1319989 - Don't show direction in tooltip when it is 'normal';

https://reviewboard.mozilla.org/r/95806/#review95952

> Yes, but if I remember correctly, it is the way to reproduce the bug; there was no ``Direction`` output if we didn't specify ``normal``.
> Let me check this evening and get back to you.

I was mistaken, this change is not needed indeed (unless we want to see if explicitely setting the value to ``normal`` changes anything).
I can cancel this change with a new global rewritten commit, would it be OK for you?

Comment 15

[Patrick Brosset <:pbro>]

(In reply to y.gravrand from comment #14)
> Comment on attachment 8814673 [details]
> Bug 1319989 - Don't show direction in tooltip when it is 'normal';
> 
> https://reviewboard.mozilla.org/r/95806/#review95952
> 
> > Yes, but if I remember correctly, it is the way to reproduce the bug; there was no ``Direction`` output if we didn't specify ``normal``.
> > Let me check this evening and get back to you.
> 
> I was mistaken, this change is not needed indeed (unless we want to see if
> explicitely setting the value to ``normal`` changes anything).
> I can cancel this change with a new global rewritten commit, would it be OK
> for you?
Don't worry about it. Your patch is fine as it is now, and TRY is fully green, so we'll just land this as is.
Thanks for your contribution!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea5eda08e2cc
Don't show direction in tooltip when it is 'normal'; r=pbro

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 (2016-11-23) (64-bit) on WIndows 7 , 64 Bit !

This bug's fix is now verified with latest Nightly

Build    ID  :      20161214030231
User  Agent  :      Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 
[bugday-20161214]