Closed Bug 930588 Opened 6 years ago Closed 6 years ago

Dark/Light theme follow-up updates for CodeMirror

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set

Tracking

(firefox27 verified, firefox28 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox27 --- verified
firefox28 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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
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.
(In reply to Paul Rouget [:paul] from comment #2)
> Can you use these colors:
> 
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> DarkTheme-Color-Palette@2x.png
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> LightTheme-Color-Palette@2x.png
> 
> 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.
Attached patch theme-1.patch (obsolete) — Splinter Review
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/DevTools-i04-Build-05@2x.png
Attachment #822354 - Flags: review?(scrapmachines)
Attachment #822354 - Flags: review?(scrapmachines) → review?(paul)
Attachment #822354 - Flags: feedback?(scrapmachines)
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 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.
Attachment #822354 - Flags: feedback?(scrapmachines)
Flags: needinfo?(shorlander)
(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.
(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.
Attached patch theme-2.patch (obsolete) — Splinter Review
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.
Attachment #822354 - Attachment is obsolete: true
Attachment #822354 - Flags: review?(paul)
Attachment #822543 - Flags: review?(paul)
Attachment #822543 - Flags: feedback?(scrapmachines)
For UI-only changes, attaching screenshots generally helps attract more valuable feedback, if it's not too much trouble.
Attached image 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)
Attached image 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).
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 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.
Attachment #822543 - Flags: feedback?(scrapmachines) → feedback+
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
Attachment #822543 - Flags: feedback+ → feedback?(scrapmachines)
Comment on attachment 822543 [details] [diff] [review]
theme-2.patch

!@#$%&*...
Attachment #822543 - Flags: feedback?(scrapmachines) → feedback+
(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.
Attached patch theme-3.patchSplinter Review
Updated patch based on feedback
Attachment #822543 - Attachment is obsolete: true
Attachment #822543 - Flags: review?(paul)
Attachment #822766 - Flags: review?(paul)
Comment on attachment 822766 [details] [diff] [review]
theme-3.patch

Let's land that.

We need proper mockups. I'll talk to Shorlander.
Attachment #822766 - Flags: review?(paul) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/80ff8dbb228b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Can we now get this in Aurora too, please?
Attachment #822766 - Flags: checkin+
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:
Attachment #822766 - Flags: approval-mozilla-aurora?
Comment on attachment 822766 [details] [diff] [review]
theme-3.patch

low risk polish only, css change. Approving on aurora.
Attachment #822766 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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?
Flags: needinfo?(shorlander)
(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..
Keywords: verifyme
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).
Blocks: 916766
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.