Dark theme #239

htwyford · 2018-05-09T19:03:53Z

Addresses #185. The onboarding in the sidebar and doorhanger now take on the dark theme if it's applied in Firefox.

One outstanding issue is that Firefox broadcasts a switch from the default to the dark theme, but not from the the dark theme to the default. This means the sidebar changes when the dark theme is applied but doesn't return to default without a browser restart. I figured this is a smallish edge case: a user would have to switch from default to dark while the onboarding window is open. This issue doesn't cause any breaking changes. Doesn't seem like it can be resolved until https://bugzilla.mozilla.org/show_bug.cgi?id=1435216 is addressed.

Alternatively, we could disable the listener. This would allow us to remove the management permission. The user would have to close and reopen the sidebar after switching to dark theme to see the changes.

ericawright

Some cursory comments:

~~The side bar seems to stay in dark theme mode until you close and open it again.~~
(edit) both the sidebar and popup stay in dark mode until opened again - though will switch dynamically from light to dark

nit:

there is a white flash on the sidebar when it is in dark mode.

run npm run lint to find some code style fixes

ericawright · 2018-05-09T19:12:58Z

addon/theming.js

@@ -0,0 +1,111 @@
+var updateBackground = function(el, color) {


use let and const instead of var

ericawright · 2018-05-09T19:50:56Z

addon/popup.js

+  handleThemes(isPopup = true); 
+
+  // // watch for switch to dark theme.
+  // // BUG: No event transmitted when switching back to default


remove this comment, since we aren't using themes other than dark/light

ericawright · 2018-05-09T19:57:09Z

Instead of using isPopup, can't we just have one listener and update both the popup and the sidebar when triggered?

htwyford · 2018-05-09T19:59:16Z

Regarding isPopup, the sidebar and popup are blind to each other, and the popup is only updated when it's opened. If you attempt to update the sidebar variables when the popup needs to be drawn, there's an error and both fail, and vice versa.

ericawright · 2018-05-09T20:17:17Z

addon/theming.js

+      (theme.colors.popup_highlight || "#ededf0") + "!important }";
+    let style = document.createElement("style");
+
+    if (style.styleSheet) {


does this ever evaluate to true?

ericawright · 2018-05-09T20:23:34Z

addon/theming.js

+  if (theme.colors) {
+    let panel = document.getElementById("panel");
+    let tabs = document.getElementsByClassName("tab");
+    let feedback = document.getElementsByClassName("feedback-button")[0];


document.querySelector('.feedback-button') will get you only the first element it finds with that selector

ericawright · 2018-05-09T20:26:52Z

addon/theming.js

+};
+
+function updateSidebarStyle() {
+  if (theme.colors) {


can these styles be applied with css instead? select a parent element el.classList.add('dark-theme') then style all the children within it dark?
We talked about how the border and carat of the panel can only be changed with JS, but in all other cases css is better

htwyford · 2018-05-09T21:38:47Z

The selection of what elements to dark theme is more specific than "all children of root" to avoid writing too many exceptions in the CSS.

ericawright

Can the "Recent Tabs" and "Current Tabs" labels be a lighter color in dark mode?

Can you use variables in css, it doesn't look like they've been used yet, but might as well start them.
ex:
root {
--dark-theme-background-color: #6d6d6f;
}

ericawright · 2018-05-10T13:47:42Z

addon/sidebar.js

@@ -2,6 +2,25 @@ function element(selector) {
  return document.querySelector(selector);
 }

+function applyDarkTheme() {
+  document.body.style.background = "#4a4a4f";
+  document.body.style.color = "#fff";


Do you know why this needs to be applied when the sidebar is triggered? I tried removing it and the popup styling was affected

ericawright · 2018-05-10T13:48:10Z

addon/sidebar.js

+}
+
+async function checkForDark() {
+  browser.management.getAll().then((extensions) => {


I wish there was a better way to listen for this event that didn't require us to request more permissions...

ericawright · 2018-05-10T13:59:43Z

addon/popup.css

+
+#panel.dark-theme,
+#panel.dark-theme .tab {
+  background: #4a4a4f !important;


I think you can remove most, if not all, of the !important

ericawright · 2018-05-10T14:03:46Z

addon/sidebar.css

+.page.dark-theme button:hover,
+.page.dark-theme button:focus,
+.page.dark-theme .graphic__shadow {
+  background: #6d6d6f;


I like the effect on the shadow, but it creates a strange highlighter effect on the links.

ericawright · 2018-05-10T14:06:05Z

addon/sidebar.js

+function applyDarkTheme() {
+  document.body.style.background = "#4a4a4f";
+  document.body.style.color = "#fff";
+  document.querySelector(".page").classList.add("dark-theme");


looks like there's already a utility function written above on this page for this, we should use it.

ianb · 2018-05-10T18:13:13Z

Is there anything with browser_style that would help us here? (A property mentioned on this page)

There's a possibility we could detect the theme using browser_style and getComputedStyle(). (Note that I don't know if browser_style even respects the dark theme, and I haven't tested any of this, it's just a hunch.)

There's also a possibility that if we don't specify things browser_style already gives us, then the dark theme would magically be applied? (That seems unlikely to me.)

htwyford · 2018-05-10T18:54:27Z

browser_style doesn't seem to respect dark theme. Searching through chrome://browser/content/extension.css doesn't turn up any references to any of the dark theme colours (or the works "dark", "night", "firefox-compact-dark", etc., for that matter).
I also tried removing specific references to browser_style as well, and it just defaults to the usual white look. I think something custom is needed, as least until browser_style starts broadcasting dark-theme changes.

ericawright · 2018-05-11T17:30:18Z

@htwyford this is ready right? @fzzzy or @ianb want to review again?

htwyford · 2018-05-11T17:41:40Z

It's ready!

fzzzy · 2018-05-14T14:57:25Z

@htwyford This looks good, but it has some conflicts now. Can you resolve them? Thanks!

although JS theming is required for the base of the popup. A listener is installed to update the sidebar on default -> dark changes.

fzzzy · 2018-05-14T18:41:54Z

Nice!

ericawright reviewed May 9, 2018

View reviewed changes

addon/theming.js Outdated

@@ -0,0 +1,111 @@

var updateBackground = function(el, color) {

Copy link

ericawright May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use let and const instead of var

ericawright reviewed May 9, 2018

View reviewed changes

ericawright reviewed May 10, 2018

View reviewed changes

htwyford force-pushed the dark-theme branch from 64c9a4e to 3c1ea79 Compare May 10, 2018 17:29

htwyford mentioned this pull request May 14, 2018

Adds a dismiss button to remove Recent tabs (#177) #248

Merged

htwyford force-pushed the dark-theme branch from 6e5364c to 3c1ea79 Compare May 14, 2018 17:48

New dark theme. Updated CSS and graphics. Most theming is CSS

6cebf0d

although JS theming is required for the base of the popup. A listener is installed to update the sidebar on default -> dark changes.

htwyford force-pushed the dark-theme branch from 3c1ea79 to 6cebf0d Compare May 14, 2018 17:51

fzzzy merged commit d99f763 into mozilla:master May 14, 2018

Softvision-CristinaBadescu mentioned this pull request May 15, 2018

The "Welcome" screen wrongly keeps a Dark background after the theme is changed to Light/Default #251

Open

jaredhirsch mentioned this pull request Aug 28, 2024

Side View “Welcome” screen and extension panel do not change color scheme based on the applied default Dark/Light themes #463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dark theme #239

Dark theme #239

htwyford commented May 9, 2018 •

edited

Loading

ericawright left a comment •

edited

Loading

ericawright May 9, 2018

ericawright May 9, 2018

ericawright commented May 9, 2018

htwyford commented May 9, 2018

ericawright May 9, 2018

ericawright May 9, 2018 •

edited

Loading

ericawright May 9, 2018 •

edited

Loading

htwyford commented May 9, 2018

ericawright left a comment

ericawright May 10, 2018

ericawright May 10, 2018

ericawright May 10, 2018

ericawright May 10, 2018

ericawright May 10, 2018

ianb commented May 10, 2018

htwyford commented May 10, 2018

ericawright commented May 11, 2018

htwyford commented May 11, 2018

fzzzy commented May 14, 2018

fzzzy commented May 14, 2018

		@@ -0,0 +1,111 @@
		var updateBackground = function(el, color) {

Dark theme #239

Dark theme #239

Conversation

htwyford commented May 9, 2018 • edited Loading

ericawright left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericawright commented May 9, 2018

htwyford commented May 9, 2018

Choose a reason for hiding this comment

ericawright May 9, 2018 • edited Loading

Choose a reason for hiding this comment

ericawright May 9, 2018 • edited Loading

Choose a reason for hiding this comment

htwyford commented May 9, 2018

ericawright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianb commented May 10, 2018

htwyford commented May 10, 2018

ericawright commented May 11, 2018

htwyford commented May 11, 2018

fzzzy commented May 14, 2018

fzzzy commented May 14, 2018

htwyford commented May 9, 2018 •

edited

Loading

ericawright left a comment •

edited

Loading

ericawright May 9, 2018 •

edited

Loading

ericawright May 9, 2018 •

edited

Loading