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

RESOLVED FIXED in Firefox 53

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: birtles, Assigned: y.gravrand, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
Posted image 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)
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.
Mentor: pbrosset
Keywords: good-first-bug
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3

Comment 3

3 years ago
Hi Patrick,

I am new to open-source development but I will try to fix the bug tonight.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 6

3 years ago
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 hidden (mozreview-request)
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!
Assignee: nobody → y.gravrand
Status: NEW → ASSIGNED
Flags: needinfo?(y.gravrand)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8814674 - Attachment is obsolete: true
Attachment #8814674 - Flags: review?(pbrosset)
Assignee

Updated

3 years ago
Attachment #8814676 - Attachment is obsolete: true
Attachment #8814676 - Flags: review?(pbrosset)

Comment 10

3 years ago
mozreview-review
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

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

https://reviewboard.mozilla.org/r/95806/#review95954
Attachment #8814673 - Flags: review?(pbrosset) → review+
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.
Flags: needinfo?(y.gravrand)
Assignee

Comment 13

3 years ago
mozreview-review-reply
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.
Assignee

Comment 14

3 years ago
mozreview-review-reply
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?
(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

3 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea5eda08e2cc
Don't show direction in tooltip when it is 'normal'; r=pbro

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea5eda08e2cc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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]

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.