Bug 1858711 - Refactor lwtheme attributes to account for missing colors. r=dao,desktop-theme-reviewers

When a theme doesn't specify popup colors, it makes sense to follow the
system-preferred color scheme. Refactor theming attributes so that we
can distinguish this. Instead of a single [lwt-popup-brighttext], we get
lwt-popup="dark"/"light", and handle it appropriately.

Same for the other relevant attributes.

Refactor sidebar theming (which already did this tri-state thing via
lwt-sidebar and lwt-sidebar-brighttext) to do the same using a single
lwt-sidebar attribute, and update the docs.

Differential Revision: https://phabricator.services.mozilla.com/D190919

browser/base/content/browser.css

@@ -385,11 +385,13 @@ toolbarpaletteitem {
   }
   toolbaritem:is([overflowedItem="true"], [cui-areatype="panel"]) > & {
     list-style-image: var(--webextension-menupanel-image, inherit);
-
-    :root[lwt-popup-brighttext] & {
+    /* TODO: This feels a bit odd, why do we have three images? It feels we
+     * should probably have only two (light/dark), and choose based on
+     * prefers-color-scheme + lwt-popup */
+    :root[lwt-popup="dark"] & {
       list-style-image: var(--webextension-menupanel-image-light, inherit);
     }
-    :root:not([lwt-popup-brighttext]) &:-moz-lwtheme {
+    :root[lwt-popup="light"] & {
       list-style-image: var(--webextension-menupanel-image-dark, inherit);
     }
   }

browser/base/content/contentTheme.js

@@ -97,18 +97,15 @@
         processColor(rgbaChannels, element) {
           if (!rgbaChannels) {
             element.removeAttribute("lwt-sidebar");
-            element.removeAttribute("lwt-sidebar-brighttext");
             return null;
           }
 
-          element.setAttribute("lwt-sidebar", "true");
+          // TODO(emilio): Can we share this code somehow with LightWeightThemeConsumer?
           const { r, g, b, a } = rgbaChannels;
-          if (!_isTextColorDark(r, g, b)) {
-            element.setAttribute("lwt-sidebar-brighttext", "true");
-          } else {
-            element.removeAttribute("lwt-sidebar-brighttext");
-          }
-
+          element.setAttribute(
+            "lwt-sidebar",
+            _isTextColorDark(r, g, b) ? "light" : "dark"
+          );
           return `rgba(${r}, ${g}, ${b}, ${a})`;
         },
       },

browser/themes/ThemeVariableMap.sys.mjs

@@ -126,14 +126,11 @@ export const ThemeVariableMap = [
     "--sidebar-background-color",
     {
       lwtProperty: "sidebar",
-      optionalElementID: "sidebar-box",
-      processColor(rgbaChannels, element) {
+      processColor(rgbaChannels) {
         if (!rgbaChannels) {
-          element.removeAttribute("lwt-sidebar");
           return null;
         }
         const { r, g, b } = rgbaChannels;
-        element.setAttribute("lwt-sidebar", "true");
         // Drop alpha channel
         return `rgb(${r}, ${g}, ${b})`;
       },

browser/themes/linux/browser.css

@@ -7,6 +7,10 @@
 
 @namespace html url("http://www.w3.org/1999/xhtml");
 
+:root {
+  --sidebar-border-color: ThreeDShadow;
+}
+
 /**
  * We intentionally do not include browser-custom-colors.css,
  * instead choosing to fall back to system colours and transparencies
@@ -127,11 +131,6 @@ menuitem.bookmark-item {
 
 /* Content area */
 
-
-#browser {
-  --sidebar-border-color: ThreeDShadow;
-}
-
 .sidebar-splitter {
   appearance: none;
   width: 6px;

browser/themes/osx/browser.css

@@ -11,6 +11,9 @@
   appearance: none;
 
   --arrowpanel-field-background: light-dark(rgba(249, 249, 250, .3), rgba(12, 12, 13, .3));
+
+  --sidebar-border-color: hsla(240, 5%, 5%, .1);
+  --sidebar-background-color: -moz-mac-source-list;
 }
 
 #browser,
@@ -229,20 +232,15 @@ moz-input-box > menupopup .context-menu-add-engine > .menu-iconic-left {
 
 /* ----- SIDEBAR ELEMENTS ----- */
 
-#browser {
-  --sidebar-border-color: hsla(240, 5%, 5%, .1);
-}
-
 #sidebar-box {
   /* Default font size is 11px on mac, so this is 12px */
   font-size: 1.0909rem;
-  --sidebar-background-color: -moz-mac-source-list;
 
   /* Give the sidebar a vibrant appearance. Only do this when no lwtheme sidebar
-  * rules are in use. Vibrant appearance values only work if there is no
-  * background-color rendered behind the element. If the active theme has sidebar
-  * rules, we want to show the theme's background-color in the sidebar. */
-  &:not([lwt-sidebar]) {
+   * rules are in use. Vibrant appearance values only work if there is no
+   * background-color rendered behind the element. If the active theme has sidebar
+   * rules, we want to show the theme's background-color in the sidebar. */
+  :root:not([lwt-sidebar]) & {
     appearance: auto;
     -moz-default-appearance: -moz-mac-source-list;
     -moz-font-smoothing-background-color: -moz-mac-source-list;

browser/themes/shared/places/sidebar.css

@@ -12,37 +12,45 @@
 .sidebar-panel[lwt-sidebar] {
   background-color: var(--lwt-sidebar-background-color);
   color: var(--lwt-sidebar-text-color);
-  scrollbar-color: rgba(204,204,204,.5) rgba(230,230,235,.5);
   color-scheme: light;
+
+  scrollbar-color:
+    light-dark(
+      rgba(204,204,204,.5),
+      rgba(249,249,250,.4)
+    )
+    light-dark(
+      rgba(230,230,235,.5),
+      rgba(20,20,25,.3)
+    );
 }
 
-.sidebar-panel[lwt-sidebar-brighttext] {
-  scrollbar-color: rgba(249,249,250,.4) rgba(20,20,25,.3);
+.sidebar-panel[lwt-sidebar="dark"] {
   color-scheme: dark;
 }
 
 .sidebar-panel[lwt-sidebar] .sidebar-placesTreechildren::-moz-tree-row(selected) {
-  background-color: hsla(0,0%,80%,.3);
+  background-color: light-dark(hsla(0,0%,80%,.3), rgba(249,249,250,.1));
 }
 
-.sidebar-panel[lwt-sidebar-brighttext] .sidebar-placesTreechildren::-moz-tree-row(selected) {
-  background-color: rgba(249,249,250,.1);
+.sidebar-panel[lwt-sidebar="dark"] .sidebar-placesTreechildren {
+  &::-moz-tree-image(selected),
+  &::-moz-tree-twisty(selected),
+  &::-moz-tree-cell-text(selected) {
+    color: var(--lwt-sidebar-text-color);
+  }
 }
 
-.sidebar-panel[lwt-sidebar-brighttext] .sidebar-placesTreechildren::-moz-tree-image(selected),
-.sidebar-panel[lwt-sidebar-brighttext] .sidebar-placesTreechildren::-moz-tree-twisty(selected),
-.sidebar-panel[lwt-sidebar-brighttext] .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
-  color: var(--lwt-sidebar-text-color);
-}
+.sidebar-panel[lwt-sidebar-highlight] .sidebar-placesTreechildren {
+  &::-moz-tree-row(selected,focus) {
+    background-color: var(--lwt-sidebar-highlight-background-color);
+  }
 
-.sidebar-panel[lwt-sidebar-highlight] .sidebar-placesTreechildren::-moz-tree-row(selected,focus) {
-  background-color: var(--lwt-sidebar-highlight-background-color);
-}
-
-.sidebar-panel[lwt-sidebar-highlight] .sidebar-placesTreechildren::-moz-tree-image(selected, focus),
-.sidebar-panel[lwt-sidebar-highlight] .sidebar-placesTreechildren::-moz-tree-twisty(selected, focus),
-.sidebar-panel[lwt-sidebar-highlight] .sidebar-placesTreechildren::-moz-tree-cell-text(selected, focus) {
-  color: var(--lwt-sidebar-highlight-text-color, var(--lwt-sidebar-text-color, var(--sidebar-text-color)));
+  &::-moz-tree-image(selected, focus),
+  &::-moz-tree-twisty(selected, focus),
+  &::-moz-tree-cell-text(selected, focus) {
+    color: var(--lwt-sidebar-highlight-text-color, var(--lwt-sidebar-text-color, var(--sidebar-text-color)));
+  }
 }
 
 /* Sidebar tree */

browser/themes/shared/searchbar.css

@@ -31,7 +31,7 @@
 }
 
 /* Make the contrast stronger in dark mode */
-:root[lwt-toolbar-field-focus-brighttext] .search-panel-header > label {
+:root[lwt-toolbar-field-focus="dark"] .search-panel-header > label {
   opacity: 1;
 }
 

browser/themes/shared/sidebar.css

@@ -4,12 +4,22 @@
 
 @namespace html url("http://www.w3.org/1999/xhtml");
 
-#sidebar-box {
+:root {
   --sidebar-background-color: Field;
   --sidebar-text-color: FieldText;
+}
+
+#sidebar-box {
   background-color: var(--sidebar-background-color);
   color: var(--sidebar-text-color);
   text-shadow: none;
+
+  :root[lwt-sidebar="light"] & {
+    color-scheme: light;
+  }
+  :root[lwt-sidebar="dark"] & {
+    color-scheme: dark;
+  }
 }
 
 #sidebar-header {

browser/themes/shared/syncedtabs/sidebar.css

@@ -290,22 +290,27 @@ body {
 :root[lwt-sidebar] {
   background-color: var(--lwt-sidebar-background-color);
   color: var(--lwt-sidebar-text-color);
-  scrollbar-color: rgba(204,204,204,.5) rgba(230,230,235,.5);
+  scrollbar-color:
+    light-dark(
+      rgba(204,204,204,.5),
+      rgba(249,249,250,.4)
+    )
+    light-dark(
+      rgba(230,230,235,.5),
+      rgba(20,20,25,.3)
+    );
+  color-scheme: light;
 }
 
-:root[lwt-sidebar-brighttext] {
-  scrollbar-color: rgba(249,249,250,.4) rgba(20,20,25,.3);
+:root[lwt-sidebar="dark"] {
+  color-scheme: dark;
 }
 
 :root[lwt-sidebar] .item.selected > .item-title-container {
-  background-color: hsla(0,0%,80%,.3);
+  background-color: light-dark(hsla(0,0%,80%,.3), rgba(249,249,250,.1));
   color: inherit;
 }
 
-:root[lwt-sidebar-brighttext] .item.selected > .item-title-container {
-  background-color: rgba(249,249,250,.1);
-}
-
 :root[lwt-sidebar-highlight] .item.selected:focus > .item-title-container {
   background-color: var(--lwt-sidebar-highlight-background-color);
   color: var(--lwt-sidebar-highlight-text-color);

browser/themes/shared/tabs.css

@@ -770,7 +770,8 @@ toolbar:not(#TabsToolbar) #firefox-view-button {
         url("chrome://browser/skin/tabbrowser/tab-loading@2x.png") 2x
       );
 
-      :root[lwt-popup-brighttext] &[progress]:not([selected]) {
+      /* FIXME: This should probably also apply in regular dark mode? */
+      :root[lwt-popup="dark"] &[progress]:not([selected]) {
         list-style-image: image-set(
           url("chrome://browser/skin/tabbrowser/tab-loading-inverted.png"),
           url("chrome://browser/skin/tabbrowser/tab-loading-inverted@2x.png") 2x

browser/themes/shared/urlbar-searchbar.css

@@ -20,11 +20,10 @@
 #search-container {
   padding-block: 4px;
   margin-inline: var(--urlbar-margin-inline);
-}
 
-:root[uidensity=touch] #urlbar-container,
-:root[uidensity=touch] #search-container {
-  padding-block: 5px;
+  :root[uidensity=touch] & {
+    padding-block: 5px;
+  }
 }
 
 #urlbar,
@@ -32,28 +31,25 @@
   min-height: var(--urlbar-min-height);
   text-shadow: none;
   color: var(--toolbar-field-color);
-}
-
-/**
- * System colors and widgets are set based on toolbar color. Since toolbar
- * fields can be styled differently from the toolbar, we need to use the
- * correct color scheme in toolbar fields.
- */
-#urlbar:-moz-lwtheme,
-#searchbar:-moz-lwtheme {
-  color-scheme: light;
-}
-
-:root[lwt-toolbar-field-brighttext] {
-  #urlbar:not([focused="true"]),
-  #searchbar:not(:focus-within) {
+  /**
+   * System colors and widgets are set based on toolbar color. Since toolbar
+   * fields can be styled differently from the toolbar, we need to use the
+   * correct color scheme in toolbar fields.
+   */
+  :root[lwt-toolbar-field="light"] & {
+    color-scheme: light;
+  }
+  :root[lwt-toolbar-field="dark"] & {
     color-scheme: dark;
   }
 }
 
-:root[lwt-toolbar-field-focus-brighttext] {
-  #urlbar[focused="true"],
-  #searchbar:focus-within {
+#urlbar[focused="true"],
+#searchbar:focus-within {
+  :root[lwt-toolbar-field-focus="light"] & {
+    color-scheme: light;
+  }
+  :root[lwt-toolbar-field-focus="dark"] & {
     color-scheme: dark;
   }
 }

browser/themes/shared/urlbarView.css

@@ -497,7 +497,7 @@
     pointer-events: none;
   }
 
-  :root[lwt-toolbar-field-focus-brighttext] &::before {
+  :root[lwt-toolbar-field-focus="dark"] &::before {
     /* Same as `.search-panel-header > label` in searchbar.css */
     opacity: 1;
   }

browser/themes/windows/browser.css

@@ -6,6 +6,10 @@
 @import url("chrome://browser/skin/contextmenu.css");
 @import url("chrome://browser/skin/browser-custom-colors.css");
 
+:root {
+  --sidebar-border-color: ThreeDLightShadow;
+}
+
 #menubar-items {
   flex-direction: column; /* for flex hack */
   justify-content: normal; /* align the menubar to the top also in customize mode */
@@ -344,12 +348,6 @@
   border-top: 1px solid ThreeDShadow;
 }
 
-/* Content area */
-
-#browser {
-  --sidebar-border-color: ThreeDLightShadow;
-}
-
 /* Tabstrip */
 
 #TabsToolbar {

docs/code-quality/coding-style/css_guidelines.rst

@@ -503,14 +503,17 @@ Writing theme-friendly CSS
 -  Never write CSS specially for the built-in light/dark theme in
    ``compacttheme.css`` unless that CSS isn't supposed to affect
    WebExtension themes.
--  These selectors can be used to target dark areas:
+-  These selectors can be used to target themed areas, though in general it's
+   recommended to try to avoid them and use ``light-dark()`` to get the right
+   colors automatically:
 
-   -  ``:-moz-lwtheme-brighttext``: dark window frame.
-   -  ``:root[lwt-toolbar-field-brighttext]``: dark address bar and
-      searchbar.
-   -  ``:root[lwt-popup-brighttext]``: dark arrow panels and
-      autocomplete panels.
-   -  ``:root[lwt-sidebar-brighttext]``: dark sidebars.
+   - ``:root[lwt-toolbar-field="light/dark"]``: explicitly light or dark address bar and
+     searchbar.
+   - ``:root[lwt-toolbar-field-focus="light/dark"]``: explicitly light or dark address bar and
+     searchbar in the focused state.
+   - ``:root[lwt-popup="light/dark"]``: explicitly light or dark arrow panels
+     and autocomplete panels.
+   - ``:root[lwt-sidebar="light/dark"]``: explicitly light or dark sidebars.
 
 -  If you'd like a different shade of a themed area and no CSS variable
    is adequate, using colors with alpha transparency is usually a good

toolkit/components/extensions/test/browser/browser_ext_themes_sidebars.js

@@ -6,7 +6,7 @@
  * Test whether the selected browser has the sidebar theme applied
  *
  * @param {object} theme that is applied
- * @param {boolean} isBrightText whether the brighttext attribute should be set
+ * @param {boolean} isBrightText whether the text color is light
  */
 async function test_sidebar_theme(theme, isBrightText) {
   let extension = ExtensionTestUtils.loadExtension({
@@ -16,20 +16,21 @@ async function test_sidebar_theme(theme, isBrightText) {
   });
 
   const sidebarBox = document.getElementById("sidebar-box");
+  const browserRoot = document.documentElement;
   const content = SidebarUI.browser.contentWindow;
   const root = content.document.documentElement;
 
   ok(
-    !sidebarBox.hasAttribute("lwt-sidebar"),
-    "Sidebar box should not have lwt-sidebar attribute"
+    !browserRoot.hasAttribute("lwt-sidebar"),
+    "Browser should not have lwt-sidebar attribute"
   );
   ok(
     !root.hasAttribute("lwt-sidebar"),
-    "Sidebar should not have lwt-sidebar attribute"
+    "Root should not have lwt-sidebar attribute"
   );
   ok(
-    !root.hasAttribute("lwt-sidebar-brighttext"),
-    "Sidebar should not have lwt-sidebar-brighttext attribute"
+    !browserRoot.hasAttribute("lwt-sidebar-highlight"),
+    "Browser should not have lwt-sidebar-brighttext attribute"
   );
   ok(
     !root.hasAttribute("lwt-sidebar-highlight"),
@@ -80,23 +81,30 @@ async function test_sidebar_theme(theme, isBrightText) {
   const isCustomSidebar = !!theme.colors.sidebar_text;
 
   is(
-    sidebarBox.hasAttribute("lwt-sidebar"),
+    browserRoot.hasAttribute("lwt-sidebar"),
     isCustomSidebar,
-    `Sidebar box should${
-      !isCustomSidebar ? " not" : ""
-    } have lwt-sidebar attribute`
+    `Browser should${!isCustomSidebar ? " not" : ""} have lwt-sidebar attribute`
   );
   is(
     root.hasAttribute("lwt-sidebar"),
     isCustomSidebar,
     `Sidebar should${!isCustomSidebar ? " not" : ""} have lwt-sidebar attribute`
   );
+  if (isCustomSidebar) {
+    // Quite confusingly, getAttribute() on XUL elements for attributes that
+    // are not present has different behavior to HTML (empty string vs. null).
+    is(
+      root.getAttribute("lwt-sidebar"),
+      browserRoot.getAttribute("lwt-sidebar"),
+      `Sidebar lwt-sidebar attribute should match browser`
+    );
+  }
   is(
-    root.hasAttribute("lwt-sidebar-brighttext"),
+    browserRoot.getAttribute("lwt-sidebar") == "dark",
     isBrightText,
-    `Sidebar should${
+    `Browser should${
       !isBrightText ? " not" : ""
-    } have lwt-sidebar-brighttext attribute`
+    } have lwt-sidebar="dark" attribute`
   );
   is(
     root.hasAttribute("lwt-sidebar-highlight"),
@@ -126,7 +134,6 @@ async function test_sidebar_theme(theme, isBrightText) {
     "Sidebar background should be set."
   );
   is(rootCS.color, actualColor, "Sidebar text color should be set.");
-
   is(
     highlightCS.backgroundColor,
     actualHighlightBackground,
@@ -143,17 +150,13 @@ async function test_sidebar_theme(theme, isBrightText) {
   Services.ppmm.sharedData.flush();
 
   ok(
-    !sidebarBox.hasAttribute("lwt-sidebar"),
-    "Sidebar box should not have lwt-sidebar attribute"
+    !browserRoot.hasAttribute("lwt-sidebar"),
+    "Browser should not have lwt-sidebar attribute"
   );
   ok(
     !root.hasAttribute("lwt-sidebar"),
     "Sidebar should not have lwt-sidebar attribute"
   );
-  ok(
-    !root.hasAttribute("lwt-sidebar-brighttext"),
-    "Sidebar should not have lwt-sidebar-brighttext attribute"
-  );
   ok(
     !root.hasAttribute("lwt-sidebar-highlight"),
     "Sidebar should not have lwt-sidebar-highlight attribute"

toolkit/modules/LightweightThemeConsumer.sys.mjs

@@ -550,37 +550,32 @@ function _setDarkModeAttributes(doc, root, colors) {
     }
   }
 
-  if (
-    _determineIfColorPairIsDark(
+  const setAttribute = function (
+    attribute,
+    textPropertyName,
+    backgroundPropertyName
+  ) {
+    let dark = _determineIfColorPairIsDark(
       doc,
       colors,
-      "toolbar_field_text",
-      "toolbar_field"
-    )
-  ) {
-    root.setAttribute("lwt-toolbar-field-brighttext", "true");
-  } else {
-    root.removeAttribute("lwt-toolbar-field-brighttext");
-  }
+      textPropertyName,
+      backgroundPropertyName
+    );
+    if (dark === null) {
+      root.removeAttribute(attribute);
+    } else {
+      root.setAttribute(attribute, dark ? "dark" : "light");
+    }
+  };
 
-  if (
-    _determineIfColorPairIsDark(
-      doc,
-      colors,
-      "toolbar_field_text_focus",
-      "toolbar_field_focus"
-    )
-  ) {
-    root.setAttribute("lwt-toolbar-field-focus-brighttext", "true");
-  } else {
-    root.removeAttribute("lwt-toolbar-field-focus-brighttext");
-  }
-
-  if (_determineIfColorPairIsDark(doc, colors, "popup_text", "popup")) {
-    root.setAttribute("lwt-popup-brighttext", "true");
-  } else {
-    root.removeAttribute("lwt-popup-brighttext");
-  }
+  setAttribute("lwt-toolbar-field", "toolbar_field_text", "toolbar_field");
+  setAttribute(
+    "lwt-toolbar-field-focus",
+    "toolbar_field_text_focus",
+    "toolbar_field_focus"
+  );
+  setAttribute("lwt-popup", "popup_text", "popup");
+  setAttribute("lwt-sidebar", "sidebar_text", "sidebar");
 }
 
 /**
@@ -594,8 +589,8 @@ function _setDarkModeAttributes(doc, root, colors) {
  *   The key for the foreground element in `colors`.
  * @param {string} backgroundElementId
  *   The key for the background element in `colors`.
- * @returns {boolean} True if the element should be considered dark, false
- *   otherwise.
+ * @returns {boolean | null} True if the element should be considered dark, false
+ *   if light, null for preferred scheme.
  */
 function _determineIfColorPairIsDark(
   doc,
@@ -605,7 +600,7 @@ function _determineIfColorPairIsDark(
 ) {
   if (!colors[backgroundPropertyName] && !colors[textPropertyName]) {
     // Handles the system theme.
-    return false;
+    return null;
   }
 
   let color = _cssColorToRGBA(doc, colors[backgroundPropertyName]);
@@ -617,7 +612,7 @@ function _determineIfColorPairIsDark(
   if (!color) {
     // Handles the case where a theme only provides a background color and it is
     // semi-transparent.
-    return false;
+    return null;
   }
 
   return !_isColorDark(color.r, color.g, color.b);

toolkit/themes/shared/global-shared.css

@@ -79,11 +79,11 @@
 
 :root:-moz-lwtheme {
   toolbar,
-  panel {
+  &[lwt-popup="light"] panel {
     color-scheme: light;
   }
   toolbar[brighttext],
-  &[lwt-popup-brighttext] panel {
+  &[lwt-popup="dark"] panel {
     color-scheme: dark;
   }
 }