930588 - Dark/Light theme follow-up updates for CodeMirror

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

Line numbers are dark theme are a little too light: https://www.dropbox.com/s/lhg4v58dbo60vyp/Screenshot%202013-10-24%2011.03.28.png.  Should look more like this: http://cl.ly/image/2W2z033G1f40

Comment 1

[Brian Grinstead [:bgrins]]

The comments seem to be too bright in the light theme, almost matching the string color: https://www.dropbox.com/s/3c5w160fxg7yea5/Screenshot%202013-10-24%2011.04.09.png.  Could use .theme-comment instead.

Comment 2

[Paul Rouget [:paul]]

Can you use these colors:

http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]
http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]

From:

http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #2)
> Can you use these colors:
> 
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail]
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail]
> 
> From:
> 
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/

What I've done is used the existing classes (.theme-fg-color1 - .theme-fg-color7, plus some others) so that it matches the existing colors).  The thought is that once we update the colors in Bug 916766, the editor colors will come along for the ride.  This is especially important that we use the same colors for the markup view, since it matches the UI of the surrounding tree.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: theme-1.patch

Minor CSS updates for the CodeMirror themes.  Using colors from the new dark palette for the line numbers and gutter line, and colors from the light theme for the line numbers and gutter line.  Also made comments grey instead of bright, as in this screenshot: https://s3.amazonaws.com/f.cl.ly/items/2y1o1P2D1E0Q3m410Y28/[e-mail]

Comment 5

[Girish Sharma [:Optimizer]]

A few points after using this patch :

Dark Theme -
 - The color highlighting the current line does not have enough contrast.
 - The selection color is too bright.

Light Theme -
 - The matching bracket is shown in green. This green on top of the blue background for the highlighted row is hard to figure out.

Comment 6

[Girish Sharma [:Optimizer]]

Comment on attachment 822354 [details] [diff] [review]
theme-1.patch

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

apart from the above comments, I think we really need a fresh mockup of dark/light theme in the editor by Stephen so that we can correctly distribute the palette colors amongst different keywords in JS/HTML/CSS.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> A few points after using this patch :
> 
> Dark Theme -
>  - The color highlighting the current line does not have enough contrast.
>  - The selection color is too bright.
> 
> Light Theme -
>  - The matching bracket is shown in green. This green on top of the blue
> background for the highlighted row is hard to figure out.

I will update the patch addressing these contrast concerns and ask for review from there.

> apart from the above comments, I think we really need a fresh mockup of
> dark/light theme in the editor by Stephen so that we can correctly
> distribute the palette colors amongst different keywords in JS/HTML/CSS.

I agree.  I'd be happy to work on implementing nicer editor themes based on mockups using the new color palettes in Bug 916766.  I wonder how the themes will look when we just swap out all the old colors to the new ones - which will automatically happen with Bug 916776 as they are sharing the same color definitions at the moment.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> A few points after using this patch :
> 
> Dark Theme -
>  - The color highlighting the current line does not have enough contrast.
>  - The selection color is too bright.
> 

Regarding the selection color, I actually just sampled the RGB from the normal selection of the dark theme (highlight some text in the inspector tab, for instance).  It is the same color that is used for standard text selection with this theme.  I don't think we should update it to be different from the default.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: theme-2.patch

Changes from first patch plus:
* Added contrast with active line on dark theme.
* Switched to a color from the new light theme for matching bracket with more contrast between the active line color.

Unfortunately have to use this crazy selector: `div.cm-s-mozilla span.CodeMirror-matchingbracket` to override the matching bracket color, that is just a limitation of the way the cm themes are built.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

For UI-only changes, attaching screenshots generally helps attract more valuable feedback, if it's not too much trouble.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: cm-dark-updates.png

With changes on right.  The changes are:

* More contrast for active line (can't see active line on left, but it is a little more visible now)
* Line numbers and gutter next to them are closer in color to the rest of the editor
* Code comments are using theme-comment color (which is grey)

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: cm-light-updates.png

Changes on right.   The differences are:

* Using colors from theme for line numbers and gutters
* Using theme-comment for comments (which is grey)
* Using a different color for matching bracket (see the parens around "Tabzilla" in the right one).

Comment 13

[Brian Grinstead [:bgrins]]

And the hope is that we can land these minor updates for 27, then revisit the editor themes with UX either as part of Bug 916766, or a new bug following that.  Will this be possible at this point with the uplift on 10-28?

Comment 14

[Girish Sharma [:Optimizer]]

Comment on attachment 822543 [details] [diff] [review]
theme-2.patch

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

Looks good.
I guess, just changing the color of the bracket is not enough for recognizing the matching bracket. But then, the feature is not of that importance to block this bug from landing.

May be as part of bug 916766, we should revert back to adding a square border around the brackets, as Orion did.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 822543 [details] [diff] [review]
theme-2.patch

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

Looks like a a solid improvement to me. I'm sure paul can review this on time. We can always uplift css-only changes to Aurora.

::: browser/themes/shared/devtools/light-theme.css
@@ +165,5 @@
>  .CodeMirror-activeline-background { /* selected color with alpha */
> +  background: rgba(185, 215, 253, .35);
> +}
> +
> +div.cm-s-mozilla span.CodeMirror-matchingbracket { /* contast active line */

Typo: contrast

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 822543 [details] [diff] [review]
theme-2.patch

!@#$%&*...

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: cm-matching-bracket2.png

(In reply to Girish Sharma [:Optimizer] from comment #14)]

> May be as part of bug 916766, we should revert back to adding a square
> border around the brackets, as Orion did.

That's an interesting idea.  It looks like Orion only outlined the opposing matching bracket, and we don't currently have a class to target just that one in CM, but we could probably work it out.  Here is what it looks like with the original color applied and an outline instead of changing the color.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: theme-3.patch

Updated patch based on feedback

Comment 19

[Paul Rouget [:paul]]

Comment on attachment 822766 [details] [diff] [review]
theme-3.patch

Let's land that.

We need proper mockups. I'll talk to Shorlander.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/fx-team/rev/80ff8dbb228b

Comment 21

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=80ff8dbb228b

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80ff8dbb228b

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Can we now get this in Aurora too, please?

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 822766 [details] [diff] [review]
theme-3.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 895561
User impact if declined: The text editor in devtools will not match the existing themes, and generally not look as good.
Testing completed (on m-c, etc.): Been on fx-team since 10-28 and m-c since 10-29.  The updates can be seen in nightly builds.
Risk to taking this patch (and alternatives if risky): Minimal, this is a CSS only change inside of DevTools.
String or IDL/UUID changes made by this patch:

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 822766 [details] [diff] [review]
theme-3.patch

low risk polish only, css change. Approving on aurora.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/531f69d6c11e

Comment 27

[Stephen Horlander [:shorlander]]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> Comment on attachment 822354 [details] [diff] [review]
> theme-1.patch
> 
> Review of attachment 822354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> apart from the above comments, I think we really need a fresh mockup of
> dark/light theme in the editor by Stephen so that we can correctly
> distribute the palette colors amongst different keywords in JS/HTML/CSS.

Do we still need this?

Comment 28

[Girish Sharma [:Optimizer]]

(In reply to Stephen Horlander [:shorlander] from comment #27)
> (In reply to Girish Sharma [:Optimizer] from comment #6)
> > Comment on attachment 822354 [details] [diff] [review]
> > theme-1.patch
> > 
> > Review of attachment 822354 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > apart from the above comments, I think we really need a fresh mockup of
> > dark/light theme in the editor by Stephen so that we can correctly
> > distribute the palette colors amongst different keywords in JS/HTML/CSS.
> 
> Do we still need this?

Even there now we have light color palette too, it is a bit hard to decide which exact color to use in which situations .. like for comments, function keyword, other keywords , border, matching bracket, toolbar, etc etc..

Comment 29

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on latest Aurora 27.0a2 (buildID: 20131128004001).

Comment 30

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0

Verified as fixed on latest Aurora 28.0a2 (buildID: 2013121100400).