Closed Bug 789364 Opened 7 years ago Closed 7 years ago

Light theme for markup panel, orion, and rule view

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: harth, Assigned: harth)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch light theme (obsolete) — Splinter Review
Right now the markup panel has a very beautiful dark theme. We need a light theme for it and the rest tools, to appease the masses.

Proposed theme attached. white background with colors:

hsl(90,2%,46%)   light grey (html:comment, orion:comment, rules:source)
hsl(276,47%,46%) purple     (rules:property, orion:keyword)
hsl(72,100%,27%) green      (orion:string)
hsl(208,81%,21%) dark blue  (html:tag)
hsl(208,63%,40%) blue       (html:attrname)
hsl(24,85%,39%)  orange     (html:attrvalue)

It looks okay (: Might be good in the future to find colors that look more awesome together. And also the tag color has a much higher contrast than the attributes in the HTML view, I'm not sure that's a good thing.
Attachment #659087 - Flags: review?(paul)
Mihai pointed out in irc that the line number gutter in orion should be changed from it's original light blue color too.
Attachment #659087 - Attachment is patch: true
Comment on attachment 659087 [details] [diff] [review]
light theme

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

I think we should be able to share these colors across our tools.

You could use the preprocessor or some classes to achieve that.

It would then make our life much easier if we want to:
- make another tool use this color scheme
- tweak color values without having to touch too many files
- implement a dark theme


We could have reusable classes:

.devtools-theme-background {}
.devtools-theme-text {}
.devtools-theme-keyword {}
.devtools-theme-DOMattribute {}
.devtools-theme-comments {}
.devtools-theme-number {}
…

::: browser/devtools/sourceeditor/orion/orion.css
@@ +11,3 @@
>  	font-family: monospace;
>  	font-size: 10pt;
>  }

You add spaces here. This file uses tabs.
Attachment #659087 - Flags: review?(paul) → review-
(In reply to Paul Rouget [:paul] from comment #2)
> Comment on attachment 659087 [details] [diff] [review]
> light theme
> 
> Review of attachment 659087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/sourceeditor/orion/orion.css
> @@ +11,3 @@
> >  	font-family: monospace;
> >  	font-size: 10pt;
> >  }
> 
> You add spaces here. This file uses tabs.

This file shouldn't be changed. orion.css here is the vanilla/unchanged CSS that comes from upstream.
I'd advocate:
a) Add the generic classes (devtools-theme-background, devtools-theme-keyword, etc) to theme/*stripe/devtools/common.css in this patch.
b) Land this patch on m-c/aurora quickly
c) File followups to convert individual tools to the new generic classes.
Attached patch add generic class names (obsolete) — Splinter Review
This patch does as Dave suggests. It adds a few generic theme classes to common.css, the markup panel uses these classes and the other tools have them hard-coded for now.
Assignee: nobody → fayearthur
Attachment #659087 - Attachment is obsolete: true
Attachment #661127 - Flags: review?(paul)
This looks really good, Heather. Thank you very much! I'm really glad to see the markup panel, Orion, the rule view and computed styles view all having a very, very much readable color palette.

Two comments:

- the debugger Orion uses the old theme for some reason?
- the line highlight background seems quite strong now. Shouldn't it be a bit brighter/subtler? (in orion)
Trying it out in my editor. So far, I really like it.
Comment on attachment 661127 [details] [diff] [review]
add generic class names

I like it :) Can you confirm this works well on Windows? (I can't test now).
(or can anyone else test it?)

For Mihai's comments, feel free to fix that now or to file follow-up bugs.

As soon as this lands, please request for an Aurora approval.
Attachment #661127 - Flags: review?(paul) → review+
Updated to comments. Changed the background color to a very light grey for read-only sources like the debugger. Also changed the line highlight to a lighter blue.
Attachment #661127 - Attachment is obsolete: true
(In reply to Heather Arthur [:harth] from comment #0)
> Right now the markup panel has a very beautiful dark theme. We need a light
> theme for it and the rest tools, to appease the masses.

Out of curiosity what qualifies as "the masses"? Is there some evidence that vast amounts of people dislike, or can't use, the dark theme?
(In reply to Stephen Horlander from comment #10)
> (In reply to Heather Arthur [:harth] from comment #0)
> > Right now the markup panel has a very beautiful dark theme. We need a light
> > theme for it and the rest tools, to appease the masses.
> 
> Out of curiosity what qualifies as "the masses"? Is there some evidence that
> vast amounts of people dislike, or can't use, the dark theme?

People feel strangely attached to just the right color scheme, so we did get lots of feedback that dark-only would be polarizing. We're not saying 'no dark theme', just wanting a light theme to be an option when we had dark.

I'm not sure of any popular editor that has a dark theme without support for a light theme, but many significant editors the other way around.
Oh, I totally agree we should have an option :) Maybe I misunderstood the bug. It seems like this is replacing the portions that are currently dark with light not adding a dark and a light theme?

My main concern is about what we present as the default theme out of the box. Currently it is pretty inconsistent but mostly light. Which is a stark contrast to the UI of the tools being dark.
(In reply to Stephen Horlander from comment #12)
> Oh, I totally agree we should have an option :) Maybe I misunderstood the
> bug. It seems like this is replacing the portions that are currently dark
> with light not adding a dark and a light theme?

That is what this bug is doing. It seems worse to ship with some devtools having a dark theme and others light, and best to change our dark-themed tools (so far only the markup panel) to a consistent light theme, as the light theme would be default anyways.

A dark theme option would definitely be in the future, but not in this round, given we want it in Aurora.

> My main concern is about what we present as the default theme out of the
> box. Currently it is pretty inconsistent but mostly light. Which is a stark
> contrast to the UI of the tools being dark.

True. I personally had trouble finding an in-between background color that looked good, but it would be great to find one.

Are you recommending that we do a dark theme as the default?
I'm also worried that while trying to appease users who prefer a light theme, we might end up in a compromise of a dark UI with light editors, forgoing all the benefits of the dark theme that faaborg and shorlander had articulated in the past.

If we have evidence that a default dark theme is hurting the adoption of our tools, then a u-turn on this would probably be the right thing to do, but let's not give up without a fight. And fighting would mean to present the full vision of the dark theme to our users before considering their feedback.
So here are our options:

1. Leave the editing surfaces with a mixture of dark/light themes until we have dark and light themes with an option to switch
2. Change everything to dark with no light option until we have a light theme and an option to switch
3. Make it consistent now until we have proper dark and light themes and a switch

Does anyone have a 4th option, or is anyone suggesting that we should do 1. or 2.?
The thing is we don't have a dark theme right now, it's just that the current theme is sort of a middle ground. In bug 715472 Stephen provided us with the colors of a dark theme and my perhaps mistaken impression was that the markup panel was using those. So in that light it looks like we are changing our minds regarding the dark theme.

If I am mistaken and the markup panel colors do need fixing either way, then how come we don't do 2.? Or, even better, since Heather has already provided a light theme, how about we go all the way and finish the job by adding a theme option?
I like the proposed light theme. Why not have this work completed? A new dark theme option should be a separate bug.
(In reply to Panos Astithas [:past] from comment #16)
> The thing is we don't have a dark theme right now, it's just that the
> current theme is sort of a middle ground.

Yes, it's a middle ground. This patch makes the split more clearly along 'chrome/content' lines (if that makes any sense).

> In bug 715472 Stephen provided us
> with the colors of a dark theme and my perhaps mistaken impression was that
> the markup panel was using those. So in that light it looks like we are
> changing our minds regarding the dark theme.

I see this as a stepping stone to where we want to go.

> If I am mistaken and the markup panel colors do need fixing either way, then
> how come we don't do 2.?

Mostly because of my thoughts in comment 11.

> Or, even better, since Heather has already provided
> a light theme, how about we go all the way and finish the job by adding a
> theme option?

Because we have too much to do. :(
(In reply to Joe Walker [:jwalker] from comment #15)

> 3. Make it consistent now until we have proper dark and light themes and a
> switch

I think this is the best thing to do for now. However once we get a dark theme and a light theme we should change the dark theme to the default.

I haven't had a chance to build this patch yet, are the colors in line with the colors proposed in bug 715472?
(In reply to Stephen Horlander from comment #19)
> (In reply to Joe Walker [:jwalker] from comment #15)
> 
> > 3. Make it consistent now until we have proper dark and light themes and a
> > switch
> 
> I think this is the best thing to do for now. However once we get a dark
> theme and a light theme we should change the dark theme to the default.
> 
> I haven't had a chance to build this patch yet, are the colors in line with
> the colors proposed in bug 715472?

I'm not sure how to answer if 2 color themes are in line with each other.

There are some obvious similarities, the keywords are both purple, and the comments are both grey for example. There are some differences too - orion doesn't have the same set of tokens (which means implementing your theme is not possible with current tech), and in Heather's theme, keywords are applied differently in CSS.

I don't think there are jarring differences that should make us think twice about using this theme, which is the underlying question, I think.
What's the status of this bug? Do we want the light theme in Firefox 17? Or maybe we want to keep the dark theme in 17, see how people react. Should we land the current patch in Firefox 18?

Also, we need a clear plan of what we want to do for the global theme, and we should file the relevant bugs. It is important to come to an agreement at this point (maybe we already did) as this will have an impact on the visual design of the future Toolbox project.
I don't hear anyone arguing against landing it, and I think this patch is both a stepping stone towards where we want to be, and a clear improvement on where we are now.

We shouldn't forget to get a proper dark theme, and completed light theme, and we should land this now.
Heather, can you please make sure that all the followup bugs are filed (some might be already open).
Whiteboard: [land-in-fx-team]
This patch makes the minimap have black on black text.
https://hg.mozilla.org/integration/fx-team/rev/0f91d1acd09b
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
(In reply to Victor Porof [:vp] from comment #24)
> This patch makes the minimap have black on black text.

Filed bug 795956.
Heather, is the background supposed to be grey rgb(127,127,127) or white?

iirc, you said it was supposed to be white, and looking at it, I don't see any background declaration anywhere. So it takes the default window background (grey).
Did anyone test that on Windows and Linux?
Depends on: 795956
(In reply to Paul Rouget [:paul] from comment #27)
> Heather, is the background supposed to be grey rgb(127,127,127) or white?
> 
> iirc, you said it was supposed to be white, and looking at it, I don't see
> any background declaration anywhere. So it takes the default window
> background (grey).

It's supposed to be white. `.devtools-theme-background` has `background-color: white` and the HTML view's body should use that class, so that's really odd.

Optimizer did screenshots on Windows and it looked fine, about the same as Mac.
Can someone else take a screenshot of the new markup view on Mac and post it here?
Attached image markup panel on os x
Definitely not white.
https://hg.mozilla.org/mozilla-central/rev/0f91d1acd09b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
(In reply to Heather Arthur [:harth] from comment #29)
> (In reply to Paul Rouget [:paul] from comment #27)
> > Heather, is the background supposed to be grey rgb(127,127,127) or white?
> > 
> > iirc, you said it was supposed to be white, and looking at it, I don't see
> > any background declaration anywhere. So it takes the default window
> > background (grey).
> 
> It's supposed to be white. `.devtools-theme-background` has
> `background-color: white` and the HTML view's body should use that class, so
> that's really odd.


Looks like the check-in didn't contain that part of my last patch, weird, but the rest looks like it made it. Filed bug 797270.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.