Playback rate in animations inspector panel is unreadable in dark theme

RESOLVED FIXED in Firefox 40

Status

DevTools
Inspector
RESOLVED FIXED
3 years ago
6 days ago

People

(Reporter: ntim, Assigned: Chirag Bhatia, Mentored)

Tracking

unspecified
Firefox 40

Firefox Tracking Flags

(firefox40 fixed)

Details

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

Attachments

(5 attachments)

Comment hidden (empty)
Created attachment 8589588 [details]
dark-theme-expanded-dropdown.png
Created attachment 8589589 [details]
dark-theme-collapsed-dropdown.png
Mentor: pbrosset
Whiteboard: [good first bug][lang=css]
(Assignee)

Comment 3

3 years ago
Created attachment 8591764 [details]
animation.jpg

Hi Patrick,

I would like to work on this bug. But I'm unable to reproduce this (screenshot attached). Not sure if I'm looking in the wrong place or something. I'm using the mozilla-central branch on Linux 64-bit btw.
Flags: needinfo?(pbrosset)
(In reply to Chirag Bhatia from comment #3)
> Created attachment 8591764 [details]
> animation.jpg
> 
> Hi Patrick,
> 
> I would like to work on this bug. But I'm unable to reproduce this
> (screenshot attached). Not sure if I'm looking in the wrong place or
> something. I'm using the mozilla-central branch on Linux 64-bit btw.
The attached screenshot doesn't have the playback rate selector at all, so you're probably testing with a build that was done before this new feature landed on m-c.
For info, the feature landed with bug 1144615, 4th of April in m-c.
Also, devtools developments are usually done off the fx-team branch, so make sure you clone and build from there: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking
Flags: needinfo?(pbrosset)
(Assignee)

Comment 5

3 years ago
Ok, I've got the fx-team branch and I was able to reproduce the bug. Can you please tell me where to look for to make the changes necessary to fix this bug?
Flags: needinfo?(pbrosset)
The <select> that is used for the playback rate drop-down is created here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#776
And the <option> elements inside are created right after, here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#785
The CSS file for these elements is here:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/animationinspector.css
In particular, the CSS for the <select> is here:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/animationinspector.css#178

So I guess we just need an extra CSS rule that, if the current theme is dark-theme, sets the color of the text in the <option> elements correctly.
There's a class you can use to prefix your rule to ensure it only applies when the dark theme is ON: .theme-dark
Flags: needinfo?(pbrosset)
(Reporter)

Comment 7

3 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> The <select> that is used for the playback rate drop-down is created here:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> animationinspector/animation-panel.js#776
> And the <option> elements inside are created right after, here:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> animationinspector/animation-panel.js#785
> The CSS file for these elements is here:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> animationinspector.css
> In particular, the CSS for the <select> is here:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> animationinspector.css#178
> 
> So I guess we just need an extra CSS rule that, if the current theme is
> dark-theme, sets the color of the text in the <option> elements correctly.
> There's a class you can use to prefix your rule to ensure it only applies
> when the dark theme is ON: .theme-dark

Or better yet, you can set the color to inherit, or to var(--theme-body-color)
(Assignee)

Comment 8

3 years ago
Created attachment 8593441 [details]
animation-after-fix1.jpg

Thank you for the helpful pointers, Patrick and Tim. I've attached a screenshot that shows the changes.

Let me know if that is the expected behavior or you feel any more change is required. If this is correct, you can assign me the bug and I'll upload the patch :)
Flags: needinfo?(pbrosset)
(Reporter)

Updated

3 years ago
Assignee: nobody → chiragbhatia2006
Status: NEW → ASSIGNED
Thanks for working on this. Bug assigned thanks to Tim.
Please do attach the patch. I'm just wondering, why is the font-size bigger in the select than in the rest of the UI?
Flags: needinfo?(pbrosset)
(Assignee)

Comment 10

3 years ago
Created attachment 8593458 [details] [diff] [review]
1152281.patch

Here's the patch. Let me know if you need me to approach the code fix in any other way. I've not changed the font-size so I'm not sure why it looks slightly bigger than the rest of the text. We could fix that as a separate bug or include it in this one?
Attachment #8593458 - Flags: review?(pbrosset)
Comment on attachment 8593458 [details] [diff] [review]
1152281.patch

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

Looks good to me. Thanks for the fix!
No need for a try build for this simple css fix. Pushed to fx-team directly: 
https://hg.mozilla.org/integration/fx-team/rev/5cc6a0a94086
Attachment #8593458 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/5cc6a0a94086
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40

Updated

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