Bug 1352120 - fix theming for the star icon, fix theming dealing with empty string icon urls, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Thu, 22 Jun 2017 13:08:52 +0100
changeset 414919 8a35db6fffa6adbb42fb3f5f837c55ad124399e7
parent 414918 ffcc7b9e7baaf11c375619d296aac1f90c2aaaff
child 414920 5554fd16af9b429902897cb1d32f1fcd7d47517b
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1352120
milestone56.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 1352120 - fix theming for the star icon, fix theming dealing with empty string icon urls, r=jaws When debugging the test failures in this test, I noticed that the info() messages indicated we *were* using moz-extension icon references even when we shouldn't be - they just didn't include the 'fox.svg' bit. When pausing in the debugger, you can see that all the buttons are blank - we don't load any icon in this case. This seemed bad, so I updated the test to actually check if we're using a moz-extension URI at all, and then updated the implementation to actually make it work. MozReview-Commit-ID: GGXaivJrzxj
browser/base/content/theme-vars.inc.css
browser/components/extensions/test/browser/browser_ext_themes_icons.js
toolkit/components/extensions/ext-theme.js
--- a/browser/base/content/theme-vars.inc.css
+++ b/browser/base/content/theme-vars.inc.css
@@ -15,20 +15,26 @@
 :root[lwthemeicons~="--reload-icon"] #reload-button:-moz-lwtheme {
   list-style-image: var(--reload-icon) !important;
 }
 
 :root[lwthemeicons~="--stop-icon"] #stop-button:-moz-lwtheme {
   list-style-image: var(--stop-icon) !important;
 }
 
+%ifdef MOZ_PHOTON_THEME
+:root[lwthemeicons~="--bookmark_star-icon"] #star-button:-moz-lwtheme,
+%endif
 :root[lwthemeicons~="--bookmark_star-icon"] #bookmarks-menu-button:-moz-lwtheme {
   list-style-image: var(--bookmark_star-icon) !important;
 }
 
+%ifdef MOZ_PHOTON_THEME
+:root[lwthemeicons~="--bookmark_menu-icon"] #bookmarks-menu-button:-moz-lwtheme,
+%endif
 :root[lwthemeicons~="--bookmark_menu-icon"] #bookmarks-menu-button[cui-areatype='toolbar'] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon:-moz-lwtheme {
   list-style-image: var(--bookmark_menu-icon) !important;
 }
 
 :root[lwthemeicons~="--downloads-icon"] #downloads-button:-moz-lwtheme {
   list-style-image: var(--downloads-icon) !important;
 }
 
@@ -123,16 +129,20 @@
 :root[lwthemeicons~="--forget-icon"] #panic-button:-moz-lwtheme {
   list-style-image: var(--forget-icon) !important;
 }
 
 :root[lwthemeicons~="--pocket-icon"] #pocket-button:-moz-lwtheme {
   list-style-image: var(--pocket-icon) !important;
 }
 
+%ifdef MOZ_PHOTON_THEME
+:root[lwthemeicons~="--bookmark_star-icon"] #star-button:-moz-lwtheme,
+:root[lwthemeicons~="--bookmark_menu-icon"] #bookmarks-menu-button:-moz-lwtheme,
+%endif
 :root[lwthemeicons~="--back-icon"] #back-button:-moz-lwtheme,
 :root[lwthemeicons~="--forward-icon"] #forward-button:-moz-lwtheme,
 :root[lwthemeicons~="--reload-icon"] #reload-button:-moz-lwtheme,
 :root[lwthemeicons~="--stop-icon"] #stop-button:-moz-lwtheme,
 :root[lwthemeicons~="--bookmark_star-icon"] #bookmarks-menu-button:-moz-lwtheme,
 :root[lwthemeicons~="--bookmark_menu-icon"] #bookmarks-menu-button[cui-areatype='toolbar'] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon:-moz-lwtheme,
 :root[lwthemeicons~="--downloads-icon"] #downloads-button:-moz-lwtheme,
 :root[lwthemeicons~="--home-icon"] #home-button:-moz-lwtheme,
--- a/browser/components/extensions/test/browser/browser_ext_themes_icons.js
+++ b/browser/components/extensions/test/browser/browser_ext_themes_icons.js
@@ -27,17 +27,17 @@ function verifyButtonProperties(selector
       element = document.getAnonymousElementByAttribute(element, "class", "toolbarbutton-menubutton-dropmarker");
       element = document.getAnonymousElementByAttribute(element, "class", "dropmarker-icon");
     } else {
       element = document.querySelector(selector);
     }
 
     let listStyleImage = getComputedStyle(element).listStyleImage;
     info(`listStyleImage for fox.svg is ${listStyleImage}`);
-    is(listStyleImage.includes("fox.svg"), shouldHaveCustomStyling, message);
+    is(listStyleImage.includes("moz-extension:"), shouldHaveCustomStyling, message);
   } catch (ex) {
     ok(false, `Unable to verify ${selector}: ${ex}`);
   }
 }
 
   /**
    * Verifies that the button uses default styling.
    *
@@ -72,20 +72,20 @@ function verifyButtonWithCustomStyling(s
    *   CSS selectors.
    * @param {string} area The name of the area that the button resides in.
    */
 function checkButtons(icons, iconInfo, area) {
   for (let button of iconInfo) {
     let iconInfo = icons.find(arr => arr[0] == button[0]);
     if (iconInfo[1]) {
       verifyButtonWithCustomStyling(button[1],
-        `The ${button[1]} should have it's icon customized in the ${area}`);
+        `The ${button[1]} should have its icon customized in the ${area}`);
     } else {
       verifyButtonWithoutCustomStyling(button[1],
-        `The ${button[1]} should not have it's icon customized in the ${area}`);
+        `The ${button[1]} should not have its icon customized in the ${area}`);
     }
   }
 }
 
 async function runTestWithIcons(icons) {
   const FRAME_COLOR = [71, 105, 91];
   const TAB_TEXT_COLOR = [207, 221, 192, .9];
   let manifest = {
@@ -109,18 +109,16 @@ async function runTestWithIcons(icons) {
   // At position 1: The CSS selector for the button in the DOM.
   // At position 2: The CustomizableUI name for the widget, only defined
   //                if customizable.
   const ICON_INFO = [
     ["back", "#back-button"],
     ["forward", "#forward-button"],
     ["reload", "#reload-button"],
     ["stop", "#stop-button"],
-    ["bookmark_star", "#bookmarks-menu-button", "bookmarks-menu-button"],
-    ["bookmark_menu", "#bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon"],
     ["downloads", "#downloads-button", "downloads-button"],
     ["home", "#home-button", "home-button"],
     ["app_menu", "#PanelUI-menu-button"],
     ["cut", "#cut-button", "edit-controls"],
     ["copy", "#copy-button"],
     ["paste", "#paste-button"],
     ["new_window", "#new-window-button", "new-window-button"],
     ["new_private_window", "#privatebrowsing-button", "privatebrowsing-button"],
@@ -137,26 +135,33 @@ async function runTestWithIcons(icons) {
     ["sidebars", "#sidebar-button", "sidebar-button"],
     ["share_page", "#social-share-button", "social-share-button"],
     ["subscribe", "#feed-button", "feed-button"],
     ["text_encoding", "#characterencoding-button", "characterencoding-button"],
     ["email_link", "#email-link-button", "email-link-button"],
     ["forget", "#panic-button", "panic-button"],
     ["pocket", "#pocket-button", "pocket-button"],
   ];
+  if (AppConstants.MOZ_PHOTON_THEME) {
+    ICON_INFO.push(["bookmark_star", "#star-button"]);
+    ICON_INFO.push(["bookmark_menu", "#bookmarks-menu-button", "bookmarks-menu-button"]);
+  } else {
+    ICON_INFO.push(["bookmark_star", "#bookmarks-menu-button", "bookmarks-menu-button"]);
+    ICON_INFO.push(["bookmark_menu", "#bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon"]);
+  }
 
   window.maximize();
 
   for (let button of ICON_INFO) {
     if (button[2]) {
       CustomizableUI.addWidgetToArea(button[2], CustomizableUI.AREA_NAVBAR);
     }
 
     verifyButtonWithoutCustomStyling(button[1],
-      `The ${button[1]} should not have it's icon customized when the test starts`);
+      `The ${button[1]} should not have its icon customized when the test starts`);
 
     let iconInfo = icons.find(arr => arr[0] == button[0]);
     manifest.theme.icons[button[0]] = iconInfo[1];
   }
 
   let extension = ExtensionTestUtils.loadExtension({manifest, files});
 
   await extension.startup();
@@ -176,17 +181,17 @@ async function runTestWithIcons(icons) {
 
     await PanelUI.hide();
   }
 
   await extension.unload();
 
   for (let button of ICON_INFO) {
     verifyButtonWithoutCustomStyling(button[1],
-      `The ${button[1]} should not have it's icon customized when the theme is unloaded`);
+      `The ${button[1]} should not have its icon customized when the theme is unloaded`);
   }
 }
 
 add_task(async function setup() {
   await SpecialPowers.pushPrefEnv({
     set: [["extensions.webextensions.themes.enabled", true],
           ["extensions.webextensions.themes.icons.enabled", true]],
   });
--- a/toolkit/components/extensions/ext-theme.js
+++ b/toolkit/components/extensions/ext-theme.js
@@ -135,17 +135,20 @@ class Theme {
   loadIcons(icons) {
     if (!Preferences.get("extensions.webextensions.themes.icons.enabled")) {
       // Return early if icons are disabled.
       return;
     }
 
     for (let icon of Object.getOwnPropertyNames(icons)) {
       let val = icons[icon];
-      if (!val || !ICONS.includes(icon)) {
+      // We also have to compare against the baseURI spec because
+      // `val` might have been resolved already. Resolving "" against
+      // the baseURI just produces that URI, so check for equality.
+      if (!val || val == this.baseURI.spec || !ICONS.includes(icon)) {
         continue;
       }
       let variableName = `--${icon}-icon`;
       let resolvedURL = this.baseURI.resolve(val);
       this.lwtStyles.icons[variableName] = resolvedURL;
     }
   }