1221494 - Playback rate selector in the animation-inspector is too big on Linux

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2015-11-04 12.24.48.png

See the screenshot. 
The font-size of the playback rates is too big, and that makes the <select> be too high and hide the border-bottom of the toolbar.

Comment 1

[Jakub Ircow]

Hello, my name is Jakub Ircow. I'm a computer science student and I would like to work on this bug as a part of my assignment.

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Jakub Ircow from comment #1)
> Hello, my name is Jakub Ircow. I'm a computer science student and I would
> like to work on this bug as a part of my assignment.
Thanks for your interest.
First of all, you need to clone the repository and set up your development environment. To do so, please read this doc: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking

Once done, I believe the fix will be limited to changing CSS, you can find the CSS file for the animation-inspector here: \devtools\client\themes\animationinspector.css

Comment 3

[Jakub Ircow]

From what I understand, I need to make the developer tools smaller so that it doesn't hide the bottom of the toolbar( which shows downloads ,bookmarks, history and so on). Is that correct ? The thing that I don't understand is what the playback rates' size has to do with is ? Could you please explain this more clearly ?

Comment 4

[Jakub Ircow]

i have tried to reproduce this bug but I am not seeing any animation rates on the right on the right. It says "no animations were found on the current element". I would attach a print screen but I don't really know how to do it.

Comment 5

[Jakub Ircow]

Attachment: Screenshot from 2015-12-08 19-13-28.png

Here is what i get when I am trying to recreate the bug. Any tips ?

Comment 6

[Patrick Brosset <:pbro>]

You need to display at least one animation in the animation-inspector to reproduce the bug.
Try these steps:
- go to http://bgrins.github.io/devtools-demos/inspector/animation-timing.html
- open the inspector
- find the node with id box1 and select it in the inspector
- in the inspector's sidebar, make sure to switch to the Animations panel

=> You should now see a new toolbar in the animation-inspector that contains a button to change the playback rate.

Comment 7

[Jakub Ircow]

Attachment: animationratesize.png

I was struggling with understanding what is wrong with it for hours just to realise that the bug does not exist or at least I can't reproduce it. If you open the attachment you can see that the font size of the play back rate font is not too big. I have changed the animation rate height to 80% from 100% in the animationinspector.css and then ran the build but I am not sure that it saved so if you are still getting this bug this may be the reason why I am not. I hope that you understand what I mean and I am looking forward to your reply.

Comment 8

[Jakub Ircow]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6)
> You need to display at least one animation in the animation-inspector to
> reproduce the bug.
> Try these steps:
> - go to
> http://bgrins.github.io/devtools-demos/inspector/animation-timing.html
> - open the inspector
> - find the node with id box1 and select it in the inspector
> - in the inspector's sidebar, make sure to switch to the Animations panel
> 
> => You should now see a new toolbar in the animation-inspector that contains
> a button to change the playback rate.

I really need your reply Patrick as these comments allow me to get a better grade ;). thank you

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Jakub Ircow from comment #7)
> Created attachment 8696993 [details]
> animationratesize.png
> 
> I was struggling with understanding what is wrong with it for hours just to
> realise that the bug does not exist or at least I can't reproduce it. If you
> open the attachment you can see that the font size of the play back rate
> font is not too big.
Hmm, so I *do* think it's too big. If you look at the text just next to the playback rate selector, the thing that indicates the current time, it's smaller/lighter. For consistency reason, these 2 things should have the same font-size/color
> I have changed the animation rate height to 80% from
> 100% in the animationinspector.css and then ran the build but I am not sure
> that it saved so if you are still getting this bug this may be the reason
> why I am not. I hope that you understand what I mean and I am looking
> forward to your reply.
Not sure what happened there. Once you built firefox once (with `mach build`) you need to rebuild the UI incrementally every time you make a change, and you should do this with `mach build faster`.

Comment 10

[Jakub Ircow]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #9)
> (In reply to Jakub Ircow from comment #7)
> > Created attachment 8696993 [details]
> > animationratesize.png
> > 
> > I was struggling with understanding what is wrong with it for hours just to
> > realise that the bug does not exist or at least I can't reproduce it. If you
> > open the attachment you can see that the font size of the play back rate
> > font is not too big.
> Hmm, so I *do* think it's too big. If you look at the text just next to the
> playback rate selector, the thing that indicates the current time, it's
> smaller/lighter. For consistency reason, these 2 things should have the same
> font-size/color
> > I have changed the animation rate height to 80% from
> > 100% in the animationinspector.css and then ran the build but I am not sure
> > that it saved so if you are still getting this bug this may be the reason
> > why I am not. I hope that you understand what I mean and I am looking
> > forward to your reply.
> Not sure what happened there. Once you built firefox once (with `mach
> build`) you need to rebuild the UI incrementally every time you make a
> change, and you should do this with `mach build faster`.

ohhh ok I thought it's only that the animation rate button was going over the bottom border. I'll look into that and thank you for putting effort to explain it properly. :D

Comment 11

[obongofon]

I am going to attach a patch soon.

Comment 12

[Jakub Ircow]

I can't find the font-size of any sort I've been looking for it in all the directories and it is nowhere to be found.

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Jakub Ircow from comment #12)
> I can't find the font-size of any sort I've been looking for it in all the
> directories and it is nowhere to be found.
Fonts are defined globally for devtools at the very top of this pre-processed CSS file:
\devtools\client\themes\common.css

Now, for the specific case of the playback rate drop-down box, it seems like adding:

color: var(--theme-body-color-alt);
font-size: 1em;

to the #timeline-rate select rule in \devtools\client\themes\animationinspector.css would fix the issue (note that I haven't tested this on Linux).

Comment 14

[Matt O'Connor [:mattoc]]

Hi, can someone assign this bug to me please.

Comment 15

[Patrick Brosset <:pbro>]

(In reply to Matt O'Connor [:mattoc] from comment #14)
> Hi, can someone assign this bug to me please.
Hi, this bug has 3 persons interested in, and enough instructions to get started I think, so at this stage, I'd prefer someone to attach a patch before assigning it to someone.

Comment 16

[Sanyam Khurana [:CuriousLearner]]

Attachment: Changes to #timeline-rate select rule for reducing the size of playback selector

I would like to take up this bug as my first one. I've attached this patch here as per discussions in the comments. Please guide me further.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8712104 [details] [diff] [review]
Changes to #timeline-rate select rule for reducing the size of playback selector

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

Looks good, just a couple of minor changes and it's ready to go.

::: devtools/client/themes/animationinspector.css
@@ +156,4 @@
>    text-align: center;
>    color: inherit;
>    font-family: inherit;
> +  color: var(--theme-body-color-alt);

Why not simply var(--theme-body-color) here? Upon closer examination, I think this is the right color for this element, since that's the same as the color of the timestamp label next to it.
Also, please remove the 'color:inherit' 2 lines before, no need for 2 color declarations.

@@ +156,5 @@
>    text-align: center;
>    color: inherit;
>    font-family: inherit;
> +  color: var(--theme-body-color-alt);
> +  font-size: 1em;

This seems to work nicely for reducing the size of the font to match the other text in the toolbar.

However, I've tested this on mac and windows, and I see the label is not correctly aligned vertically anymore.
After fiddling around with various solutions, I found this one to work:

First, add a new rule to "contain" the select element and give it enough horizontal room:

#timeline-rate {
  position: relative;
  width: 4em;
}

And second, add the following properties to the select rule:

position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;

Comment 18

[Sanyam Khurana [:CuriousLearner]]

Attachment: Bug#1221494.patch

Hi,

I've done the changes, please check this and let me know if something more needs to be done.

Thank You!

Comment 19

[Sanyam Khurana [:CuriousLearner]]

Attachment: bug-1221494-fix.patch

Sorry, attached patch-v1 by mistake. Here is the correct patch.

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8714091 [details] [diff] [review]
bug-1221494-fix.patch

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

::: devtools/client/themes/animationinspector.css
@@ +178,5 @@
> +  position: relative;
> +  width: 4em;
> +}
> +
> +select {

Why create a new rule with this very specific selector? You already have a rule just before with "#timeline-rate select" which is exactly what you need. 
Please move these properties inside of that rule instead.

Comment 21

[Sanyam Khurana [:CuriousLearner]]

Attachment: bug-1221494-fix-v3.patch

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8714281 [details] [diff] [review]
bug-1221494-fix-v3.patch

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

Looks good, thanks!
This is an isolated CSS-only change, so there's no need for a TRY push, and tests still pass locally.
So I've pushed this commit to fx-team: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=71957edda717

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71957edda717

Comment 24

[Sanyam Khurana [:CuriousLearner]]

Thank you so much! Since this was my first patch, let me know which bugs I should work on/ which path I should follow. Thanks once again.

Comment 25

[Patrick Brosset <:pbro>]

(In reply to Sanyam Khurana [:CuriousLearner] from comment #24)
> Thank you so much! Since this was my first patch, let me know which bugs I
> should work on/ which path I should follow. Thanks once again.
Here's a slightly more complex bug, involving JS too: bug 1222937.
Please let me know in the bug if you need help getting started.