Bug 1352120 - fix theming for the star icon, fix theming dealing with empty string icon urls, r?jaws draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Thu, 22 Jun 2017 13:08:52 +0100
changeset 600863 5b5ad60442cd7b3da13a298288aa118ee3886fdf
parent 600862 eb49e899a0ca3d465527988c25143e93b6e59d15
child 635107 7bed93730923888bd825179940e0df9811ef9f10
push id65889
push userbmo:gijskruitbosch+bugs@gmail.com
push dateTue, 27 Jun 2017 21:35:25 +0000
reviewersjaws
bugs1352120
milestone56.0a1
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;
     }
   }