1246313 - refresh code mirror colors for better contrast

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Helen V. Holmes (:helenvholmes) (:✨)]

I think it would be nice to update a lot of our variables.css colors to be brighter and have higher contrast.

Comment 1

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: Screen Shot 2016-02-05 at 7.03.22 PM.png

A look at the direction I'd like to go.

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

Starter patch, matches the screenshot

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: passes-AA.png

Did some work to make sure the colors pass AA.

Comment 4

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

Comment 5

[Helen V. Holmes (:helenvholmes) (:✨)]

One thing I'm uncertain about is that I'm definitely flattening the number of colors used in the tools (going from ~5 to ~3, across the markup view to the tools).

This patch is 80% subjective, 20% quantitative (or, 80% making something that I think looks good, 20% making something that passes AA). Anyone have opinions/reasons for why we removing the overall number of colors we're using would be a bad idea?

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast-alt.patch

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

Comment 8

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: comparison.png

A comparison of the three (current + proposals) different inspectors.

I don't think this is impacting the Style Editor negatively, but also not sure how to gauge that?

Comment 9

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: style-editor.png

Comment 10

[Helen V. Holmes (:helenvholmes) (:✨)]

I haven't touched the Firebug theme at all (was under the impression that we wanted the Firebug theme to ship looking as much like Firebug as possible) but Gabe's proposal might work well for it.

Comment 11

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

Rebased with today's fx-team

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8743542 [details] [diff] [review]
high-contrast.patch

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

I think all I had to say about this was in our IRC conversation: 

11:59 PM <ntim> helenvholmes: about the markup refresh, the changes look great ! a few comments though
12:00 AM <ntim> in the debugger, I would love to see `var`, `for` have a different colour than the object names
12:01 AM <ntim> all JS keywords
12:01 AM <•helenvholmes> ntim: yeah, it occurred to me after I flagged you for review that I hadn't looked at the Debugger at all

Comment 13

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: highcontrast.patch

Hey Brian, I know you're kinda busy, but would you be willing to check out this patch?

It doesn't look fabulous in dark theme currently because it assumes the background colors will change with Bug 1205330. 

The box model colors are unchanged as Gabe's changing those over with Bug 1128209.

Comment 14

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

Forgot the "r=bgrins" flag in the commit message.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #13)
> It doesn't look fabulous in dark theme currently because it assumes the
> background colors will change with Bug 1205330.

Sounds like this is dependent on 1205330 then?

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8744944 [details] [diff] [review]
high-contrast.patch

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

One general thing that needs to be fixed - syntax highlighting for DOM nodes in the console isn't matching the inspector (try executing document.body in the console).  In that case it's adding a class 'console-string' instead of 'theme-fg-color6' so we should change the class that's being added here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3129.  We should also check text content colors in that case to make sure it matches what you see in the markup view.

::: devtools/client/themes/light-theme.css
@@ -100,4 @@
>  .theme-fg-color2,
>  .cm-s-mozilla .cm-attribute,
>  .cm-s-mozilla .cm-builtin,
> -.cm-s-mozilla .cm-def,

cm-def has been removed and not added anywhere.  is this intentional?

@@ +177,4 @@
>    border-color: var(--theme-splitter-color);
>  }
>  
> +.devtools-sidebar-tabs tabs {

There's already a rule doing just this in the file (search for .devtools-sidebar-tabs tabs)

::: devtools/client/themes/variables.css
@@ +87,5 @@
>    --theme-highlight-lightorange: #d99b28;
>    --theme-highlight-orange: #d96629;
>    --theme-highlight-red: #eb5368;
>    --theme-highlight-pink: #df80ff;
> +  --theme-highlight-gray: #e9f4fe;

If a new variable is being added (especially one that seems generally useful like a gray) then we should add it in all themes so it can be used safely by tools

@@ +132,2 @@
>    --theme-highlight-purple: #5b5fff;
> +  --theme-highlight-lightorange: #39cf65; /* todo */

What are the todos for?  Still working out colors?

Comment 18

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

(In reply to Brian Grinstead [:bgrins] from comment #16)
> ::: devtools/client/themes/light-theme.css
> @@ -100,4 @@
> >  .theme-fg-color2,
> >  .cm-s-mozilla .cm-attribute,
> >  .cm-s-mozilla .cm-builtin,
> > -.cm-s-mozilla .cm-def,
> 
> cm-def has been removed and not added anywhere.  is this intentional?
Kinda—wanted it to just inherit the unstyled body-copy-color. I redeclared it to be more obvious that was my intent.

> @@ +177,4 @@
> >    border-color: var(--theme-splitter-color);
> >  }
> >  
> > +.devtools-sidebar-tabs tabs {
> 
> There's already a rule doing just this in the file (search for
> .devtools-sidebar-tabs tabs)
:+1:

> ::: devtools/client/themes/variables.css
> @@ +87,5 @@
> >    --theme-highlight-lightorange: #d99b28;
> >    --theme-highlight-orange: #d96629;
> >    --theme-highlight-red: #eb5368;
> >    --theme-highlight-pink: #df80ff;
> > +  --theme-highlight-gray: #e9f4fe;
> 
> If a new variable is being added (especially one that seems generally useful
> like a gray) then we should add it in all themes so it can be used safely by
> tools
Done!

> @@ +132,2 @@
> >    --theme-highlight-purple: #5b5fff;
> > +  --theme-highlight-lightorange: #39cf65; /* todo */
> 
> What are the todos for?  Still working out colors?
This is me being bad at rebasing—I didn't mean to affect the firebug theme at all (with the exception of now adding the --theme-highlight-gray). I think I fixed it?

Comment 19

[Tim Nguyen :ntim]

Is it intentional that the Debugger and the Style Editor looks less colourful now ? The inspector definitively looks better though.

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8746616 [details] [diff] [review]
high-contrast.patch

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

Code changes are fine with me.  This should wait to land until Bug 1205330, correct?

::: devtools/client/themes/light-theme.css
@@ +225,5 @@
>    color: black;
>  }
>  
> +div.CodeMirror span.CodeMirror-matchingbracket {
> +  color: blue;

Is this meant to be actually blue and not --theme-highlight-blue / --theme-highlight-bluegrey ?

Comment 21

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: high-contrast.patch

(In reply to Tim Nguyen :ntim from comment #19)
> Is it intentional that the Debugger and the Style Editor looks less
> colourful now ? The inspector definitively looks better though.

Yep, this is intentional.


(In reply to Brian Grinstead [:bgrins] from comment #20)
> Comment on attachment 8746616 [details] [diff] [review]
> high-contrast.patch
> 
> Review of attachment 8746616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code changes are fine with me.  This should wait to land until Bug 1205330,
> correct?
Yes, that would be preferable! The AA enhancements would be moot in the dark theme without that bug getting applied too.

> ::: devtools/client/themes/light-theme.css
> @@ +225,5 @@
> >    color: black;
> >  }
> >  
> > +div.CodeMirror span.CodeMirror-matchingbracket {
> > +  color: blue;
> 
> Is this meant to be actually blue and not --theme-highlight-blue /
> --theme-highlight-bluegrey ?

Ah, this is unintentional—I've removed it in the above patch. (Otherwise, should all be the same.)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/217746cce0e6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/217746cce0e6

Comment 25

[Mohammad Maruf Rahman]

This bug was about to implement variables.css colors to be brighter and have higher contrast as Description. I have seen the feature being implemented with latest Beta 49.0b3 on Windows 7, 64 Bit!

The bug's fix is verified on latest Beta 49.0b3 

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

[testday-20160812]