Bug 1386015 - Do not generate styling for each element with inherited color. r=jaws
authorZibi Braniecki <zbraniecki@mozilla.com>
Fri, 11 Aug 2017 17:38:14 -0700
changeset 375119 31a7f0775f925e74906ff7633c9968628da25e20
parent 375118 eddf184c4d74ecdc7eb0c463b9f4b67ab1c0d3bd
child 375120 c24de5ff5ebf18beaef47b112598c381066b667d
push id48941
push userzbraniecki@mozilla.com
push dateWed, 16 Aug 2017 18:22:50 +0000
treeherderautoland@31a7f0775f92 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1386015
milestone57.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 1386015 - Do not generate styling for each element with inherited color. r=jaws This patch does a minor refactor of the code used to style popup menu for the <select> element. It improves the custom styling experience on MacOS, preserves the functionality on Windows and removes the unnecessary per-item CSS rules significantly improving the performance of opening the <select> list. MozReview-Commit-ID: 7myXq8aDAWr
browser/base/content/test/forms/browser_selectpopup_colors.js
toolkit/modules/SelectParentHelper.jsm
--- a/browser/base/content/test/forms/browser_selectpopup_colors.js
+++ b/browser/base/content/test/forms/browser_selectpopup_colors.js
@@ -164,16 +164,29 @@ let SELECT_LONG_WITH_TRANSITION =
 for (let i = 0; i < 75; i++) {
   SELECT_LONG_WITH_TRANSITION +=
     '  <option>{"color": "rgb(128, 0, 128)", "backgroundColor": "rgba(0, 0, 0, 0)"}</option>';
 }
 SELECT_LONG_WITH_TRANSITION +=
     '  <option selected="true">{"end": "true"}</option>' +
   "</select></body></html>";
 
+const SELECT_INHERITED_COLORS_ON_OPTIONS_DONT_GET_UNIQUE_RULES_IF_RULE_SET_ON_SELECT = `
+   <html><head><style>
+     select { color: blue; text-shadow: 1px 1px 2px blue; }
+     .redColor { color: red; }
+     .textShadow { text-shadow: 1px 1px 2px black; }
+   </style></head><body><select id='one'>
+     <option>{"color": "rgb(0, 0, 255)", "backgroundColor": "rgba(0, 0, 0, 0)"}</option>
+     <option class="redColor">{"color": "rgb(255, 0, 0)", "backgroundColor": "rgba(0, 0, 0, 0)"}</option>
+     <option class="textShadow">{"color": "rgb(0, 0, 255)", "textShadow": "rgb(0, 0, 0) 1px 1px 2px", "backgroundColor": "rgba(0, 0, 0, 0)"}</option>
+     <option selected="true">{"end": "true"}</option>
+   </select></body></html>
+`;
+
 function getSystemColor(color) {
   // Need to convert system color to RGB color.
   let textarea = document.createElementNS("http://www.w3.org/1999/xhtml", "textarea");
   textarea.style.color = color;
   return getComputedStyle(textarea).color;
 }
 
 function testOptionColors(index, item, menulist) {
@@ -467,8 +480,53 @@ add_task(async function test_select_with
   let selectPopup = menulist.menupopup;
   let scrollBox = selectPopup.scrollBox;
   is(scrollBox.scrollTop, scrollBox.scrollTopMax,
     "The popup should be scrolled to the bottom of the list (where the selected item is)");
 
   await hideSelectPopup(selectPopup, "escape");
   await BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
+
+add_task(async function test_select_inherited_colors_on_options_dont_get_unique_rules_if_rule_set_on_select() {
+  let options = {
+    selectColor: "rgb(0, 0, 255)",
+    selectBgColor: "rgb(255, 255, 255)",
+    selectTextShadow: "rgb(0, 0, 255) 1px 1px 2px",
+    leaveOpen: true
+  };
+
+  await testSelectColors(SELECT_INHERITED_COLORS_ON_OPTIONS_DONT_GET_UNIQUE_RULES_IF_RULE_SET_ON_SELECT, 4, options);
+
+  let stylesheetEl = document.getElementById("ContentSelectDropdownStylesheet");
+  let sheet = stylesheetEl.sheet;
+  /* Check that there are no rulesets for the first option, but that
+     one exists for the second option and sets the color of that
+     option to "rgb(255, 0, 0)" */
+
+  function hasMatchingRuleForOption(cssRules, index, styles = {}) {
+    for (let rule of cssRules) {
+      if (rule.selectorText.includes(`:nth-child(${index})`)) {
+        if (Object.keys(styles).some(key => rule.style[key] !== styles[key])) {
+          continue;
+        }
+        return true;
+      }
+    }
+    return false;
+  }
+
+  is(hasMatchingRuleForOption(sheet.cssRules, 1), false,
+    "There should be no rules specific to option1");
+  is(hasMatchingRuleForOption(sheet.cssRules, 2, {
+    color: "rgb(255, 0, 0)"
+  }), true, "There should be a rule specific to option2 and it should have color: red");
+  is(hasMatchingRuleForOption(sheet.cssRules, 3, {
+    "text-shadow": "rgb(0, 0, 0) 1px 1px 2px"
+  }), true, "There should be a rule specific to option3 and it should have text-shadow: rgb(0, 0, 0) 1px 1px 2px");
+
+
+  let menulist = document.getElementById("ContentSelectDropdown");
+  let selectPopup = menulist.menupopup;
+
+  await hideSelectPopup(selectPopup, "escape");
+  await BrowserTestUtils.removeTab(gBrowser.selectedTab);
+});
--- a/toolkit/modules/SelectParentHelper.jsm
+++ b/toolkit/modules/SelectParentHelper.jsm
@@ -21,22 +21,50 @@ const SEARCH_MINIMUM_ELEMENTS = 40;
 // Make sure to clear these objects when the popup closes to avoid leaking.
 var currentBrowser = null;
 var currentMenulist = null;
 var selectRect = null;
 
 var currentZoom = 1;
 var closedWithEnter = false;
 var customStylingEnabled = Services.prefs.getBoolPref("dom.forms.select.customstyling");
-var usedSelectBackgroundColor;
 
 this.SelectParentHelper = {
+  /**
+   * `populate` takes the `menulist` element and a list of `items` and generates
+   * a popup list of options.
+   *
+   * If `customStylingEnabled` is set to `true`, the function will alse
+   * style the select and its popup trying to prevent the text
+   * and background to end up in the same color.
+   *
+   * All `ua*` variables represent the color values for the default colors
+   * for their respective form elements used by the user agent.
+   * The `select*` variables represent the color values defined for the
+   * particular <select> element.
+   *
+   * The `customoptionstyling` attribute controls the application of
+   * `-moz-appearance` on the elements and is disabled if the element is
+   * defining its own background-color.
+   *
+   * @param {Element}        menulist
+   * @param {Array<Element>} items
+   * @param {Number}         selectedIndex
+   * @param {Number}         zoom
+   * @param {String}         uaBackgroundColor
+   * @param {String}         uaColor
+   * @param {String}         uaSelectBackgroundColor
+   * @param {String}         uaSelectColor
+   * @param {String}         selectBackgroundColor
+   * @param {String}         selectColor
+   * @param {String}         selectTextShadow
+   */
   populate(menulist, items, selectedIndex, zoom, uaBackgroundColor, uaColor,
-           uaSelectBackgroundColor, uaSelectColor, selectBackgroundColor, selectColor,
-           selectTextShadow) {
+           uaSelectBackgroundColor, uaSelectColor, selectBackgroundColor,
+           selectColor, selectTextShadow) {
     // Clear the current contents of the popup
     menulist.menupopup.textContent = "";
     let stylesheet = menulist.querySelector("#ContentSelectDropdownStylesheet");
     if (stylesheet) {
       stylesheet.remove();
     }
 
     let doc = menulist.ownerDocument;
@@ -45,54 +73,71 @@ this.SelectParentHelper = {
       stylesheet = doc.createElementNS("http://www.w3.org/1999/xhtml", "style");
       stylesheet.setAttribute("id", "ContentSelectDropdownStylesheet");
       stylesheet.hidden = true;
       stylesheet = menulist.appendChild(stylesheet);
       sheet = stylesheet.sheet;
     }
 
     let ruleBody = "";
+    let usedSelectBackgroundColor;
+    let usedSelectColor;
+    let selectBackgroundSet = false;
 
     // Some webpages set the <select> backgroundColor to transparent,
     // but they don't intend to change the popup to transparent.
     if (customStylingEnabled &&
         selectBackgroundColor != uaSelectBackgroundColor &&
         selectBackgroundColor != "rgba(0, 0, 0, 0)") {
       ruleBody = `background-image: linear-gradient(${selectBackgroundColor}, ${selectBackgroundColor});`;
       usedSelectBackgroundColor = selectBackgroundColor;
+      selectBackgroundSet = true;
     } else {
       usedSelectBackgroundColor = uaSelectBackgroundColor;
     }
 
     if (customStylingEnabled &&
         selectColor != uaSelectColor &&
-        selectColor != selectBackgroundColor &&
-        (selectBackgroundColor != "rgba(0, 0, 0, 0)" ||
-         selectColor != uaSelectBackgroundColor)) {
+        selectColor != usedSelectBackgroundColor) {
       ruleBody += `color: ${selectColor};`;
+      usedSelectColor = selectColor;
+    } else {
+      usedSelectColor = uaColor;
     }
 
     if (customStylingEnabled &&
         selectTextShadow != "none") {
       ruleBody += `text-shadow: ${selectTextShadow};`;
+      sheet.insertRule(`#ContentSelectDropdown > menupopup > [_moz-menuactive="true"] {
+        text-shadow: none;
+      }`, 0);
     }
 
     if (ruleBody) {
       sheet.insertRule(`#ContentSelectDropdown > menupopup {
         ${ruleBody}
       }`, 0);
+      sheet.insertRule(`#ContentSelectDropdown > menupopup > :not([_moz-menuactive="true"]) {
+         color: inherit;
+      }`, 0);
+    }
+
+    // We only set the `customoptionstyling` if the background has been
+    // manually set. This prevents the overlap between moz-appearance and
+    // background-color. `color` and `text-shadow` do not interfere with it.
+    if (selectBackgroundSet) {
       menulist.menupopup.setAttribute("customoptionstyling", "true");
     } else {
       menulist.menupopup.removeAttribute("customoptionstyling");
     }
 
     currentZoom = zoom;
     currentMenulist = menulist;
     populateChildren(menulist, items, selectedIndex, zoom,
-                     uaBackgroundColor, uaColor, sheet);
+                     usedSelectBackgroundColor, usedSelectColor, selectTextShadow, selectBackgroundSet, sheet);
   },
 
   open(browser, menulist, rect, isOpenedViaTouch) {
     menulist.hidden = false;
     currentBrowser = browser;
     closedWithEnter = false;
     selectRect = rect;
     this._registerListeners(browser, menulist.menupopup);
@@ -248,20 +293,46 @@ this.SelectParentHelper = {
     browser.ownerGlobal.removeEventListener("keydown", this, true);
     browser.ownerGlobal.removeEventListener("fullscreen", this, true);
     browser.messageManager.removeMessageListener("Forms:UpdateDropDown", this);
     browser.messageManager.removeMessageListener("Forms:BlurDropDown-Ping", this);
   },
 
 };
 
+/**
+ * `populateChildren` creates all <menuitem> elements for the popup menu
+ * based on the list of <option> elements from the <select> element.
+ *
+ * It attempts to intelligently add per-item CSS rules if the single
+ * item values differ from the parent menu values and attempting to avoid
+ * ending up with the same color of text and background.
+ *
+ * @param {Element}        menulist
+ * @param {Array<Element>} options
+ * @param {Number}         selectedIndex
+ * @param {Number}         zoom
+ * @param {String}         usedSelectBackgroundColor
+ * @param {String}         usedSelectColor
+ * @param {String}         selectTextShadow
+ * @param {String}         selectBackgroundSet
+ * @param {CSSStyleSheet}  sheet
+ * @param {Element}        parentElement
+ * @param {Boolean}        isGroupDisabled
+ * @param {Number}         adjustedTextSize
+ * @param {Boolean}        addSearch
+ * @param {Number}         nthChildIndex
+ * @returns {Number}
+ */
 function populateChildren(menulist, options, selectedIndex, zoom,
-                          uaBackgroundColor, uaColor, sheet,
+                          usedSelectBackgroundColor, usedSelectColor,
+                          selectTextShadow, selectBackgroundSet, sheet,
                           parentElement = null, isGroupDisabled = false,
-                          adjustedTextSize = -1, addSearch = true, nthChildIndex = 1) {
+                          adjustedTextSize = -1, addSearch = true,
+                          nthChildIndex = 1) {
   let element = menulist.menupopup;
   let win = element.ownerGlobal;
 
   // -1 just means we haven't calculated it yet. When we recurse through this function
   // we will pass in adjustedTextSize to save on recalculations.
   if (adjustedTextSize == -1) {
     // Grab the computed text size and multiply it by the remote browser's fullZoom to ensure
     // the popup's text size is matched with the content's. We can't just apply a CSS transform
@@ -279,48 +350,60 @@ function populateChildren(menulist, opti
     item.style.fontSize = adjustedTextSize;
     item.hidden = option.display == "none" || (parentElement && parentElement.hidden);
     // Keep track of which options are hidden by page content, so we can avoid showing
     // them on search input
     item.hiddenByContent = item.hidden;
     item.setAttribute("tooltiptext", option.tooltip);
 
     let ruleBody = "";
+    let usedBackgroundColor;
+    let optionBackgroundSet = false;
+
     if (customStylingEnabled &&
         option.backgroundColor &&
         option.backgroundColor != "rgba(0, 0, 0, 0)" &&
         option.backgroundColor != usedSelectBackgroundColor) {
       ruleBody = `background-color: ${option.backgroundColor};`;
+      usedBackgroundColor = option.backgroundColor;
+      optionBackgroundSet = true;
+    } else {
+      usedBackgroundColor = usedSelectBackgroundColor;
     }
 
     if (customStylingEnabled &&
         option.color &&
-        option.color != uaColor) {
+        option.color != usedBackgroundColor &&
+        option.color != usedSelectColor) {
       ruleBody += `color: ${option.color};`;
     }
 
     if (customStylingEnabled &&
-        option.textShadow) {
+        option.textShadow &&
+        option.textShadow != selectTextShadow) {
       ruleBody += `text-shadow: ${option.textShadow};`;
     }
 
     if (ruleBody) {
       sheet.insertRule(`#ContentSelectDropdown > menupopup > :nth-child(${nthChildIndex}):not([_moz-menuactive="true"]) {
         ${ruleBody}
       }`, 0);
 
-      if (option.textShadow) {
+      if (option.textShadow && option.textShadow != selectTextShadow) {
         // Need to explicitly disable the possibly inherited
         // text-shadow rule when _moz-menuactive=true since
         // _moz-menuactive=true disables custom option styling.
         sheet.insertRule(`#ContentSelectDropdown > menupopup > :nth-child(${nthChildIndex})[_moz-menuactive="true"] {
           text-shadow: none;
         }`, 0);
       }
+    }
 
+    if (customStylingEnabled &&
+        (optionBackgroundSet || selectBackgroundSet)) {
       item.setAttribute("customoptionstyling", "true");
     } else {
       item.removeAttribute("customoptionstyling");
     }
 
     element.appendChild(item);
     nthChildIndex++;
 
@@ -328,17 +411,18 @@ function populateChildren(menulist, opti
     let isDisabled = isGroupDisabled || option.disabled;
     if (isDisabled) {
       item.setAttribute("disabled", "true");
     }
 
     if (isOptGroup) {
       nthChildIndex =
         populateChildren(menulist, option.children, selectedIndex, zoom,
-                         uaBackgroundColor, uaColor, sheet,
+                         usedSelectBackgroundColor, usedSelectColor,
+                         selectTextShadow, selectBackgroundSet, sheet,
                          item, isDisabled, adjustedTextSize, false, nthChildIndex);
     } else {
       if (option.index == selectedIndex) {
         // We expect the parent element of the popup to be a <xul:menulist> that
         // has the popuponly attribute set to "true". This is necessary in order
         // for a <xul:menupopup> to act like a proper <html:select> dropdown, as
         // the <xul:menulist> does things like remember state and set the
         // _moz-menuactive attribute on the selected <xul:menuitem>.