Skip to content
This repository has been archived by the owner. It is now read-only.

Fix Bug 1402312 - Implement dark theme #4044

Merged
merged 2 commits into from Mar 28, 2018
Merged

Conversation

@rlr
Copy link
Contributor

@rlr rlr commented Mar 19, 2018

r? @Mardak / @k88hudson

In action:

Screenshots:
screen shot 2018-03-19 at 15 56 29-fullpage

screen shot 2018-03-19 at 3 57 22 pm

screen shot 2018-03-19 at 3 57 47 pm

screen shot 2018-03-19 at 4 01 24 pm

@rlr rlr requested review from Mardak and k88hudson Mar 19, 2018
}

// Jump through some hoops to check if the current theme has light or dark
// text. If light, then we enable our dark (background) theme.

This comment has been minimized.


const THEME_UPDATE_EVENT = "lightweight-theme-styling-update";

this.ThemeFeed = class ThemeFeed {

This comment has been minimized.

@rlr

rlr Mar 19, 2018
Author Contributor

This feed will be obsolete or need to change once https://bugzilla.mozilla.org/show_bug.cgi?id=1444459 is implemented and lands.

updateTheme(data) {
if (data && data.window) {
// We only update newtab theme if the theme activated isn't window specific.
// We'll be able to do better in the future: see Bug 1444459

This comment has been minimized.

@rlr

rlr Mar 19, 2018
Author Contributor

Fun fact, different windows can be assigned different themes via web extensions, for example. Also private browsing. Out of scope for now :)

@rlr rlr force-pushed the rlr:Bug1402312/theming branch 2 times, most recently from 90f145b to 1feae14 Mar 23, 2018
@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 23, 2018

Does anybody have some time to look at this today or early next week? @sarracini @Mardak @k88hudson @piatra ? Please 😄

@Mardak
Copy link
Member

@Mardak Mardak commented Mar 23, 2018

Looking!

--newtab-contextmenu-button-color: $white;
--newtab-modal-color: $white;
--newtab-overlay-color: $grey-20;
--newtab-searchbox-color: $grey-10;

This comment has been minimized.

@Mardak

Mardak Mar 23, 2018
Member

nit: --newtab-searchbox-background-color as there's a bunch of colors ;)

Also, could you make this $white here to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1447752 :)


&:active {
background-color: $background-primary;
background-color: $grey-20;

This comment has been minimized.

@Mardak

Mardak Mar 23, 2018
Member

This should be grey-20 when clicking always?

This comment has been minimized.

@rlr

rlr Mar 23, 2018
Author Contributor

This is the hover on the prefs gear icon. For overall consistency, this should probably match --newtab-section-active-color so I should rename that.

--newtab-section-active-contextmenu-color: $white;
--newtab-topsites-label-color: $grey-10-80;
--newtab-card-background-color: $grey-90;
--newtab-inner-box-shadow-color: $grey-60;

This comment has been minimized.

@Mardak

Mardak Mar 23, 2018
Member

I'm still looking through this, but are there any conceptually related uses of a color that are used for more than one variable? I don't think I spotted any yet, but for example if --newtab-text-primary-color and --newtab-textbox-color were the same, there could be a local scss $var to share the two. But then, why would we have two different css --var? (I suppose the css var could exist if say dark shared a color but light had different colors -- but then again, why are they not consistent between the two themes?)

Copy link
Member

@Mardak Mardak left a comment

Could you generally describe your overall process of converting? E.g., the scss $alias over to css --var?

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 23, 2018

Basically, I went through every color spec'd out in the dark theme spec that needed to be changed and started breaking them out into variables. And then I tried to consolidate into as few variables as possible in some cases I had to get UX (Amy) to nod some changes/compromises to save a variable and keep more generic. But it's possible we can reduce it more?

See https://mozilla.invisionapp.com/share/H4FW3RYFXVK#/screens/279681634_New_Tab_Dark_Theme_Spec and:

dark theme spec - outstanding items

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 23, 2018

Afterwards, I wondered if I should've kept all the scss $vars and just make them point to var(--vars). I could switch back if it makes it easier to read or maintain.

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 23, 2018

Also, there are cases that two variables are the same in the default theme but different in the dark theme, and vice-versa.

--newtab-button-secondary-color: inherit;
--newtab-contextmenu-button-color: $white;
--newtab-modal-color: $white;
--newtab-overlay-color: $grey-20;

This comment has been minimized.

@rlr

rlr Mar 23, 2018
Author Contributor

I guess up to here, the variables are pretty generic. And all the ones below are pretty specific to different sections.

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

At least in reviewing, having these variables sorted would have been much easier to track things down. I suppose potentially you could have 2 list of variables separated by a newline with a comment each sorted for more generic vs section specific.

--newtab-inner-box-shadow-color: $grey-60;
}

// scss variables related to the theme.

This comment has been minimized.

@rlr

rlr Mar 23, 2018
Author Contributor

I moved these variables below into this file because some depend on the CSS variables defined above. But I guess they can go back to _variables.scss even though that's imported first because the vars don't get evaluated until runtime anyway.

$border-secondary: 1px solid var(--newtab-border-secondary-color);
$input-primary: $blue-60;
$input-secondary: transparent;
$shadow-primary: 0 0 0 5px $grey-30;

This comment has been minimized.

@Mardak

Mardak Mar 23, 2018
Member

I would have expected any shade of black to be different for light vs dark. Default background is $grey-10, so this $grey-30 shadow is 20 degrees away. To maintain the same amount of contrast on the dark theme's $grey-80 background, I would have expected $grey-60 for the shadow:

60 screen shot 2018-03-23 at 4 40 01 pm

30 screen shot 2018-03-23 at 4 40 22 pm

@bryanbell ?

This comment has been minimized.

@rlr

rlr Mar 23, 2018
Author Contributor

ah, Amy (is she on github?) said the box shadow on hover could stay the same.

screen shot 2018-03-23 at 7 48 51 pm

This comment has been minimized.

@Mardak

Mardak Mar 23, 2018
Member

bryan said "Top is better by far" with +1 from aaron

Copy link
Member

@Mardak Mardak left a comment

Gotta run, but here's some comments for now. Also, placeholders disappear! ;)

screen shot 2018-03-23 at 5 49 24 pm

--newtab-text-secondary-color: $grey-10-40;
--newtab-text-conditional-color: $grey-10;
--newtab-link-primary-color: $blue-40;
--newtab-link-secondary-color: $blue-40;

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

The "secondary" link color is currently used for Pocket stuff, and I'm pretty sure it's teal for default theme specifically because Pocket uses teal for its links. Should get design to confirm that it was intentional to make Pocket links not teal anymore. And if so, why keep it teal for default theme?

text-decoration: none;

&:hover {
color: $link-secondary;

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

Removing the :hover -secondary is right, so we can get rid of this overriding color too:

&:hover {
text-decoration: underline;
color: $blue-60;

I seem to recall design wanting links to underline on hover.. but I guess leave it this way for now.

@@ -22,12 +21,7 @@ h2 {
}

a {
color: $link-primary;

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

Why did this need to move into Base? I suppose technically there could be links under body that aren't under .outer-wrapper but there's other things in this file that are using var()…?

This comment has been minimized.

@rlr

rlr Mar 26, 2018
Author Contributor

I moved to .outer-wrapper because that's the first (topmost?) node that we control from react. I can't (cleanly) set classes or css vars to body or anything above .outer-wrapper. A positive side effect of moving the themed stuff to be sandboxed in .outer-wrapper is that this doesn't break snippets styles.

This comment has been minimized.

@Mardak

Mardak Mar 26, 2018
Member

I haven't actually used css variables, but looking at m-c usage/declarations, they seem to typically be under a :root { --var: … } selector.

@k88hudson do you have any preference or know of desired usage patterns?

This comment has been minimized.

@Mardak

Mardak Mar 26, 2018
Member

mconley points out that there is some movement to more appropriately scope css variables for performance reasons, e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=1435122

@@ -84,7 +78,7 @@ a {
padding: 15px 25px 0;

button {
background-color: $input-secondary;
background: var(--newtab-button-secondary-color);

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

Was there a need to make this background and not more scoped to background-color? It would probably be better consistency / expectation to use background-color wherever we use a var(…color)

This comment has been minimized.

@rlr

rlr Mar 26, 2018
Author Contributor

no need, just saving bytes 😝 . kind of my personal preference but dont really care either way.

@@ -1,5 +1,5 @@
.context-menu {
background: $background-primary;
background: var(--newtab-background-color);
border-radius: $context-menu-border-radius;
box-shadow: $context-menu-shadow;

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

This context menu box-shadow / border should probably be a themed value. Not entirely sure what it should be.. At least the second 1px shadow should be something closer to --newtab-border-primary-color.

--newtab-border-primary-color screen shot 2018-03-23 at 5 46 47 pm

current shadow screen shot 2018-03-23 at 5 42 44 pm

@@ -22,9 +21,11 @@
width: 100%;

input {
border: 0;
background: var(--newtab-searchbox-background-color);
border: solid 1px var(--newtab-searchbox-border-color);
border-radius: $search-border-radius;
box-shadow: $shadow-secondary, 0 0 0 1px $black-15;

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

There's various blacks here that probably need to be themed. E.g., search button hover, etc.

@@ -0,0 +1,80 @@
// Default theme
.outer-wrapper {

This comment has been minimized.

@Mardak

Mardak Mar 24, 2018
Member

We can set :not(.dark-theme) to avoid having both styles apply with the latter overriding for dark theme. Not sure if there's a good way to make sure both lists define the same variables though…

This comment has been minimized.

@rlr

rlr Mar 26, 2018
Author Contributor

is there a performance gain to doing that? or just a little less clutter in dev tools inspector?

This comment has been minimized.

@Mardak

Mardak Mar 26, 2018
Member

I'm not sure actually ;) Yeah I noticed it from devtools style inspector. I would assume it's quite implementation detail specific if it's more "expensive" to track multiple rules that apply and get overridden vs doing a :not selector.

@k88hudson any thoughts here?

This comment has been minimized.

@Mardak

Mardak Mar 26, 2018
Member

John-Galt seems to suggest it probably won't make much of a difference performance-wise. So for developer / QA checking, I think we can stick without :not as the default theme vars will always be set, so if we accidentally forget to set it in .dark, it should be more obvious when a light theme's color shows up incorrectly.

This comment has been minimized.

@rlr

rlr Mar 28, 2018
Author Contributor

A problem with having the variables under .outer-wrapper is that snippets/onboarding won't be able to see them because they live outside of this in the DOM tree. So, we probably wan't to assign them to :root at that point, but then we will need a new way to override them for each theme because we can't set a .dark-theme class to :root can we? I guess we can cross that bridge when we get there. Maybe https://bugzilla.mozilla.org/show_bug.cgi?id=1444459 will be fixed soonish and we can just wait for that.

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 26, 2018

I just got off vidyo with @bryanbell @aaronrbenson +Amy. Those were the changes we came up with ^^ 0253625

  • lighter teal for better contrast copy pasted off pocket website
  • darker separators for context menu
  • leave the lack of visible border and drop shadow as is on context menu
  • no visible hover state for search box (this is consistent with awesome bar in dark theme)
  • themeify the searchbox focus outline. use blue-40 in dark theme
  • tweaked placeholder and empty section borders

And they want it in their hands ASAP. I guess we can further tweak it in production^Wnightly if necessary.

@nt1m
Copy link
Contributor

@nt1m nt1m commented Mar 27, 2018

@Mardak I was talking about the main browser searchbar (hidden by default), not the one in about:newtab.

@Mardak
Copy link
Member

@Mardak Mardak commented Mar 28, 2018

Oh, I see. @rlr, activity stream is the only consumer of contentSearchUI now, so most likely we'll need to update it. Or more likely set the appropriate styling for default/dark from our css.

@@ -84,3 +85,8 @@
transform: translateY(2px);
}
}

// The search dropdown isn't themed (yet), so we need to keep dark text.
#searchSuggestionTable { // sass-lint:disable-line no-ids

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

There's already a .contentSearchSuggestionTable just a few lines higher that we can reuse.

@@ -79,7 +80,25 @@
   }
+}
 
+@at-root {
   // Adjust the style of the contentSearchUI-generated table
-  .contentSearchSuggestionTable { // sass-lint:disable-line class-name-format
+  // sass-lint:disable-block class-name-format
+  .contentSearchSuggestionTable {
+    background-color: var(--newtab-searchbox-background-color);
     border: 0;
-    transform: translateY(2px);
+    transform: translateY(3px);
+
+    .contentSearchHeader {
+      background-color: var(--newtab-background-color);
+      color: var(--newtab-text-secondary-color);
+    }
+
+    .contentSearchHeader,
+    .contentSearchSuggestionsList {
+      border-color: var(--newtab-border-primary-color);
+    }
+
+    .contentSearchSearchWithHeaderSearchText {
+      color: var(--newtab-text-primary-color);
+    }
   }

screen shot 2018-03-27 at 9 30 15 pm
screen shot 2018-03-27 at 9 30 24 pm

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

Oh, and I wonder if we should name the --vars as --newtab-search-… (no "box") ?

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

(I used @at-root mostly to create a new block for the sass disable.)

This comment has been minimized.

@rlr

rlr Mar 28, 2018
Author Contributor

awesome, thanks!. and --newtab-search- sounds good. fixing

Copy link
Member

@Mardak Mardak left a comment

We'll want to style the search table with css vars. Also cleaning up input-primary/secondary scss and css var conflicts/discrepancy.

I suppose design wants to try things out sooner, so various questions, e.g., context menu text color matching icons, should be tracked and maybe fixed in a followup?

@@ -93,19 +99,17 @@ main {
background-color: transparent;
border: 0;
cursor: pointer;
fill: $fill-secondary;
fill: var(--newtab-icon-primary-color);

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

This changes the color for default theme, but I guess this is fine (going from grey-90-60 to grey-90-80)

}

&:hover,
&:focus,
&:active {

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

Is it intended to have no differentiation between hovering vs actively clicking the gear? I believe the previous hover was to also help it show up against the sidebar's background, but now we only can open, so probably a reasonable time to change the expected styling.

This comment has been minimized.

@rlr

rlr Mar 28, 2018
Author Contributor

currently, when you click the background goes away. I guess we can keep that behavior. I dont think it really makes a difference either way since it's behavior is more like a link than a button.

--newtab-button-secondary-color: inherit;
--newtab-contextmenu-button-color: $white;
--newtab-modal-color: $white;
--newtab-overlay-color: $grey-20;

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

At least in reviewing, having these variables sorted would have been much easier to track things down. I suppose potentially you could have 2 list of variables separated by a newline with a comment each sorted for more generic vs section specific.

@@ -99,13 +93,14 @@ a {
}

&.dismiss {
background-color: $input-secondary;

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

Sorry for the earlier comment to switch this away from transparent. After looking more at the whole patch, I think we should just get rid of $input-secondary as it's confusing to also have --newtab-button-secondary-color. Looks like this .dismiss is a special case styling for the manual migration "link" button.

@@ -20,7 +20,7 @@
width: 100%;

&.separator {
border-bottom: 1px solid $context-menu-border-color;
border-bottom: 1px solid var(--newtab-border-secondary-color);

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

This changed from $black-20 (20% opacity) to $grey-30 (no transparency). Looks pretty close, so probably fine.

This border value ends up being just $border-secondary now.

this._prefs.set("prerender", PrerenderData.arePrefsValid(pref => this._prefs.get(pref), indexedDBPrefs));
const prefsAreValid = PrerenderData.arePrefsValid(pref => this._prefs.get(pref), indexedDBPrefs);
const themeIsDefault = (theme || this.store.getState().Theme).className === INITIAL_STATE.Theme.className;
this._prefs.set("prerender", prefsAreValid && themeIsDefault);

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

Maybe an alternative to invalidating prerendering or extra prerendering dark theme.. What if we used localStorage or something in the page to add .dark-theme to <body> as the page loads? The storage gets updated when Theme.className arrives so the next load of the page should synchronously render the appropriate background color. Although this would prevent different themes per window…

I guess we'll just keep the invalidation for now…

Services.obs.addObserver(this, THEME_UPDATE_EVENT);

let temp = {LightweightThemeManager};
ChromeUtils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

This seems odd… I guess for injecting custom LightweightThemeManager for testing? That's what the injector is for? Although in this case, we would just override the global if this was imported normally?

This comment has been minimized.

@rlr

rlr Mar 28, 2018
Author Contributor

I was assuming whoever did this knew what they were doing: https://searchfox.org/mozilla-central/source/toolkit/modules/LightweightThemeConsumer.jsm#29-30

But I could make it global if there is no reason to have done that.

beforeEach(() => {
globals = new GlobalOverrider();
sandbox = globals.sandbox;
({ThemeFeed, THEME_UPDATE_EVENT} = injector({}));

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

If nothing is being injected, why not just import the module normally?

This comment has been minimized.

@rlr

rlr Mar 28, 2018
Author Contributor

In case you asked me to make LightweightThemeManager global? 😉

@@ -1,5 +1,5 @@
.context-menu {
background: $background-primary;
background: var(--newtab-background-color);

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

Do we also need to set the text color for the context menu items? Dark theme doesn't have icons that match up with the text color anymore.. unless it's hovered:
image
image

padding-left: 3px;
}

button {
background: $grey-10;
background: $input-secondary;

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

Looks like this should be var(--newtab-button-secondary-color) instead of transparent
image
image

This comment has been minimized.

@Mardak

Mardak Mar 29, 2018
Member

@rlr looks like this was skipped? The disclaimer's button is now just transparent instead of a var

@Mardak
Copy link
Member

@Mardak Mardak commented Mar 28, 2018

I'm guessing this missing background is different from what kate pointed out earlier as I think you fixed that. Scrolling down the page shows <body>'s white background:
screen shot 2018-03-28 at 12 59 27 am

Oh, and onboarding stuff stuff stuff…

@Mardak Mardak assigned rlr and unassigned Mardak Mar 28, 2018
@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 28, 2018

I guess I'll have to fix merge conflicts if this is ready to land before pull #4058

--newtab-search-icon-color: $grey-90-40;
--newtab-section-header-text-color: $grey-50;
--newtab-section-navigation-text-color: $grey-50;
--newtab-section-navigation-link-color: $blue-40;

This comment has been minimized.

@rlr

rlr Mar 28, 2018
Author Contributor

this was missing a dark theme value. but it was actually unused! removing

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 28, 2018

Snippets/onboarding was out of scope for this I thought. @k88hudson had mentioned something something or the other.

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 28, 2018

border: 0;
padding: 0;
text-decoration: underline;
}

&.done {
background: $input-primary;
background-color: var(--newtab-button-primary-color);
border: solid 1px $blue-60;

This comment has been minimized.

@Mardak

Mardak Mar 28, 2018
Member

not sure why I didn't use $input-primary instead of this $blue-60 for border.. but it should probably just be var(--newtab-button-primary-color) now

@Mardak
Copy link
Member

@Mardak Mardak commented Mar 28, 2018

From the feedback on slack, given that we'll make all dark's blues blue-40 and default's blue-60, should we have a different var name:

+  --newtab-button-primary-color: $blue-60;
+  --newtab-link-primary-color: $blue-60;
+  --newtab-search-focus-border-color: $blue-50;

+  --newtab-button-primary-color: $blue-60;
+  --newtab-link-primary-color: $blue-40;
+  --newtab-search-focus-border-color: $blue-40;
@Mardak
Copy link
Member

@Mardak Mardak commented Mar 28, 2018

Although keeping individual css variables allows them to be individually overridden by a theme if desired…

@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 28, 2018

Having one var for all the blues sounds compelling. But yeah maybe they should remain separate because I can't think a name for a variable that combines them. --newtab-blue? but then what if I wanted pink 😆 . --newtab-action-color 🤔

@Mardak
Mardak approved these changes Mar 28, 2018
Copy link
Member

@Mardak Mardak left a comment

I guess good enough for now! I merged #4058, so this might still need to be rebased but hopefully easier?

@Mardak
Copy link
Member

@Mardak Mardak commented Mar 28, 2018

--newtab-interaction-primary-color ? ;)

@rlr rlr force-pushed the rlr:Bug1402312/theming branch from f9c6f0c to 20bfa69 Mar 28, 2018
@rlr rlr force-pushed the rlr:Bug1402312/theming branch from 20bfa69 to bbb3a6d Mar 28, 2018
@rlr rlr merged commit bbb3a6d into mozilla:master Mar 28, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rlr
Copy link
Contributor Author

@rlr rlr commented Mar 28, 2018

I guess we can still get stingier with variables when we start to expose them to theming api.

const {initialized} = App;
const prefs = props.Prefs.values;

const shouldBeFixedToTop = PrerenderData.arePrefsValid(name => prefs[name]);

const outerClassName = `outer-wrapper${shouldBeFixedToTop ? " fixed-to-top" : ""} ${prefs.enableWideLayout ? "wide-layout-enabled" : "wide-layout-disabled"}`;
const outerClassName = `outer-wrapper ${Theme.className}${shouldBeFixedToTop ? " fixed-to-top" : ""} ${prefs.enableWideLayout ? "wide-layout-enabled" : "wide-layout-disabled"}`;

This comment has been minimized.

@Mardak

Mardak Mar 29, 2018
Member

This ends up causing all the prerendered files to have an extra space:

"outer-wrapper  fixed-to-top…"
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants