Closed Bug 1317414 Opened 3 years ago Closed 3 years ago

Animation names are barely visible with the dark theme

Categories

(DevTools :: Inspector: Animations, defect, P1)

defect

Tracking

(firefox52 verified, firefox53 verified)

VERIFIED FIXED
Firefox 53
Tracking Status
firefox52 --- verified
firefox53 --- verified

People

(Reporter: pbro, Assigned: andre, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(3 files, 2 obsolete files)

Attached image Capture.PNG
STR:
- open jsbin.com
- open devtools
- switch to the dark theme (in the settings panel)
- go to the animation inspector tool
- click on the jsbin logo (this triggers an animation on the page)

=> animations appear in the tool, their names are not easily visible. See screenshot.
Not exactly a regression, but due to bug 1210795 nevertheless.
Now that animations have a specific shape (they're not just rectangles), it means that their names don't fit nicely within them all the time. So bug 1210795 added a semi-transparent background to make them more visible, but we didn't test with the dark theme enough.

This should be fixed quickly and uplifted to 52 to avoid riding the trains with this, since it hides an important piece of information.
See Also: → 1210795
Finding the right background color and text color in both themes will take some trial and errors, but actually doing the fix should be really easy. So, I'm flagging this as a good first bug if anyone with some CSS skills wants to try this out.

Make sure you know how to contribute to DevTools first: https://developer.mozilla.org/en-US/docs/Tools/Contributing

Then head over to this CSS file: devtools\client\themes\animationinspector.css

The best way to make CSS changes to devtools and see how these changes look in real-time is to use the browser toolbox (basically: devtools for devtools): https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

As far as I can see, the color of the text is defined in rule .animation-timeline .animation .name {}
And the background in rule .animation-timeline .animation .name div {}

Now, there's a way to define different colors for the light and dark theme by using the .theme-dark and .theme-light classes as prefixes of the CSS rule selectors.
Mentor: pbrosset
Keywords: good-first-bug
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Actually, let's make this a P1, and uplift it to 52 asap.
Priority: P2 → P1
Attached patch Bug1317414.patch (obsolete) — Splinter Review
This patch changes the "--theme-selection-color3" variable inside the ".theme-dark" to a dark value. Don't know if this is a viable approach to solve this bug but it works. I am also attaching a screenshot of the result.
Attachment #8812459 - Flags: review?(pbrosset)
Attachment #8812459 - Flags: feedback?(pbrosset)
Attached image tentative-fix-animationinspector-css.png (obsolete) —
This is a screenshot with the modification from the tentative solution I have submitted. Looks more readable.
I think --theme-selection-color3 is a typo.
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #7)
> I think --theme-selection-color3 is a typo.
I think the same about that. the only place is this https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/animationinspector.css#378 in the entire codebase that says about that value. pretty strange
(In reply to andre from comment #6)
> Created attachment 8812460 [details]
> tentative-fix-animationinspector-css.png
> 
> This is a screenshot with the modification from the tentative solution I
> have submitted. Looks more readable.
The screenshot looks good! Way more readable indeed.
Thanks for working on this. I'll assign the bug to you, and start reviewing the patch.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #7)
> I think --theme-selection-color3 is a typo.
(In reply to [:Towkir] Ahmed from comment #8)
> I think the same about that. the only place is this
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> animationinspector.css#378 in the entire codebase that says about that
> value. pretty strange
Right, good catch! This variable does not exist indeed. And I don't think it makes sense creating it as Andre's patch suggests. The only other variable we have that has a similar name is --theme-selection-color, and we don't have a --theme-selection-color1 or --theme-selection-color2, so creating a --theme-selection-color3 seems strange.
Assignee: nobody → andre
Status: NEW → ASSIGNED
Comment on attachment 8812459 [details] [diff] [review]
Bug1317414.patch

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

::: devtools/client/themes/animationinspector.css
@@ +10,4 @@
>    --pause-image: url(chrome://devtools/skin/images/pause.svg);
>    --rewind-image: url(chrome://devtools/skin/images/rewind.svg);
>    --play-image: url(chrome://devtools/skin/images/play.svg);
> +  --theme-selection-color3: rgba(10,10,10,0.98);

As I said in comment 9, I don't think that defining this variable is the right approach here.
It is indeed used in animationinspector.css in the '.animation-timeline .animation .name' rule, but I think this must have been a typo or something like this. 
In fact, even if I change it to --theme-selection-color instead, it doesn't look good with the light theme, it's white on white, so basically unreadable.

For information, we have 2 levels of variables most of the time. What we call "theme variables" is what you can fine inside the devtools\client\themes\variables.css file. All the variables that start with --theme are colors we use throughout the tools, to have some consistency and make it easier to create new themes.
Then, on top of this, some other stylesheets define their own variables. Like this one here which defines a number of colors, distances and images. This is just to make it easier to write the CSS without having to duplicate information.

Now, to actually solve this problem, instead of adding this variable here, you'll need to find a color that works well in the '.animation-timeline .animation .name' rule.
You should try colors defined in devtools\client\themes\variables.css. If you find one that is readable in the 3 themes we have, bingo, let's use this one.

If not, it may be that we need one color for the dark theme, and another one for the light and firebug themes.
These is easily feasible by using the color that works for the light and Firebug themes, and then define another one in another rule like: '.theme-dark .animation-timeline .animation .name { color: <another color>; }'

Let me know if that makes sense.
Attachment #8812459 - Flags: review?(pbrosset)
Attachment #8812459 - Flags: feedback?(pbrosset)
Gotcha! Thanks for pointing all these to me. Sorry for the confusion, I am new to the codebase. Upon further diving on the variables.css, I noticed that there is a --theme-content-color3 variable, I believe that the typo was actually trying to be this color reference for it works well in all themes. I am now attaching a new patch and another screenshot showing it on all themes.
Attached image fix-for-all-themes.png
Animation Inspector being readable in all themes.
Attachment #8812459 - Attachment is obsolete: true
Attachment #8812460 - Attachment is obsolete: true
Attached patch Bug1317414.patchSplinter Review
This new patch uses --theme-content-color3 variable present in all three themes for the label. It works well.
Attachment #8812831 - Flags: review?(pbrosset)
Attachment #8812831 - Flags: feedback?(pbrosset)
Comment on attachment 8812831 [details] [diff] [review]
Bug1317414.patch

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

Sweet! The screenshots look great, and it's just a one line CSS change for a color, so let's R+ this change and ask for it to be checked-in the code base.
Thanks for your help.
Attachment #8812831 - Flags: review?(pbrosset)
Attachment #8812831 - Flags: review+
Attachment #8812831 - Flags: feedback?(pbrosset)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d1b6c75329
Fix :"Animation names are barely visible with the dark theme". r=pbrosset
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9d1b6c75329
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8812831 [details] [diff] [review]
Bug1317414.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1210795
[User impact if declined]: If declined, animation names in the animation-inspector tool will be really hard to read when people use the dark theme in devtools.
[Describe test coverage new/current, TreeHerder]: No automated test added for this. I verified this manually in m-c.
[Risks and why]: Just a simple one-line CSS color change. No risk associated with this.
[String/UUID change made/needed]: None
Attachment #8812831 - Flags: approval-mozilla-aurora?
Comment on attachment 8812831 [details] [diff] [review]
Bug1317414.patch

devtools css fix, for aurora52
Attachment #8812831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1320155
I have reproduce this bug with Nightly 53.0a1 (2016-11-14) (64-bit) on Windows 7, 64 Bit !

This bug's fix is verified with latest Aurora and latest  Nightly

Build   ID  20161126004005
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

Build   ID  20161126030207
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
 
[bugday-20161123]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.