Depends on bug 1184644 that removes some hardcoded colors for the sidemenu selected blue background.

Comment on attachment 8712842 [details] [diff] [review] Patch Review of attachment 8712842 [details] [diff] [review]: ----------------------------------------------------------------- Generally: I like it and it feels lighter and looks like a nice refresh. I'll do another pass on the next pass but clearing review for now - see comments inline. One extra UI thing along with the comments: the box shadow at the bottom of the toolbox tabs looks a little dark now. ::: devtools/client/themes/variables.css @@ +14,5 @@ > * so the formatting should be consistent (i.e. no '}' inside a rule). > */ > > :root.theme-light { > + --theme-body-background: #fff; This is shared with dev edition theme: https://dxr.mozilla.org/mozilla-central/search?q=%23fcfcfc&redirect=true&case=true. Not sure if we want to copy that one across @@ +15,5 @@ > */ > > :root.theme-light { > + --theme-body-background: #fff; > + --theme-sidebar-background: #fefefe; This is shared with gcli and some images: https://dxr.mozilla.org/mozilla-central/search?q=%23f7f7f7&redirect=false&case=true @@ +20,3 @@ > --theme-contrast-background: #e6b064; > > + --theme-tab-toolbar-background: #f4f4f4; This is shared with gcli: https://dxr.mozilla.org/mozilla-central/search?q=%23ebeced&redirect=true&case=true @@ +20,4 @@ > --theme-contrast-background: #e6b064; > > + --theme-tab-toolbar-background: #f4f4f4; > + --theme-toolbar-background: #fcfcfc; This is shared with gcli and some images: https://dxr.mozilla.org/mozilla-central/search?q=%23f0f1f2&redirect=false&case=true @@ +20,5 @@ > --theme-contrast-background: #e6b064; > > + --theme-tab-toolbar-background: #f4f4f4; > + --theme-toolbar-background: #fcfcfc; > + --theme-selection-background: #4a90e2; This color is used in a couple of other places: * Splitview (which I'm assuming is updated in the other bug) * GCLI * Dev Edition tab colors If we want to switch this we should also switch those other places: https://dxr.mozilla.org/mozilla-central/search?q=%234c9ed9&redirect=true&case=true. So I'm wondering if we also want to affect the browser chrome UI for Dev Edition with this change or if we should keep the current selection color. (or alternatively have different styles between the two) @@ +26,2 @@ > --theme-selection-color: #f5f7fa; > + --theme-splitter-color: #dedede; The "Performance" tool has an abnormally dark side splitter with this applied.. it's probably not using the variable. Can you update that please? It's also shared with gcli: https://dxr.mozilla.org/mozilla-central/search?q=%23aaaaaa&redirect=true&case=true

Tried to address some of the concerns Brian had in the above comments and made a few additional changes to the toolbar. Brian, can we follow-up about what work needs to be done to translate this work into DE?

Comment on attachment 8716526 [details] [diff] [review] light-theme-colors.patch Review of attachment 8716526 [details] [diff] [review]: ----------------------------------------------------------------- Lots of notes here - I like where this is heading but we need to make sure we bring all of the other colors along with it so we stay consistent in the UI and also make it as easy as we can to switch colors again in the future. ::: devtools/client/themes/splitters.css @@ +8,4 @@ > background-image: none; > background-color: transparent; > border: 0; > + border-bottom: 1px solid #dde1e4; IIRC this same color is used in light and dark themes and against light and dark page background (hence the .5 alpha value). This might work anyway, I'll have to test locally. Whatever color is used should be duplicated on both devtools-side-splitter and devtools-horizontal-splitter in this file though (used for horizontal and vertical docking) ::: devtools/client/themes/variables.css @@ +15,4 @@ > */ > > :root.theme-light { > + --theme-body-background: white; Technical point - I wonder if we should do something like #fefefe so that we can more easily find/replace other instances in the case of another color change - it would be oh so slightly off white but probably not noticeable on many monitors. Also there are currently a couple of other instances of #fcfcfc here that should be converted: https://dxr.mozilla.org/mozilla-central/search?q=%23fcfcfc&redirect=true&case=true @@ +15,5 @@ > */ > > :root.theme-light { > + --theme-body-background: white; > + --theme-sidebar-background: white; My 2 cents, feel free to disregard: I wish the sidebar had some slightly different background color just to visually distinguish the sidebar a bit (like maybe #fcfcfc or even a bit darker) @@ -15,5 @@ > */ > > :root.theme-light { > - --theme-body-background: #fcfcfc; > - --theme-sidebar-background: #f7f7f7; Please convert other relevant instances of #f7f7f7 in the code base also - I know gcli has some and a couple images too: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%23f7f7f7&redirect=true&case=true @@ +16,5 @@ > > :root.theme-light { > + --theme-body-background: white; > + --theme-sidebar-background: white; > + --theme-contrast-background: #fcfcfc; I don't think this is right. Try changing an attribute in the markup view for instance and see the flash that happens. This should be a dark enough color to support light text in the light theme (esp given the name 'contrast'). Right now it's a kind of orangy thing that's used to flash notifications to users, and it should probably stay as something similar in spirit @@ -21,2 @@ > > - --theme-tab-toolbar-background: #ebeced; Please convert gcli colors: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%23ebeced&redirect=true&case=true @@ -21,3 @@ > > - --theme-tab-toolbar-background: #ebeced; > - --theme-toolbar-background: #f0f1f2; Please convert gcli colors: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%23f0f1f2&redirect=true&case=true @@ +24,2 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); At least based on the name of this var, this isn't consistent but I'm not sure if it's really a problem. I do feel like the contrast isn't great on this though, for something like the hover state in netmonitor requests. @@ +24,3 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); > + --theme-selection-color: white; Same note about 'white' and future replacements of this variable as above with --theme-body-background @@ -24,3 @@ > --theme-selection-background: #4c9ed9; > - --theme-selection-background-semitransparent: rgba(76, 158, 217, .23); > - --theme-selection-color: #f5f7fa; Please convert other instances (including variable for dark theme): https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%23f5f7fa&redirect=true&case=true @@ +29,2 @@ > > + --theme-body-color: #696969; Just a heads up - this is lowering the contrast between text and the background color. Would be worth measuring with an accessibility tool to make sure we are still within a good range. @@ -29,2 @@ > > - --theme-body-color: #18191a; Please convert gcli colors: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%2318191a&redirect=true&case=true ::: devtools/client/themes/widgets.css @@ +13,4 @@ > .theme-light { > --table-splitter-color: rgba(0,0,0,0.15); > --table-zebra-background: rgba(0,0,0,0.05); > + --smw-margin: #dde1e4; I think we could switch all instances of --smw-margin with --theme-comment in this file and be better off for it (less to deal with later on with dark theme also)

(In reply to Brian Grinstead [:bgrins] from comment #5) > ::: devtools/client/themes/variables.css > @@ +15,4 @@ > > */ > > > > :root.theme-light { > > + --theme-body-background: white; > > Technical point - I wonder if we should do something like #fefefe so that we > can more easily find/replace other instances in the case of another color > change - it would be oh so slightly off white but probably not noticeable on > many monitors. Also there are currently a couple of other instances of > #fcfcfc here that should be converted: > https://dxr.mozilla.org/mozilla-central/ > search?q=%23fcfcfc&redirect=true&case=true So I think part of what makes this patch look and feel a lot brighter /is/ that we're using white instead of an off-white of some sort. I guess I'll just have to ask you to trust me on this? I'm not sure if part of this was asking me to switch from the css name to #ffffff; happy to make that change. > @@ +15,5 @@ > > */ > > > > :root.theme-light { > > + --theme-body-background: white; > > + --theme-sidebar-background: white; > > My 2 cents, feel free to disregard: I wish the sidebar had some slightly > different background color just to visually distinguish the sidebar a bit > (like maybe #fcfcfc or even a bit darker) See point above for my thoughts on this. > @@ +24,2 @@ > > --theme-selection-background: #4c9ed9; > > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); > > At least based on the name of this var, this isn't consistent but I'm not > sure if it's really a problem. I do feel like the contrast isn't great on > this though, for something like the hover state in netmonitor requests. So I ran this through a color contrast analyzer to be sure—it does pass our accessibility requirements but I don't feel super strongly about the color—I'm happy to make it lighter. > @@ +29,2 @@ > > > > + --theme-body-color: #696969; > > Just a heads up - this is lowering the contrast between text and the > background color. Would be worth measuring with an accessibility tool to > make sure we are still within a good range. Done! It does.

Thanks for taking over! I didn't have much time to look at this recently due to health problems and exams, but I'm glad this is moving forward :)

Comment on attachment 8716774 [details] [diff] [review] light-refresh.patch Review of attachment 8716774 [details] [diff] [review]: ----------------------------------------------------------------- This patch seems to include two patches (light-theme-colors.patch & high-contrast.patch).

Comment on attachment 8716774 [details] [diff] [review] light-refresh.patch Review of attachment 8716774 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devedition.inc.css @@ +75,4 @@ > --chrome-secondary-background-color: #f5f6f7; > --chrome-navigator-toolbox-separator-color: #cccccc; > --chrome-nav-bar-separator-color: #B6B6B8; > + --chrome-nav-buttons-background: #ffffff; Since we are going to #ffffff we should have a comment pointing back to the variable name that this is matching (kind of like we are doing in commandline.css) ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css @@ +90,4 @@ > #go { > margin-left: 100px; > background-color: #70bf53; /* green */ > + color: white; /* selection text color */ Nit: /* --theme-selection-color */ instead of /* selection text color */ ::: devtools/client/light-theme-colors.patch @@ +1,1 @@ > +# HG changeset patch There are two .patch files included in this patch that should be removed. Was high-contrast.patch intended to be attached as a different patch to the bug? ::: devtools/client/sourceeditor/codemirror/lib/codemirror.css @@ +24,4 @@ > > .CodeMirror-gutters { > border-right: 1px solid #ddd; > + background-color: #ffffff; This file is pulled in from the codemirror repo so it'll get blown away on updates. I don't think we actually need to make this change either since light-theme.css overrides the bg on .CodeMirror-gutters

Whoops! Sorry about the extra patch files...

Just to be clear, the arrows in the screenshot here currently match but now are changing to not match colors. Are you OK with changing it so they don't match anymore? Also notice that the selected tab looks different in terms of the top blue bar that's visible in the browser tabs but not in the devtools tabs.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #8) > (In reply to Brian Grinstead [:bgrins] from comment #5) > > @@ +24,2 @@ > > > --theme-selection-background: #4c9ed9; > > > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); > > > > At least based on the name of this var, this isn't consistent but I'm not > > sure if it's really a problem. I do feel like the contrast isn't great on > > this though, for something like the hover state in netmonitor requests. > So I ran this through a color contrast analyzer to be sure—it does pass our > accessibility requirements but I don't feel super strongly about the > color—I'm happy to make it lighter. Again, just to be clear - the color I was talking about was the light blue state here in the top row of the netmonitor. It can also be seen when hovering a console message.

(In reply to Brian Grinstead [:bgrins] from comment #13) > Created attachment 8717234 [details] > de-theme-comparison.png > > Just to be clear, the arrows in the screenshot here currently match but now > are changing to not match colors. Are you OK with changing it so they don't > match anymore? Yes. > Also notice that the selected tab looks different in terms of the top blue > bar that's visible in the browser tabs but not in the devtools tabs. I'm not sure what you're referring to—I did remove the blue top bar, I didn't think it looked good/added anything. I'm not seeing anything like that in the browser tabs, perhaps I'm missing something? (In reply to Brian Grinstead [:bgrins] from comment #14) > Created attachment 8717237 [details] > netmonitor-hover.png > Again, just to be clear - the color I was talking about was the light blue > state here in the top row of the netmonitor. It can also be seen when > hovering a console message. Okay, good, we're talking about the same color then!

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #15) > > Also notice that the selected tab looks different in terms of the top blue > > bar that's visible in the browser tabs but not in the devtools tabs. > I'm not sure what you're referring to—I did remove the blue top bar, I > didn't think it looked good/added anything. I'm not seeing anything like > that in the browser tabs, perhaps I'm missing something? It might be hard to see in the screenshot, but look at the selected tab's top 2px. You could also look at the dark theme, it's more obvious there.

(In reply to Brian Grinstead [:bgrins] from comment #16) > (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #15) > > > Also notice that the selected tab looks different in terms of the top blue > > > bar that's visible in the browser tabs but not in the devtools tabs. > > I'm not sure what you're referring to—I did remove the blue top bar, I > > didn't think it looked good/added anything. I'm not seeing anything like > > that in the browser tabs, perhaps I'm missing something? > > It might be hard to see in the screenshot, but look at the selected tab's > top 2px. You could also look at the dark theme, it's more obvious there. Oh, I thought by 'browser tabs' you meant in Firefox tabs—hence the confusion. I removed that on purpose for ~visuals~. Are there other places in the tools where we're using a 2px top blue bar to represent selected state?

Comment on attachment 8717222 [details] [diff] [review] light-refresh.patch Review of attachment 8717222 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/styleeditor/test/sourcemap-css/test-bootstrap-scss.css @@ +4041,4 @@ > font-size: 14px; > font-weight: normal; > line-height: 18px; > + background-color: #ffffff; This one could probably be omitted too ::: devtools/client/themes/dark-theme.css @@ +366,4 @@ > } > > .CodeMirror-Tern-fname { > + color: #ffffff; This change can be omitted (it's dark theme only)

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #17) > (In reply to Brian Grinstead [:bgrins] from comment #16) > > (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #15) > > > > Also notice that the selected tab looks different in terms of the top blue > > > > bar that's visible in the browser tabs but not in the devtools tabs. > > > I'm not sure what you're referring to—I did remove the blue top bar, I > > > didn't think it looked good/added anything. I'm not seeing anything like > > > that in the browser tabs, perhaps I'm missing something? > > > > It might be hard to see in the screenshot, but look at the selected tab's > > top 2px. You could also look at the dark theme, it's more obvious there. > > Oh, I thought by 'browser tabs' you meant in Firefox tabs—hence the > confusion. I did, shouldn't have used 'selected tab' alone - it's ambiguous. So without the patch applied, both Firefox tabs and Devtools toolbox tabs have the 2px stripe at the top. With the patch applied, only Firefox tabs have the 2px stripe at top. Another one of those places where at least when the DE theme was made the Firefox tabs were meant to match exactly the Devtools tabs. Not saying it has to stay that way though.. > I removed that on purpose for ~visuals~. Are there other places in the tools > where we're using a 2px top blue bar to represent selected state? Not that I know of

Comment on attachment 8717612 [details] [diff] [review] light-refresh.patch Review of attachment 8717612 [details] [diff] [review]: ----------------------------------------------------------------- I may not get to review it this week - flagging ntim to see if he might be able to check out this patch and give feedback/review comments

Comment on attachment 8717612 [details] [diff] [review] light-refresh.patch Review of attachment 8717612 [details] [diff] [review]: ----------------------------------------------------------------- Looks really great! Can you change the instances of #ffffff and white to #fff ? Thanks! Other than that, here are a few more comments: ::: browser/themes/shared/devedition.inc.css @@ +73,4 @@ > --chrome-secondary-background-color: #f5f6f7; > --chrome-navigator-toolbox-separator-color: #cccccc; > --chrome-nav-bar-separator-color: #B6B6B8; > + --chrome-nav-buttons-background: #ffffff; /* --theme-body-background */ nit: use short hex notation (#fff) ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css @@ +90,4 @@ > #go { > margin-left: 100px; > background-color: #70bf53; /* green */ > + color: white; /* --theme-selection-color */ nit: The comment isn't quite right, white doesn't correspond to the selection color, #f5f7fa does. Also, I usually prefer short hex notation over color names (since it's more consistent) ::: devtools/client/themes/splitters.css @@ +20,4 @@ > background-image: none; > background-color: transparent; > border: 0; > + -moz-border-end: 1px solid #dde1e4; Changing those colors will make the splitters very light on the dark theme, which isn't quite right. Can you find a (rgba) color that works on both themes ? ::: devtools/client/themes/variables.css @@ +15,5 @@ > */ > > :root.theme-light { > + --theme-body-background: white; > + --theme-sidebar-background: white; nit: please use short hex (for consistency) @@ +24,2 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); The network monitor item hover state is still very faint, there's not enough contrast to distinguish a hovered item from a normal item.

(In reply to Tim Nguyen [:ntim] from comment #22) > Comment on attachment 8717612 [details] [diff] [review] > light-refresh.patch > > Review of attachment 8717612 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks really great! Can you change the instances of #ffffff and white to > #fff ? Thanks! So we had this conversation over IRC, but I've left in the instances of 'white'. I think having human-readable code where we can is preferable to hex codes instead of dogmatic consistency for consistency's sake—and we're already using named colors elsewhere, so I'm not sure it's really breaking a pattern. > ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css > @@ +90,4 @@ > > #go { > > margin-left: 100px; > > background-color: #70bf53; /* green */ > > + color: white; /* --theme-selection-color */ > > nit: The comment isn't quite right, white doesn't correspond to the > selection color, #f5f7fa does. Fixed! > ::: devtools/client/themes/splitters.css > @@ +20,4 @@ > > background-image: none; > > background-color: transparent; > > border: 0; > > + -moz-border-end: 1px solid #dde1e4; > > Changing those colors will make the splitters very light on the dark theme, > which isn't quite right. Can you find a (rgba) color that works on both > themes ? I couldn't find a color that worked well for both themes, so I created new variables on the :root in that file for each theme. Can you check that I did it correctly? The variables/css-that-needs-to-be-compiled is all a little mysterious to me, not sure I understand the rules. > @@ +24,2 @@ > > --theme-selection-background: #4c9ed9; > > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); > > The network monitor item hover state is still very faint, there's not enough > contrast to distinguish a hovered item from a normal item. Bumped up the contrast here, should be better.

Comment on attachment 8718960 [details] [diff] [review] light-refresh.patch Review of attachment 8718960 [details] [diff] [review]: ----------------------------------------------------------------- A few more comments, and this should be good to go. ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css @@ +90,4 @@ > #go { > margin-left: 100px; > background-color: #70bf53; /* green */ > + color: white; /* --theme-body-background */ This comment is not right either. --theme-body-background is white for the light theme. Since you've change --theme-selection-color to white, it's safe to put --theme-selection-color instead. ::: devtools/client/styleeditor/test/sourcemap-css/test-bootstrap-scss.css @@ +4041,4 @@ > font-size: 14px; > font-weight: normal; > line-height: 18px; > + background-color: #ffffff; Can you undo this change ? This is unrelated to the devtools theme (it's a stylesheet only used for testing source maps) ::: devtools/client/themes/images/itemArrow-ltr.svg @@ +6,1 @@ > <path fill="#ababab" d="M7,0 6,0 0,6 6,12 7,12 7,11.6 1.5,6 7,.4z"/> "#ababab" should be changed to match the new splitter color ::: devtools/client/themes/images/itemArrow-rtl.svg @@ +6,1 @@ > <path fill="#ababab" d="M1,0 0,0 0,.4 5.5,6 0,11.6 0,12 1,12 7,6z"/> "#ababab" should be changed to match the new splitter color ::: devtools/client/themes/light-theme.css @@ +177,5 @@ > border-color: var(--theme-splitter-color); > } > > +.devtools-sidebar-tabs tabs { > + color: var(--theme-body-color); Instead of introducing this new rule, can you simply change the previous one ? It affects a few other places than the sidebar tabs, but that's not a big problem. ::: devtools/client/themes/variables.css @@ +64,5 @@ > --theme-toolbar-background: #343c45; > --theme-selection-background: #1d4f73; > --theme-selection-background-semitransparent: rgba(29, 79, 115, .5); > + --theme-selection-color: white; > + --theme-splitter-color: #262b35; Can you change the dark splitter color in a different bug ? Let's not expand this bug's scope.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #23) > > ::: devtools/client/themes/splitters.css > > @@ +20,4 @@ > > > background-image: none; > > > background-color: transparent; > > > border: 0; > > > + -moz-border-end: 1px solid #dde1e4; > > > > Changing those colors will make the splitters very light on the dark theme, > > which isn't quite right. Can you find a (rgba) color that works on both > > themes ? > I couldn't find a color that worked well for both themes, so I created new > variables on the :root in that file for each theme. Can you check that I did > it correctly? The variables/css-that-needs-to-be-compiled is all a little > mysterious to me, not sure I understand the rules. Your approach worked. Previously, that wasn't possible since we didn't have the devtoolstheme attributes yet on browser.xul, but now that we have the attribute, this approach works fine. > > @@ +24,2 @@ > > > --theme-selection-background: #4c9ed9; > > > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .5); > > > > The network monitor item hover state is still very faint, there's not enough > > contrast to distinguish a hovered item from a normal item. > Bumped up the contrast here, should be better. Much better, thanks ! Still looks a tad bit faint, but it looks contrast-y enough

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9a56956bfd7

Comment on attachment 8719811 [details] [diff] [review] light-theme.patch Review of attachment 8719811 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I know this has been a lot of iterations on this patch but it *is* getting really close ::: 1243406.patch @@ +1,1 @@ > +commit a7c6c0598ced6470900f81b8c47a887c2ec5f8a8 Please remove this file from the patch ::: browser/themes/shared/devedition.inc.css @@ +33,4 @@ > --tab-hover-background-color: #07090a; > --tab-selection-color: #f5f7fa; > --tab-selection-background-color: #1a4666; > + --tab-selection-box-shadow: none; This change is in the dark theme's variables - it should be in the light's ::: devtools/client/shared/css-reload.js @@ +1,1 @@ > +const { Services } = require("resource://gre/modules/Services.jsm"); This file should be removed ::: devtools/client/themes/commandline.inc.css @@ +26,4 @@ > --gcli-input-color: #b6babf; /* --theme-body-color-alt */ > --gcli-border-color: black; /* --theme-splitter-color */ > --selection-background: #1d4f73; /* --theme-selection-background */ > + --selection-color: #ffffff; /* --theme-selection-color */ This is affecting the dark theme - shouldn't it be removed? ::: devtools/client/themes/dark-theme.css @@ +366,4 @@ > } > > .CodeMirror-Tern-fname { > + color: #ffffff; This is affecting the dark theme - shouldn't it be removed? ::: devtools/client/themes/splitters.css @@ +3,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* Splitters */ > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { So, splitters.css is loaded in both browser.xul and the theme files. Luckily, the border bottom is overridden in the toolbox files by toolbar.css. This could use a comment - something like /* These variables are used in browser.xul but inside the toolbox they are overridden by --theme-splitter-color */ @@ +7,5 @@ > + --devtools-splitter-color: #dde1e4; > +} > + > +:root[devtoolstheme="dark"] .devtools-horizontal-splitter { > + --devtools-splitter-color: #262b35; Where did 262b35 come from - is this the eventual splitter color for the dark theme? Maybe this should be #42484f to match the hardcoded devtools tab splitter color or rgba(118, 121, 125, .5) in the mean time and then tackle changes on that in Bug 1205330? ::: devtools/client/themes/variables.css @@ +24,2 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .75); One weird thing I notice in the markup view, if you hover a closing tag then the text becomes really hard to read because of the higher opacity in this color. If you take a look at it you'll see what I mean. I think the easiest solution to get a very similar appearance to this but without problems like that is to keep the current base color but use less opacity.. So something like rgba(76, 158, 217, 0.15). Can you check it out and see what you think? @@ +24,5 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .75); > + --theme-selection-color: white; > + --theme-splitter-color: #dde1e4; > + --theme-comment: #696969; Nit: I think theme-comment has a different connotation than theme-body-color (i.e. open data:text/html,<!--hi --><p>hi</p>, in the inspector the comment should be slightly grey-er). Doesn't look like this constant has spread across our code base so I'd be OK with doing that in a follow up, but I really think it should be different.

(In reply to Brian Grinstead [:bgrins] from comment #28) > Comment on attachment 8719811 [details] [diff] [review] > light-theme.patch > > Review of attachment 8719811 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, I know this has been a lot of iterations on this patch but it *is* > getting really close Thanks dude~ > ::: devtools/client/themes/splitters.css > @@ +3,4 @@ > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > /* Splitters */ > > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { > > So, splitters.css is loaded in both browser.xul and the theme files. > Luckily, the border bottom is overridden in the toolbox files by > toolbar.css. This could use a comment - something like > > /* These variables are used in browser.xul but inside the toolbox they are > overridden by --theme-splitter-color */ Added what you suggested! > @@ +7,5 @@ > > + --devtools-splitter-color: #dde1e4; > > +} > > + > > +:root[devtoolstheme="dark"] .devtools-horizontal-splitter { > > + --devtools-splitter-color: #262b35; > > Where did 262b35 come from - is this the eventual splitter color for the > dark theme? Maybe this should be #42484f to match the hardcoded devtools > tab splitter color or rgba(118, 121, 125, .5) in the mean time and then > tackle changes on that in Bug 1205330? That was me just trying to find something that worked. I changed it to #42484f and I'll address changing it in the dark theme bug. > ::: devtools/client/themes/variables.css > @@ +24,2 @@ > > --theme-selection-background: #4c9ed9; > > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .75); > > One weird thing I notice in the markup view, if you hover a closing tag then > the text becomes really hard to read because of the higher opacity in this > color. If you take a look at it you'll see what I mean. I think the > easiest solution to get a very similar appearance to this but without > problems like that is to keep the current base color but use less opacity.. > So something like rgba(76, 158, 217, 0.15). Can you check it out and see > what you think? Yeah, you're right, it is hard to see. I went with your suggestion. > @@ +24,5 @@ > > --theme-selection-background: #4c9ed9; > > + --theme-selection-background-semitransparent: rgba(234, 244, 255, .75); > > + --theme-selection-color: white; > > + --theme-splitter-color: #dde1e4; > > + --theme-comment: #696969; > > Nit: I think theme-comment has a different connotation than theme-body-color > (i.e. open data:text/html,<!--hi --><p>hi</p>, in the inspector the comment > should be slightly grey-er). Doesn't look like this constant has spread > across our code base so I'd be OK with doing that in a follow up, but I > really think it should be different. So, having stared at #696969 for a bit now and growing to dislike it as the body color, I ended up changing over --theme-body-color to be darker (#393f4c, sort of a bluish-dark-gray). I grep'd around to check I'd changed over other instances of --theme-body-color but I'd appreciate you double-checking my work here.

Comment on attachment 8720893 [details] [diff] [review] light-theme.patch Review of attachment 8720893 [details] [diff] [review]: ----------------------------------------------------------------- ::: 1243406.patch @@ +1,1 @@ > +commit a7c6c0598ced6470900f81b8c47a887c2ec5f8a8 You forgot to remove this file. ::: devtools/client/themes/projecteditor/projecteditor.css @@ +4,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > :root { > + color: #696969; /* --theme-body-color */ This isn't consistent with the --theme-body-color variable used in variables.css ::: devtools/client/themes/splitters.css @@ +5,5 @@ > /* Splitters */ > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { > + --devtools-splitter-color: #dde1e4; /* These variables are used in > + browser.xul but inside the toolbox they are overridden by > + --theme-splitter-color */ This seems like an odd way to lay-out a comment. I would probably lay it out like this: /* These variables are used in browser.xul but inside the toolbox they are overridden by --theme-splitter-color */ --devtools-splitter-color: #dde1e4;

Comment on attachment 8720893 [details] [diff] [review] light-theme.patch Review of attachment 8720893 [details] [diff] [review]: ----------------------------------------------------------------- ::: 1243406.patch @@ +1,1 @@ > +commit a7c6c0598ced6470900f81b8c47a887c2ec5f8a8 This file needs to be removed ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css @@ +90,4 @@ > #go { > margin-left: 100px; > background-color: #70bf53; /* green */ > + color: white; /* --theme-body-color */ I think this change can be reverted, I believe the DE promo is for dark theme only (#70bf53 is mapping to the dark theme's green, so the selection text color should also be mapping to dark theme) ::: devtools/client/themes/commandline.inc.css @@ +18,2 @@ > --selection-background: #4c9ed9; /* --theme-selection-background */ > + --selection-color: #ffffff; /* --theme-selection-color */ See comment below about the selection color change ::: devtools/client/themes/projecteditor/projecteditor.css @@ +4,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > :root { > + color: #696969; /* --theme-body-color */ I just realized something we could do here that will make future changes easier (and fix the issue here right now with the body color not having been updated to the new one). Let's remove this :root rule entirely, then add 'theme-light' to the class list here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/projecteditor/chrome/content/projecteditor.xul#22. Since that page is already loading the light theme CSS it will get the body class applied automatically. ::: devtools/client/themes/splitters.css @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* Splitters */ > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { > + --devtools-splitter-color: #dde1e4; /* These variables are used in Nit: trailing whitespace @@ +4,5 @@ > > /* Splitters */ > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { > + --devtools-splitter-color: #dde1e4; /* These variables are used in > + browser.xul but inside the toolbox they are overridden by Ditto ::: devtools/client/themes/variables.css @@ +24,3 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15); > + --theme-selection-color: white; Was this change intentional? I saw Bug 1242709 was opened to do just this, and see my comment there: https://bugzilla.mozilla.org/show_bug.cgi?id=1248621#c2. There are other places that need to be updated if you want to include this change (devedition.inc.css mainly). If it were me I'd push that out into another bug since I don't think it's going to be very noticeable and there are unanswered accessibility questions still.

Addressed ntim's comments.

Comment on attachment 8720893 [details] [diff] [review] light-theme.patch Review of attachment 8720893 [details] [diff] [review]: ----------------------------------------------------------------- ::: 1243406.patch @@ +1,1 @@ > +commit a7c6c0598ced6470900f81b8c47a887c2ec5f8a8 This file needs to be removed ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css @@ +90,4 @@ > #go { > margin-left: 100px; > background-color: #70bf53; /* green */ > + color: white; /* --theme-body-color */ I think this change can be reverted, I believe the DE promo is for dark theme only (#70bf53 is mapping to the dark theme's green, so the selection text color should also be mapping to dark theme) ::: devtools/client/themes/commandline.inc.css @@ +18,2 @@ > --selection-background: #4c9ed9; /* --theme-selection-background */ > + --selection-color: #ffffff; /* --theme-selection-color */ See comment below about the selection color change ::: devtools/client/themes/projecteditor/projecteditor.css @@ +4,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > :root { > + color: #696969; /* --theme-body-color */ I just realized something we could do here that will make future changes easier (and fix the issue here right now with the body color not having been updated to the new one). Let's remove this :root rule entirely, then add 'theme-light' to the class list here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/projecteditor/chrome/content/projecteditor.xul#22. Since that page is already loading the light theme CSS it will get the body class applied automatically. ::: devtools/client/themes/splitters.css @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* Splitters */ > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { > + --devtools-splitter-color: #dde1e4; /* These variables are used in Nit: trailing whitespace @@ +4,5 @@ > > /* Splitters */ > +:root[devtoolstheme="light"] .devtools-horizontal-splitter { > + --devtools-splitter-color: #dde1e4; /* These variables are used in > + browser.xul but inside the toolbox they are overridden by Ditto ::: devtools/client/themes/variables.css @@ +24,3 @@ > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15); > + --theme-selection-color: white; Was this change intentional? I saw Bug 1242709 was opened to do just this, and see my comment there: https://bugzilla.mozilla.org/show_bug.cgi?id=1248621#c2. There are other places that need to be updated if you want to include this change (devedition.inc.css mainly). If it were me I'd push that out into another bug since I don't think it's going to be very noticeable and there are unanswered accessibility questions still.

Comment on attachment 8720922 [details] [diff] [review] light-theme.patch Clearing flag - see Comment 33

Comment on attachment 8721091 [details] [diff] [review] light-theme.patch Review of attachment 8721091 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review because of tomato.. everything else looks pretty much good to go. Please update the commit message to be a one liner ::: devtools/client/framework/dev-edition-promo/dev-edition-promo.css @@ +90,4 @@ > #go { > margin-left: 100px; > background-color: #70bf53; /* green */ > + color: #f5f7fa; /* --theme-body-color */ This comment should be reverted also ::: devtools/client/themes/variables.css @@ +15,4 @@ > */ > > :root.theme-light { > + --theme-body-background: tomato; "tomato", I'm guessing this is not on purpose :) ::: devtools/client/themes/webconsole.css @@ +211,4 @@ > .theme-selected .cm-number, > .theme-selected .cm-variable, > .theme-selected .kind-ArrayLike { > + color: #ffffff !important; /* --theme-selection-color */ This change should be reverted (selection color)

Comment on attachment 8721097 [details] [diff] [review] light-theme.patch See Comment 36

I think we posted at the same time, so this patch actually takes your last few comments into account.

Comment on attachment 8721100 [details] [diff] [review] light-theme.patch Review of attachment 8721100 [details] [diff] [review]: ----------------------------------------------------------------- Ship it!

Try push (just in case): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4b8e502de26

(Will wait for try push to complete.)