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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Animation Inspector
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

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

Tracking

({good-first-bug})

Trunk
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 months ago
Created attachment 8813949 [details]
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@mozilla.com
Keywords: good-first-bug
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3

Comment 3

5 months 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

5 months 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

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

Updated

5 months ago
Attachment #8814676 - Attachment is obsolete: true
Attachment #8814676 - Flags: review?(pbrosset)

Comment 10

5 months 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

5 months 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

5 months 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

5 months 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

5 months 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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea5eda08e2cc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: affected → fixed
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]
You need to log in before you can comment on or make changes to this bug.