commit f0664b7a8e76 Author: Gijs Kruitbosch Date: Thu Oct 5 15:17:12 2017 +0100 Bug 1405720 - Ensure only 1 theme is ever shown as selected in customize mode. r=jaws, r=johannh, a=ritu 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 --HG-- extra : source : 4099fa94e3ba6728761655f0e11e7b65d573617d --- .../components/customizableui/CustomizeMode.jsm | 13 ++++------ .../browser_1007336_lwthemes_in_customize_mode.js | 28 +++++++++++++++++++--- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git browser/components/customizableui/CustomizeMode.jsm browser/components/customizableui/CustomizeMode.jsm index b0e31cad63fc..79f23079b1a0 100644 --- browser/components/customizableui/CustomizeMode.jsm +++ browser/components/customizableui/CustomizeMode.jsm @@ -1358,9 +1358,10 @@ CustomizeMode.prototype = { 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); @@ -1374,12 +1375,8 @@ CustomizeMode.prototype = { 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]; diff --git browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js index 3ee8f9b8f697..7b94289c0177 100644 --- browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js +++ browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js @@ -45,9 +45,25 @@ add_task(async function() { 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; @@ -57,6 +73,12 @@ add_task(async function() { 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), @@ -78,7 +100,7 @@ add_task(async function() { // 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; @@ -98,7 +120,7 @@ add_task(async function() { 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");