Closed Bug 1246313 Opened 5 years ago Closed 5 years ago

refresh code mirror colors for better contrast

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox49 verified)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: hholmes, Assigned: hholmes)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 9 obsolete files)

152.63 KB, image/png
Details
392.22 KB, image/png
Details
85.43 KB, image/png
Details
6.48 KB, patch
hholmes
: review+
Details | Diff | Splinter Review
I think it would be nice to update a lot of our variables.css colors to be brighter and have higher contrast.
Attached image Screen Shot 2016-02-05 at 7.03.22 PM.png (obsolete) —
A look at the direction I'd like to go.
Depends on: 1242709
Attached patch high-contrast.patch (obsolete) — Splinter Review
Starter patch, matches the screenshot
Attached image passes-AA.png
Did some work to make sure the colors pass AA.
Attachment #8716528 - Attachment is obsolete: true
Attached patch high-contrast.patch (obsolete) — Splinter Review
Attachment #8716533 - Attachment is obsolete: true
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?
Attached patch high-contrast-alt.patch (obsolete) — Splinter Review
Attached patch high-contrast.patch (obsolete) — Splinter Review
Attachment #8743374 - Attachment is obsolete: true
Attached image 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?
Attachment #8743444 - Flags: review?(ntim.bugs)
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.
Attached patch high-contrast.patch (obsolete) — Splinter Review
Rebased with today's fx-team
Attachment #8743542 - Flags: review?(ntim.bugs)
Attachment #8743444 - Attachment is obsolete: true
Attachment #8743444 - Flags: review?(ntim.bugs)
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
Attachment #8743542 - Flags: review?(ntim.bugs) → feedback+
Attached patch highcontrast.patch (obsolete) — Splinter Review
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.
Attachment #8743389 - Attachment is obsolete: true
Attachment #8743542 - Attachment is obsolete: true
Attachment #8744943 - Flags: review?(bgrinstead)
Attached patch high-contrast.patch (obsolete) — Splinter Review
Forgot the "r=bgrins" flag in the commit message.
Attachment #8744943 - Attachment is obsolete: true
Attachment #8744943 - Flags: review?(bgrinstead)
Attachment #8744944 - Flags: review?(bgrinstead)
(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?
Depends on: 1205330
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?
Attachment #8744944 - Flags: review?(bgrinstead)
Duplicate of this bug: 997961
Attached patch high-contrast.patch (obsolete) — Splinter Review
(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?
Attachment #8746616 - Flags: review?(bgrinstead)
Is it intentional that the Debugger and the Style Editor looks less colourful now ? The inspector definitively looks better though.
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 ?
Attachment #8746616 - Flags: review?(bgrinstead) → review+
(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.)
Attachment #8744944 - Attachment is obsolete: true
Attachment #8746616 - Attachment is obsolete: true
Attachment #8747146 - Flags: review+
Assignee: nobody → hholmes
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/217746cce0e6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Duplicate of this bug: 1184019
Depends on: 1271841
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]
Status: RESOLVED → VERIFIED
Depends on: 1327993
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.