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
--- 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;