Bug 1640417 - improve reader mode font dropdown styling, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Sat, 30 May 2020 00:58:51 +0000
changeset 533095 9b06c0ec158c0e11f6cd05349a44e3ba0cd62a20
parent 533094 a5fe71693070d3fea7a60d09eef4452fddbb5704
child 533096 f65636dc2b12ab0480672a418adccc64219d8d7a
push id37462
push usermalexandru@mozilla.com
push dateSat, 30 May 2020 09:46:43 +0000
treeherdermozilla-central@8aaca63ec5c6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1640417
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1640417 - improve reader mode font dropdown styling, r=jaws This fixes the following issues: - hover/active colours now look better on dark theme (match main toolbar lwtheme styles) - radio-type items now have custom styling - radio-type items no longer use buttons, only input[type=radio] with a subsequent label - hover/active/selection styling for the radio items is improved - cleans up unused CSS variables - styling of the 'current value' boxes - removes display:none hr elements - uses classes for each 'row' container to simplify the CSS Differential Revision: https://phabricator.services.mozilla.com/D77535
toolkit/components/reader/AboutReader.jsm
toolkit/components/reader/content/aboutReader.html
toolkit/themes/shared/aboutReader.css
toolkit/themes/shared/narrate.css
--- a/toolkit/components/reader/AboutReader.jsm
+++ b/toolkit/components/reader/AboutReader.jsm
@@ -5,16 +5,19 @@
 "use strict";
 
 var EXPORTED_SYMBOLS = ["AboutReader"];
 
 const { ReaderMode } = ChromeUtils.import(
   "resource://gre/modules/ReaderMode.jsm"
 );
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
+const { AppConstants } = ChromeUtils.import(
+  "resource://gre/modules/AppConstants.jsm"
+);
 
 ChromeUtils.defineModuleGetter(
   this,
   "AsyncPrefs",
   "resource://gre/modules/AsyncPrefs.jsm"
 );
 ChromeUtils.defineModuleGetter(
   this,
@@ -55,16 +58,17 @@ var AboutReader = function(actor, articl
     win.location.href = "about:blank";
     return;
   }
 
   let doc = win.document;
   if (isAppLocaleRTL) {
     doc.dir = "rtl";
   }
+  doc.documentElement.setAttribute("platform", AppConstants.platform);
 
   this._actor = actor;
 
   this._docRef = Cu.getWeakReference(doc);
   this._winRef = Cu.getWeakReference(win);
   this._innerWindowId = win.windowUtils.currentInnerWindowID;
 
   this._article = null;
@@ -134,16 +138,17 @@ var AboutReader = function(actor, articl
   this._actor.sendAsyncMessage("Reader:OnSetup");
 
   let colorSchemeValues = JSON.parse(
     Services.prefs.getCharPref("reader.color_scheme.values")
   );
   let colorSchemeOptions = colorSchemeValues.map(value => {
     return {
       name: gStrings.GetStringFromName("aboutReader.colorScheme." + value),
+      groupName: "color-scheme",
       value,
       itemClass: value + "-button",
     };
   });
 
   let colorScheme = Services.prefs.getCharPref("reader.color_scheme");
   this._setupSegmentedButton(
     "color-scheme-buttons",
@@ -151,29 +156,29 @@ var AboutReader = function(actor, articl
     colorScheme,
     this._setColorSchemePref.bind(this)
   );
   this._setColorSchemePref(colorScheme);
 
   let styleButton = this._doc.querySelector(".style-button");
   this._setButtonTip(styleButton, "aboutReader.toolbar.typeControls");
 
-  let fontTypeSample = gStrings.GetStringFromName("aboutReader.fontTypeSample");
+  // See bug 1637089.
+  // let fontTypeSample = gStrings.GetStringFromName("aboutReader.fontTypeSample");
+
   let fontTypeOptions = [
     {
-      name: fontTypeSample,
-      description: gStrings.GetStringFromName(
-        "aboutReader.fontType.sans-serif"
-      ),
+      name: gStrings.GetStringFromName("aboutReader.fontType.sans-serif"),
+      groupName: "font-type",
       value: "sans-serif",
       itemClass: "sans-serif-button",
     },
     {
-      name: fontTypeSample,
-      description: gStrings.GetStringFromName("aboutReader.fontType.serif"),
+      name: gStrings.GetStringFromName("aboutReader.fontType.serif"),
+      groupName: "font-type",
       value: "serif",
       itemClass: "serif-button",
     },
   ];
 
   let fontType = Services.prefs.getCharPref("reader.font_type");
   this._setupSegmentedButton(
     "font-type-buttons",
@@ -973,81 +978,56 @@ AboutReader.prototype = {
     let url = win ? win.location.href : this._win.location.href;
     return ReaderMode.getOriginalUrl(url) || url;
   },
 
   _setupSegmentedButton(id, options, initialValue, callback) {
     let doc = this._doc;
     let segmentedButton = doc.getElementsByClassName(id)[0];
 
-    for (let i = 0; i < options.length; i++) {
-      let option = options[i];
-
-      let item = doc.createElement("button");
-
+    for (let option of options) {
       let radioButton = doc.createElement("input");
+      radioButton.id = "radio-item" + option.itemClass;
       radioButton.type = "radio";
       radioButton.classList.add("radio-button");
-      radioButton.id = "radio-item-" + option.itemClass;
-      item.appendChild(radioButton);
-
-      if (option.itemClass !== undefined) {
-        item.classList.add(option.itemClass);
-      }
+      radioButton.name = option.groupName;
+      segmentedButton.appendChild(radioButton);
 
-      if (option.description !== undefined) {
-        let description = doc.createElement("label");
-        description.textContent = option.description;
-        description.classList.add("description");
-        description.htmlFor = "radio-item-" + option.itemClass;
-        item.appendChild(description);
-
-        let span = doc.createElement("span");
-        span.classList.add("name");
-        item.appendChild(span);
-      } else {
-        let name = doc.createElement("label");
-        name.textContent = option.name;
-        name.classList.add("description");
-        name.htmlFor = "radio-item-" + option.itemClass;
-        item.appendChild(name);
-      }
+      let item = doc.createElement("label");
+      item.textContent = option.name;
+      item.htmlFor = radioButton.id;
+      item.classList.add(option.itemClass);
 
       segmentedButton.appendChild(item);
 
-      item.addEventListener(
-        "click",
+      radioButton.addEventListener(
+        "input",
         function(aEvent) {
           if (!aEvent.isTrusted) {
             return;
           }
 
-          aEvent.stopPropagation();
-
           // Just pass the ID of the button as an extra and hope the ID doesn't change
           // unless the context changes
           UITelemetry.addEvent("action.1", "button", null, id);
 
-          let items = segmentedButton.children;
-          for (let j = items.length - 1; j >= 0; j--) {
-            items[j].classList.remove("selected");
-            let itemRadioButton = items[j].firstElementChild;
-            itemRadioButton.checked = false;
+          let labels = segmentedButton.children;
+          for (let label of labels) {
+            label.removeAttribute("checked");
           }
 
-          item.classList.add("selected");
-          radioButton.checked = true;
+          aEvent.target.nextElementSibling.setAttribute("checked", "true");
           callback(option.value);
         },
         true
       );
 
       if (option.value === initialValue) {
         radioButton.checked = true;
-        item.classList.add("selected");
+        item.setAttribute("checked", "true");
       }
     }
   },
 
   _setupButton(id, callback, titleEntity) {
     let button = this._doc.querySelector("." + id);
     if (titleEntity) {
       this._setButtonTip(button, titleEntity);
--- a/toolkit/components/reader/content/aboutReader.html
+++ b/toolkit/components/reader/content/aboutReader.html
@@ -19,38 +19,34 @@
     <div class="toolbar reader-toolbar">
       <div class="reader-controls">
         <button class="close-button button "></button>
         <ul class="dropdown style-dropdown">
           <li>
             <button class="dropdown-toggle button style-button"></button>
           </li>
           <li class="dropdown-popup">
-            <div class="font-type-buttons"></div>
-            <hr>
-            <div class="font-size-buttons">
+            <div class="dropdown-arrow"></div>
+            <div class="font-type-buttons radiorow"></div>
+            <div class="font-size-buttons buttonrow">
               <button class="minus-button"></button>
               <span class="font-size-value"></span>
               <button class="plus-button"/>
             </div>
-            <hr>
-            <div class="content-width-buttons">
+            <div class="content-width-buttons buttonrow">
                <button class="content-width-minus-button"></button>
                <span class="content-width-value"></span>
                <button class="content-width-plus-button"/>
             </div>
-            <hr>
-            <div class="line-height-buttons">
+            <div class="line-height-buttons buttonrow">
                 <button class="line-height-minus-button"></button>
                 <span class="line-height-value"></span>
                 <button class="line-height-plus-button"/>
             </div>
-            <hr>
-            <div class="color-scheme-buttons"></div>
-            <div class="dropdown-arrow"/>
+            <div class="color-scheme-buttons radiorow"></div>
           </li>
         </ul>
       </div>
     </div>
   </div>
 
   <div class="container">
     <div class="header reader-header">
--- a/toolkit/themes/shared/aboutReader.css
+++ b/toolkit/themes/shared/aboutReader.css
@@ -2,27 +2,25 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* Avoid adding ID selector rules in this style sheet, since they could
  * inadvertently match elements in the article content. */
 :root {
   --body-padding: 64px;
   --popup-border: rgba(0, 0, 0, 0.12);
+  --opaque-popup-border: #e0e0e0;
   --popup-shadow: rgba(49, 49, 49, 0.3);
-  --radio-button-border: rgb(177, 177, 179);
   --grey-90-a10: rgba(12, 12, 13, 0.1);
   --grey-90-a20: rgba(12, 12, 13, 0.2);
   --grey-90-a30: rgba(12, 12, 13, 0.3);
   --grey-90-a80: rgba(12, 12, 13, 0.8);
-  --grey-10: #f9f9fa;
-  --grey-20: #ededf0;
   --grey-30: #d7d7db;
-  --grey-50: #737373;
   --blue-40: #45a1ff;
+  --blue-40-a30: rgba(69, 161, 255, 0.3);
   --blue-60: #0060df;
   --active-color: #0B83FF;
   --font-size: 12;
   --content-width: 22em;
   --line-height: 1.6em;
   --tooltip-background: var(--grey-90-a80);
   --tooltip-foreground: white;
   --toolbar-button-hover: var(--grey-90-a10);
@@ -30,21 +28,22 @@
 }
 
 
 body {
   --main-background: #fff;
   --main-foreground: #333;
   --toolbar-border: var(--grey-90-a20);
   --toolbar-box-shadow: var(--grey-90-a10);
-  --popup-button-hover: #e0e0e1;
+  --popup-button-hover: hsla(0,0%,70%,.4);
+  --popup-button-active: hsla(240,5%,5%,.15);
   --popup-bgcolor: white;
   --popup-button: #edecf0;
-  --selected-button: #d1e1ff;
-  --selected-border: #a0beff;
+  --selected-fonttype-background: var(--blue-40-a30);
+  --selected-border: var(--blue-40);
   --popup-line: var(--grey-30);
   --font-value-border: var(--grey-30);
   --font-color: #000000;
   --icon-fill: #3b3b3c;
   --icon-disabled-fill: #8080807F;
   /* light colours */
 }
 
@@ -56,22 +55,21 @@ body.sepia {
 
 body.dark {
   --main-background: #333;
   --main-foreground: #eee;
   --toolbar-border: #4a4a4b;
   --toolbar-box-shadow: black;
   --toolbar-button-hover: var(--grey-90-a30);
   --toolbar-button-active: var(--grey-90-a80);
-  --popup-button-hover: var(--grey-50);
+  --popup-button-active: hsla(0,0%,70%,.6);
   --popup-bgcolor: #4c4a50;
   --popup-button:  #5c5c61;
-  --selected-button: var(--blue-40);
-  --selected-border: var(--blue-60);
   --popup-line: #5c5c61;
+  --opaque-popup-border: #434146;
   --font-value-border: #656468;
   --font-color: #fff;
   --icon-fill: #fff;
   --icon-disabled-fill: #ffffff66;
   --tooltip-background: black;
   --tooltip-foreground: white;
   /* dark colours */
 }
@@ -152,24 +150,16 @@ body.dark blockquote {
   background-color: #333333;
 }
 
 .sepia-button {
   color: #5b4636;
   background-color: #f4ecd8;
 }
 
-.sans-serif-button {
-  font-family: Helvetica, Arial, sans-serif;
-}
-
-.serif-button {
-  font-family: Georgia, "Times New Roman", serif;
-}
-
 /* Loading/error message */
 
 .reader-message {
   margin-top: 40px;
   display: none;
   text-align: center;
   width: 100%;
   font-size: 0.9em;
@@ -403,245 +393,207 @@ button:disabled {
   inset-inline-start: 40px;
   z-index: 1000;
   background-color: var(--popup-bgcolor);
   visibility: hidden;
   border-radius: 4px;
   border: 1px solid var(--popup-border);
   box-shadow: 0 4px 6px 0 var(--popup-shadow);
   border-bottom-width: 0;
-}
-
-.dropdown-popup > hr {
-  display: none;
+  top: 0;
 }
 
 .open > .dropdown-popup {
   visibility: visible;
 }
 
 .dropdown-arrow {
   position: absolute;
   height: 24px;
   width: 16px;
   inset-inline-start: -16px;
   background-image: url("chrome://global/skin/reader/RM-Type-Controls-Arrow.svg");
   display: block;
   -moz-context-properties:  fill, stroke;
   fill: var(--popup-bgcolor);
-  stroke: var(--popup-border);
+  stroke: var(--opaque-popup-border);
   pointer-events: none;
 }
 
 .dropdown-arrow:dir(rtl) {
   transform: scaleX(-1);
 }
 
-/* Align the style dropdown (narrate does its own) */
-.style-dropdown .dropdown-popup {
-  top: -57px;
-}
-
+/* Align the style dropdown arrow (narrate does its own) */
 .style-dropdown .dropdown-arrow {
-  top: 64px;
+  top: 7px;
 }
 
 /*======= Font style popup =======*/
 
-.font-type-buttons,
-.font-size-buttons,
-.color-scheme-buttons,
-.content-width-buttons,
-.line-height-buttons {
+.radio-button {
+  /* We visually hide these, but we keep them around so they can be focused
+   * and changed by interacting with them via the label, or the keyboard, or
+   * assistive technology.
+   */
+  opacity: 0;
+  pointer-events: none;
+  position: absolute;
+}
+
+.radiorow,
+.buttonrow {
   display: flex;
-  flex-direction: row;
-  justify-content: space-evenly;
 }
 
 .content-width-value,
 .font-size-value,
 .line-height-value {
-  background-color: var(--popup-button);
-  border-radius: 10px;
-  display: inline-block;
-  width: 50px;
-  border: 1px solid var(--font-value-border);
-  text-align: center;
+  box-sizing: border-box;
+  width: 36px;
+  height: 15px;
+  line-height: 15px;
+  display: flex;
+  justify-content: center;
+  align-content: center;
   margin: auto;
-}
-
-.font-type-buttons > button,
-.font-size-buttons > button,
-.color-scheme-buttons > button,
-.content-width-buttons > button,
-.line-height-buttons > button {
-  text-align: center;
-  border: 0;
+  border-radius: 10px;
+  border: 1px solid var(--font-value-border);
+  background-color: var(--popup-button);
 }
 
-.font-type-buttons > button,
-.font-size-buttons > button,
-.content-width-buttons > button,
-.line-height-buttons > button {
-  width: 50%;
+.buttonrow > button {
+  border: 0;
+  height: 60px;
+  width: 90px;
   background-color: transparent;
-}
-
-.color-scheme-buttons > button {
-  font-size: 10px;
-  margin: 10px;
-  height: 34px;
-  border-radius: 4px;
-  width: 70px;
-  display: inline;
-  border: 1px solid var(--popup-border);
-  padding-block: 0 12px;
-  padding-inline: 0 21px;
+  background-repeat: no-repeat;
+  background-position: center;
 }
 
-.color-scheme-buttons .radio-button {
-  margin-block-end: -1px;
-  display: inline-block;
+.buttonrow > button:enabled:hover,
+.buttonrow > button:enabled:-moz-focusring {
+  background-color: var(--popup-button-hover);
 }
 
-.font-type-buttons > button {
-  height: 70px;
-  margin: 10px;
-  background-color: var(--popup-button);
-  border-radius: 2px;
-  border: 1px solid var(--popup-line);
+.buttonrow > button:enabled:active {
+  background-color: var(--popup-button-active);
 }
 
-.font-type-buttons,
-.font-size-buttons,
-.content-width-buttons,
-.line-height-buttons {
-  border-bottom:  1px solid var(--popup-line);
+.radiorow:not(:last-child),
+.buttonrow:not(:last-child) {
+  border-bottom: 1px solid var(--popup-line);
 }
 
-.font-size-buttons > button,
-.content-width-buttons > button,
-.line-height-buttons > button {
-  height: 60px;
-  width: 90px;
+.radiorow > label {
+  position: relative;
+  box-sizing: border-box;
+  border-radius: 2px;
+  border: 2px solid var(--popup-border);
 }
 
-.font-type-buttons > button.selected:active:hover,
-.font-type-buttons > button.selected:hover,
-.font-type-buttons > button.selected {
-  background-color: var(--selected-button);
-  border: 1px solid var(--selected-border);
-}
-
-.color-scheme-buttons > button.selected,
-.font-type-buttons > button:-moz-focusring,
-.color-scheme-buttons > button:-moz-focusring {
+.radiorow > label[checked] {
   border-color: var(--selected-border);
 }
 
-/* Make the serif button content the same size as the sans-serif button content. */
-.font-type-buttons > button > .description {
+/* For the hover style, we draw a line under the item by means of a
+ * pseudo-element. Because these items are variable height, and
+ * because their contents are variable height, position it absolutely,
+ * and give it a width of 100% (the content width) + 4px for the 2 * 2px
+ * border width.
+ */
+.radiorow > input[type=radio]:-moz-focusring + label::after,
+.radiorow > label:hover::after {
+  content: "";
+  display: block;
+  border-bottom: 2px solid var(--blue-40);
+  width: calc(100% + 4px);
+  position: absolute;
+  /* to skip the 2 * 2px border + 2px spacing. */
+  bottom: -6px;
+  /* Match the start of the 2px border of the element: */
+  inset-inline-start: -2px;
+}
+
+.color-scheme-buttons > label {
+  height: 34px;
+  width: 70px;
   font-size: 12px;
-  text-align: start;
-  display: inline-block;
-  min-width: 60%;
+  /* Center the labels horizontally as well as vertically */
+  display: inline-flex;
+  align-items: center;
+  justify-content: center;
+  /* We want 10px between items, but there's no margin collapsing in flexbox. */
+  margin: 10px 5px;
 }
 
-.description {
-  line-height: 1.5em;
-  vertical-align: middle;
+.color-scheme-buttons > input:first-child + label {
+  margin-inline-start: 10px;
+}
+
+.color-scheme-buttons > label:last-child {
+  margin-inline-end: 10px;
 }
 
-.font-type-buttons > .sans-serif-button > .name {
+.font-type-buttons > label {
+  height: 64px;
+  width: 105px;
+  /* Slightly more space between these items. */
+  margin: 10px;
+  /* Center the Sans-serif / Serif labels */
+  text-align: center;
+  background-size: 63px 39px;
+  background-repeat: no-repeat;
+  background-position: center 18px;
+  background-color: var(--popup-button);
+  fill: currentColor;
+  -moz-context-properties: fill;
+  /* This mostly matches baselines, but because of differences in font
+   * baselines between serif and sans-serif, this isn't always enough. */
+  line-height: 1;
+  padding-top: 2px;
+}
+
+.font-type-buttons > label[checked] {
+  background-color: var(--selected-fonttype-background);
+}
+
+.sans-serif-button {
+  font-family: Helvetica, Arial, sans-serif;
   background-image: url("chrome://global/skin/reader/RM-Sans-Serif.svg");
 }
 
-.font-type-buttons > .serif-button > .name{
-  background-image: url("chrome://global/skin/reader/RM-Serif.svg");
-}
-
-.font-type-buttons > .sans-serif-button > .name,
-.font-type-buttons > .serif-button > .name {
-  display: inline-block;
-  height: 39px;
-  width: 60px;
-  background-size: contain;
-  background-repeat: no-repeat;
+/* Tweak padding to match the baseline on mac */
+:root[platform=macosx] .sans-serif-button {
+  padding-top: 3px;
 }
 
-.font-size-buttons > button:enabled:hover,
-.font-size-buttons > button:enabled:-moz-focusring,
-.font-type-buttons > button:enabled:hover,
-.content-width-buttons > button:enabled:hover,
-.content-width-buttons > button:enabled:-moz-focusring,
-.line-height-buttons > button:enabled:hover,
-.line-height-buttons > button:enabled:-moz-focusring {
-  background-color: var(--popup-button-hover);
-}
-
-.font-size-buttons > button:enabled:active,
-.font-size-buttons > button.selected,
-.content-width-buttons > button:enabled:active,
-.content-width-buttons > button.selected,
-.line-height-buttons > button:enabled:active,
-.line-height-buttons > button.selected {
-  background-color: #dadada;
-}
-
-.minus-button,
-.plus-button,
-.content-width-minus-button,
-.content-width-plus-button,
-.line-height-minus-button,
-.line-height-plus-button {
-  background-color: transparent;
-  border: 0;
-  background-size: 18px 18px;
-  background-repeat: no-repeat;
-  background-position: center;
+.serif-button {
+  font-family: Georgia, "Times New Roman", serif;
+  background-image: url("chrome://global/skin/reader/RM-Serif.svg");
 }
 
 /*======= Toolbar icons =======*/
 
 .close-button {
   -moz-context-properties: fill;
   background-image: url("chrome://global/skin/reader/close-16.svg");
 }
 
 .style-button {
   background-image: url("chrome://global/skin/reader/RM-Type-Controls-24x24.svg");
 }
 
-.radio-button {
-  -moz-appearance: none;
-  -moz-outline-radius: 100%;
-  border-radius: 50%;
-  width: 10px;
-  height: 10px;
-  outline: 1px solid var(--radio-button-border);
-  background-color: #ffffff;
-  margin: 0;
-  margin-inline-end: 5px;
-  vertical-align: middle;
-}
-
-.radio-button:checked {
-  background-color: var(--blue-40);;
-  border: 2px solid #ffffff;
-}
-
-.font-type-buttons .radio-button {
-  margin-inline-start: -2px;
-}
-
 .minus-button {
+  background-size: 18px 18px;
   background-image: url("chrome://global/skin/reader/RM-Minus-24x24.svg");
 }
 
 .plus-button {
+  background-size: 18px 18px;
   background-image: url("chrome://global/skin/reader/RM-Plus-24x24.svg");
 }
 
 .content-width-minus-button {
   background-size: 42px 16px;
   background-image: url("chrome://global/skin/reader/RM-Content-Width-Minus-42x16.svg");
 }
 
--- a/toolkit/themes/shared/narrate.css
+++ b/toolkit/themes/shared/narrate.css
@@ -1,15 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* Avoid adding ID selector rules in this style sheet, since they could
  * inadvertently match elements in the article content. */
-
+:root {
+  --blue-60: #0060df;
+}
 .narrating {
   position: relative;
   z-index: 1;
 }
 
 body.light .narrating {
   background-color: #ffc;
 }