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

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Developer Tools: Animation Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbro, Assigned: CuriousLearner, Mentored)

Tracking

unspecified
Firefox 47
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [good first bug][lang=css][polish-backlog])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8683034 [details]
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

2 years ago
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.
(Reporter)

Comment 2

2 years ago
(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

2 years ago
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

2 years ago
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

2 years ago
Created attachment 8696725 [details]
Screenshot from 2015-12-08 19-13-28.png

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

Comment 6

2 years ago
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

2 years ago
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. 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

2 years ago
(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
(Reporter)

Comment 9

2 years ago
(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

2 years ago
(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

2 years ago
I am going to attach a patch soon.

Comment 12

2 years ago
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.
(Reporter)

Updated

2 years ago
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
(Reporter)

Comment 13

2 years ago
(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).
Hi, can someone assign this bug to me please.
(Reporter)

Comment 15

2 years ago
(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.
(Assignee)

Comment 16

2 years ago
Created attachment 8712104 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Attachment #8712104 - Flags: review?(pbrosset)
(Reporter)

Comment 17

2 years ago
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%;
Attachment #8712104 - Flags: review?(pbrosset)
(Reporter)

Updated

2 years ago
Assignee: nobody → sanyam.khurana01
(Assignee)

Comment 18

2 years ago
Created attachment 8714090 [details] [diff] [review]
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!
Attachment #8712104 - Attachment is obsolete: true
Attachment #8714090 - Flags: review?(pbrosset)
(Assignee)

Comment 19

2 years ago
Created attachment 8714091 [details] [diff] [review]
bug-1221494-fix.patch

Sorry, attached patch-v1 by mistake. Here is the correct patch.
Attachment #8714090 - Attachment is obsolete: true
Attachment #8714090 - Flags: review?(pbrosset)
Attachment #8714091 - Flags: review?(pbrosset)
(Reporter)

Comment 20

2 years ago
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.
Attachment #8714091 - Flags: review?(pbrosset)
(Assignee)

Comment 21

2 years ago
Created attachment 8714281 [details] [diff] [review]
bug-1221494-fix-v3.patch
Attachment #8714091 - Attachment is obsolete: true
Attachment #8714281 - Flags: review?(pbrosset)
(Reporter)

Comment 22

2 years ago
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
Attachment #8714281 - Flags: review?(pbrosset) → review+

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71957edda717
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Assignee)

Comment 24

2 years ago
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.
(Reporter)

Comment 25

2 years ago
(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.
You need to log in before you can comment on or make changes to this bug.