Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode, r?johannh,jaws draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Thu, 05 Oct 2017 15:17:12 +0100
changeset 675593 7586fe856ab77b35662887398adc5f8e1b7a41bf
parent 675508 53bbdaaa2b8c1819061be26101b075c081b23260
child 734647 cbbb9d8368283d7af7eeaaf493bc1963676e0b8c
push id83177
push usergijskruitbosch@gmail.com
push dateThu, 05 Oct 2017 15:28:18 +0000
reviewersjohannh, jaws
bugs1405720, 1402981
milestone58.0a1
Bug 1405720 - ensure only 1 theme is ever shown as selected in customize mode, r?johannh,jaws The previous code here always set the `isActive` property on all themes. When writing the patch for bug 1402981 I ran into issues because the default theme has an `isActive` property anyway (it's a different type of object). So I tried to avoid setting `isActive` if it was already present. Unfortunately, the result was that `isActive` values, once set, weren't correctly updated. Worse, these values are (and were, prior to bug 1402981) persisted in some cases. There's no point persisting these values, all that will happen is that they'll start mismatching the 'real' state of the world (LightweightThemeManager.currentTheme). So instead, let's just not set the `isActive` property at all, and rely solely on the ID of the current theme (or the default theme's ID, now that we no longer support non-lightweight-themes) to establish whether any of the themes should appear selected or not. MozReview-Commit-ID: 7rajS71FoQR
browser/components/customizableui/CustomizeMode.jsm
browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
--- a/browser/components/customizableui/CustomizeMode.jsm
+++ b/browser/components/customizableui/CustomizeMode.jsm
@@ -1353,38 +1353,35 @@ CustomizeMode.prototype = {
           tbb.setAttribute("defaulttheme", "true");
         } else {
           tbb.setAttribute("image", aTheme.iconURL);
         }
         if (aTheme.description)
           tbb.setAttribute("tooltiptext", aTheme.description);
         tbb.setAttribute("tabindex", "0");
         tbb.classList.add("customization-lwtheme-menu-theme");
-        tbb.setAttribute("aria-checked", aTheme.isActive);
+        let isActive = activeThemeID == aTheme.id;
+        tbb.setAttribute("aria-checked", isActive);
         tbb.setAttribute("role", "menuitemradio");
-        if (aTheme.isActive) {
+        if (isActive) {
           tbb.setAttribute("active", "true");
         }
         tbb.addEventListener("focus", previewTheme);
         tbb.addEventListener("mouseover", previewTheme);
         tbb.addEventListener("blur", resetPreview);
         tbb.addEventListener("mouseout", resetPreview);
 
         return tbb;
       }
 
       let themes = [aDefaultTheme];
       let lwts = LightweightThemeManager.usedThemes;
       let currentLwt = LightweightThemeManager.currentTheme;
-      // The lwts besides the builtin themes don't have an isActive property:
-      for (let lwt of lwts) {
-        if (!lwt.hasOwnProperty("isActive")) {
-          lwt.isActive = !!currentLwt && (lwt.id == currentLwt.id);
-        }
-      }
+
+      let activeThemeID = currentLwt ? currentLwt.id : DEFAULT_THEME_ID;
 
       // Move the current theme (if any) and the light/dark themes to the start:
       let importantThemes = [LIGHT_THEME_ID, DARK_THEME_ID];
       if (currentLwt && !importantThemes.includes(currentLwt.id)) {
         importantThemes.push(currentLwt.id);
       }
       for (let importantTheme of importantThemes) {
         let themeIndex = lwts.findIndex(theme => theme.id == importantTheme);
--- a/browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
+++ b/browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
@@ -40,28 +40,50 @@ add_task(async function() {
      "There should only be three themes (default, light, dark) in the 'My Themes' section by default");
   is(header.nextSibling.theme.id, DEFAULT_THEME_ID,
      "The first theme should be the default theme");
   is(header.nextSibling.nextSibling.theme.id, LIGHT_THEME_ID,
      "The second theme should be the light theme");
   is(header.nextSibling.nextSibling.nextSibling.theme.id, DARK_THEME_ID,
      "The third theme should be the dark theme");
 
+  let themeChangedPromise = promiseObserverNotified("lightweight-theme-changed");
+  header.nextSibling.nextSibling.doCommand(); // Select light theme
+  info("Clicked on light theme");
+  await themeChangedPromise;
+
+  popupShownPromise = popupShown(popup);
+  EventUtils.synthesizeMouseAtCenter(themesButton, {});
+  info("Clicked on themes button a third time");
+  await popupShownPromise;
+
+  let activeThemes = popup.querySelectorAll("toolbarbutton.customization-lwtheme-menu-theme[active]");
+  is(activeThemes.length, 1, "Exactly 1 theme should be selected");
+  if (activeThemes.length > 0) {
+    is(activeThemes[0].theme.id, LIGHT_THEME_ID, "Light theme should be selected");
+  }
+
   let firstLWTheme = recommendedHeader.nextSibling;
   let firstLWThemeId = firstLWTheme.theme.id;
-  let themeChangedPromise = promiseObserverNotified("lightweight-theme-changed");
+  themeChangedPromise = promiseObserverNotified("lightweight-theme-changed");
   firstLWTheme.doCommand();
   info("Clicked on first theme");
   await themeChangedPromise;
 
   popupShownPromise = popupShown(popup);
   EventUtils.synthesizeMouseAtCenter(themesButton, {});
   info("Clicked on themes button");
   await popupShownPromise;
 
+  activeThemes = popup.querySelectorAll("toolbarbutton.customization-lwtheme-menu-theme[active]");
+  is(activeThemes.length, 1, "Exactly 1 theme should be selected");
+  if (activeThemes.length > 0) {
+    is(activeThemes[0].theme.id, firstLWThemeId, "First theme should be selected");
+  }
+
   is(header.nextSibling.theme.id, DEFAULT_THEME_ID, "The first theme should be the Default theme");
   let installedThemeId = header.nextSibling.nextSibling.nextSibling.nextSibling.theme.id;
   ok(installedThemeId.startsWith(firstLWThemeId),
      "The second theme in the 'My Themes' section should be the newly installed theme: " +
      "Installed theme id: " + installedThemeId + "; First theme ID: " + firstLWThemeId);
   let themeCount = 0;
   let iterNode = header;
   while (iterNode.nextSibling && iterNode.nextSibling.theme) {
@@ -73,17 +95,17 @@ add_task(async function() {
 
   let defaultTheme = header.nextSibling;
   defaultTheme.doCommand();
   is(Services.prefs.getCharPref("lightweightThemes.selectedThemeID"), "", "No lwtheme should be selected");
 
   // ensure current theme isn't set to "Default"
   popupShownPromise = popupShown(popup);
   EventUtils.synthesizeMouseAtCenter(themesButton, {});
-  info("Clicked on themes button a second time");
+  info("Clicked on themes button a fourth time");
   await popupShownPromise;
 
   firstLWTheme = recommendedHeader.nextSibling;
   themeChangedPromise = promiseObserverNotified("lightweight-theme-changed");
   firstLWTheme.doCommand();
   info("Clicked on first theme again");
   await themeChangedPromise;
 
@@ -93,17 +115,17 @@ add_task(async function() {
 
   await endCustomizing();
   Services.prefs.setCharPref("lightweightThemes.usedThemes", "[]");
   Services.prefs.setCharPref("lightweightThemes.recommendedThemes", "[]");
   info("Removed all recommended themes");
   await startCustomizing();
   popupShownPromise = popupShown(popup);
   EventUtils.synthesizeMouseAtCenter(themesButton, {});
-  info("Clicked on themes button a second time");
+  info("Clicked on themes button a fifth time");
   await popupShownPromise;
   header = document.getElementById("customization-lwtheme-menu-header");
   is(header.hidden, false, "Header should never be hidden");
   let themeNode = header.nextSibling;
   is(themeNode.theme.id, DEFAULT_THEME_ID, "The first theme should be the Default theme");
   is(themeNode.hidden, false, "The default theme should never be hidden");
 
   themeNode = themeNode.nextSibling;