Bug 1779714 - Defer re-rendering about:addons themes list view when active theme changes while ColorwayCloset modal dialog is open. r=rpl,dao

Differential Revision: https://phabricator.services.mozilla.com/D152127
2022-10-18 14:40:30 +00:00 · 2022-10-18 14:40:30 +00:00 · 42aa4d20ed
--- a/browser/components/colorways/ColorwayClosetOpener.jsm
+++ b/browser/components/colorways/ColorwayClosetOpener.jsm
@ -12,12 +12,15 @@ const { BrowserWindowTracker } = ChromeUtils.import(

 let ColorwayClosetOpener = {
  /**
-   * Opens the colorway closet modal. "source" indicates from where the modal was opened,
-   * and it is used for existing telemetry probes.
+   * Opens the colorway closet modal.
+   * @param {String} source
+   *        Indicates from where the modal was opened, and it is used for existing telemetry probes.
   *        Valid "source" types include: "aboutaddons", "firefoxview" and "unknown" (default).
+   * @param {Function} onClosed
+   *        Function that is called after the modal is closed.
   * @See Events.yaml for existing colorway closet probes
   */
-  openModal: ({ source = "unknown" } = {}) => {
+  openModal: ({ source = "unknown", onClosed = null } = {}) => {
    let { gBrowser } = BrowserWindowTracker.getTopWindow();
    let dialogBox = gBrowser.getTabDialogBox(gBrowser.selectedBrowser);
    return dialogBox.open(
@ -26,7 +29,7 @@ let ColorwayClosetOpener = {
        features: "resizable=no",
        sizeTo: "available",
      },
-      { source }
+      { source, onClosed }
    );
  },
 };
--- a/browser/components/colorways/colorwaycloset.js
+++ b/browser/components/colorways/colorwaycloset.js
@ -284,6 +284,14 @@ const ColorwayCloset = {
            extraKeys: { colorway_id: this.selectedColorway.id },
          });
        }
+
+        // `onClosed` is an additional callback passed upon opening the modal.
+        // Here, `onClosed` is passed by about:addons and is called after the
+        // the modal closes. This is to defer re-rendering the about:addons page
+        // until the user has closed the dialog by setting a new colorways theme
+        // or pressing cancel to revert to the previous theme.
+        const { onClosed } = window?.arguments?.[0] || {};
+        onClosed?.({ colorwayChanged: !this.revertToPreviousTheme });
        break;
    }
  },
--- a/toolkit/mozapps/extensions/content/aboutaddons.js
+++ b/toolkit/mozapps/extensions/content/aboutaddons.js
@ -3787,9 +3787,46 @@ class ColorwayClosetCard extends HTMLElement {
    colorwaysButton.onclick = () => {
      ColorwayClosetOpener.openModal({
        source: "aboutaddons",
+        onClosed: ({ colorwayChanged }) => {
+          ColorwayClosetCard.hasModalOpen = false;
+          ColorwayClosetCard.runPendingModalClosedCallbacks(colorwayChanged);
+        },
      });
+      ColorwayClosetCard.hasModalOpen = true;
    };
  }
+
+  static hasModalOpen = false;
+  static closedModalCallbacks = new Set();
+
+  static callOnModalClosed(fn) {
+    if (!this.hasModalOpen) {
+      try {
+        fn();
+      } catch (err) {
+        Cu.reportError(err);
+      }
+      return;
+    }
+    this.closedModalCallbacks.add(fn);
+  }
+
+  static async runPendingModalClosedCallbacks(colorwayChanged) {
+    // Just drop all pending callbacks if the current active theme didn't change
+    // from when the modal was initially opened to prevent the about:addons page
+    // to be flickering because of the pending callbacks forcing the page from
+    // re-rendering unnecessarily.
+    if (colorwayChanged) {
+      for (const fn of this.closedModalCallbacks) {
+        try {
+          fn();
+        } catch (err) {
+          Cu.reportError(err);
+        }
+      }
+    }
+    this.closedModalCallbacks.clear();
+  }
 }
 customElements.define("colorways-card", ColorwayClosetCard);

@ -4298,12 +4335,34 @@ class AddonList extends HTMLElement {
  }

  updateAddon(addon) {
-    if (!this.getCard(addon)) {
+    if (addon.type === "theme" && ColorwayClosetCard.hasModalOpen) {
+      // Queue up theme card changes while the ColorwayCloset modal is still
+      // open to prevent the about:addons page visible behind the colorway
+      // closet modal from flickering when the list is refreshed in response
+      // to a new colorway theme being selected in the modal dialog.
+      ColorwayClosetCard.callOnModalClosed(() => {
+        this.updateAddon(addon);
+      });
+    } else if (!this.getCard(addon)) {
      // Try to add the add-on right away.
      this.addAddon(addon);
    } else if (this._addonSectionIndex(addon) == -1) {
-      // Try to remove the add-on right away.
+      // If the theme is a colorways theme from an active collection that is
+      // being disabled, it is expected to not be in any section (it will be
+      // available in the colorway closet dialog instead). But, we still want
+      // to defer moving the card until the list is going to lose focus (to
+      // match the same behavior the user expects for any non-colorways theme
+      // card when the theme is disabled).
+      if (
+        addon.type === "theme" &&
+        BuiltInThemes.isColorwayFromCurrentCollection?.(addon.id) &&
+        this.isUserFocused
+      ) {
+        this.updateLater(addon);
+      } else {
+        // Not a colorways theme, fallback to remove the add-on card right away.
        this._updateAddon(addon);
+      }
    } else if (this.isUserFocused) {
      // Queue up a change for when the focus is cleared.
      this.updateLater(addon);
--- a/toolkit/mozapps/extensions/test/browser/browser_colorwaycloset_aboutaddons.js
+++ b/toolkit/mozapps/extensions/test/browser/browser_colorwaycloset_aboutaddons.js
@ -8,6 +8,263 @@ loadTestSubscript(

 AddonTestUtils.initMochitest(this);

+/**
+ * This function behaves similarily to testInColorwayClosetModal, except it is customized for
+ * thoroughly testing about:addons callbacks passed to the Colorway Closet modal.
+ * @param {Boolean} isCancel true if modal is to be closed using the Cancel button, or false if setting a colorway
+ */
+async function testOpenModal(isCancel) {
+  const sinonSandbox = sinon.createSandbox();
+  let win = await loadInitialView("theme");
+  let pageDocument = win.document;
+  let ColorwayClosetCard = win.customElements.get("colorways-card");
+
+  const colorwayOpenModalSpy = sinonSandbox.spy(
+    ColorwayClosetOpener,
+    "openModal"
+  );
+
+  // Set up mock themes
+  const clearBuiltInThemesStubs = initBuiltInThemesStubs();
+  const mockThemes = [
+    SOFT_COLORWAY_THEME_ID,
+    BALANCED_COLORWAY_THEME_ID,
+    BOLD_COLORWAY_THEME_ID,
+  ];
+  const mockThemesAddons = [];
+  const mockThemesUninstall = async () => {
+    for (const addon of mockThemesAddons) {
+      await addon.uninstall();
+    }
+  };
+  registerCleanupFunction(mockThemesUninstall);
+  for (const theme of mockThemes) {
+    const { addon } = await installTestTheme(theme);
+    await addon.disable();
+    mockThemesAddons.push(addon);
+  }
+
+  // Verify that active theme is not tested colorway and not listed as enabled in about:addons.
+  const activeThemeId = Services.prefs.getStringPref(
+    "extensions.activeThemeID"
+  );
+  let enabledSection = getSection(pageDocument, "theme-enabled-section");
+  isnot(
+    activeThemeId,
+    BALANCED_COLORWAY_THEME_ID,
+    "Current Active theme should not be a colorway test theme"
+  );
+  ok(
+    !enabledSection.querySelector(
+      `addon-card[addon-id='${BALANCED_COLORWAY_THEME_ID}']`
+    ),
+    "Soon to be tested colorway should not be in enabled section in about:addons"
+  );
+
+  // Set up Colorway Closet Card spies
+  const callOnModalClosedSpy = sinonSandbox.spy(
+    ColorwayClosetCard,
+    "callOnModalClosed"
+  );
+  const runPendingCallbacksSpy = sinonSandbox.spy(
+    ColorwayClosetCard,
+    "runPendingModalClosedCallbacks"
+  );
+
+  const waitForAddonDisabled = addonId =>
+    AddonTestUtils.promiseAddonEvent("onDisabled", addon => {
+      // Log the onDisabled events to make it easier to investigate unexpected test failures.
+      info(
+        `Seen addon onDisabled addon event for ${addon.id} (waiting for ${addonId})`
+      );
+      return addon.id === addonId;
+    });
+
+  // onDisabled is triggered as a side-effect of enabling a different theme and should
+  // always be called after onEnabled for that theme.
+  const promiseDisabledAfterOpenModal = waitForAddonDisabled(activeThemeId);
+
+  // Open modal
+  info("Open the colorway closet dialog by clicking colorways CTA button");
+  is(
+    ColorwayClosetCard.hasModalOpen,
+    false,
+    "ColorwayClosetCard.hasModalOpen should be false"
+  );
+  const colorwaysButton = pageDocument.querySelector(
+    "[action='open-colorways']"
+  );
+  colorwaysButton.click();
+
+  // Verify callbacks after opening modal
+  is(
+    colorwayOpenModalSpy.callCount,
+    1,
+    "Expect 1 ColorwayClosetOpener.openModal call"
+  );
+  is(
+    ColorwayClosetCard.hasModalOpen,
+    true,
+    "ColorwayClosetCard.hasModalOpen has been updated"
+  );
+
+  // Wait for modal to load
+  const { dialog, closedPromise } = colorwayOpenModalSpy.getCall(0).returnValue;
+  ok(closedPromise instanceof Promise, "Got closedPromise as expected");
+  await dialog._dialogReady;
+  let modalDocument = dialog._frame.contentDocument;
+  let modalWindow = dialog._frame.contentWindow;
+
+  const {
+    cancelButton,
+    colorwayIntensities,
+    setColorwayButton,
+  } = getColorwayClosetTestElements(modalDocument);
+
+  info("Wait for theme update");
+  await promiseDisabledAfterOpenModal;
+
+  const activeColorwayThemeId = Services.prefs.getStringPref(
+    "extensions.activeThemeID"
+  );
+
+  ok(
+    BuiltInThemes.isColorwayFromCurrentCollection(activeColorwayThemeId),
+    `Expect ${activeColorwayThemeId} to be in the active colorway collection`
+  );
+  isnot(
+    activeColorwayThemeId,
+    SOFT_COLORWAY_THEME_ID,
+    "Automatically selected colorway theme should not be the same as the one to be manually selected"
+  );
+
+  // Ensure that about:addons UI is not updated after loading a colorway from the modal.
+  enabledSection = getSection(pageDocument, "theme-enabled-section");
+  ok(
+    !enabledSection.querySelector(
+      `addon-card[addon-id='${activeColorwayThemeId}']`
+    ),
+    "Selected colorway in modal should not be visible in enabled section in about:addons"
+  );
+
+  is(
+    callOnModalClosedSpy.callCount,
+    2,
+    "Expected callOnModalClosed to be called twice"
+  );
+  // Expected number of closedModalCallbacks:
+  // 1. Enabled balanced colorway
+  // 2. Disabled default theme
+  is(
+    ColorwayClosetCard.closedModalCallbacks.size,
+    2,
+    "There should be 2 closedModalCallbacks in total"
+  );
+
+  const promiseDisabledAfterChange = waitForAddonDisabled(
+    activeColorwayThemeId
+  );
+
+  // Wait for intensity button to be initialized
+  await BrowserTestUtils.waitForMutationCondition(
+    colorwayIntensities,
+    { subtree: true, attributeFilter: ["value"] },
+    () =>
+      colorwayIntensities.querySelector(
+        `input[value="${SOFT_COLORWAY_THEME_ID}"]`
+      ),
+    "Waiting for intensity button to be available"
+  );
+
+  info("Changing intensity");
+  colorwayIntensities
+    .querySelector(`input[value="${SOFT_COLORWAY_THEME_ID}"]`)
+    .click();
+  await promiseDisabledAfterChange;
+
+  // Expected number of closedModalCallbacks:
+  // 1. Enabled colorway (automatically selected on modal opened)
+  // 2. Disabled default theme
+  // 3. Enabled soft colorway (manually selected in the modal)
+  // 4. Disabled colorway (automatically selected on modal opened)
+  is(
+    ColorwayClosetCard.closedModalCallbacks.size,
+    4,
+    "There should be 4 closedModalCallbacks in total"
+  );
+
+  // Add a sentinelCallbackSpy to the closedModalCallbacks set to
+  // verify that callbacks are tracked after setting a colorway and
+  // closing the modal.
+  let sentinelCallbackSpy = sinonSandbox.spy();
+  ColorwayClosetCard.closedModalCallbacks.add(sentinelCallbackSpy);
+
+  let modalClosedPromise = BrowserTestUtils.waitForEvent(
+    modalWindow,
+    "unload",
+    "Waiting for modal to close"
+  );
+
+  if (isCancel) {
+    info("Selecting cancel button");
+    cancelButton.click();
+  } else {
+    info("Selecting set colorway button");
+    setColorwayButton.click();
+  }
+  info("Closing modal");
+  await modalClosedPromise;
+  is(
+    ColorwayClosetCard.hasModalOpen,
+    false,
+    "ColorwayClosetCard.hasModalOpen should be false"
+  );
+
+  // ColorwayClosetCard should track remaining pending callbacks.
+  ok(
+    runPendingCallbacksSpy.calledOnce,
+    "Expect ColorwayClosetCard.runPendingModalClosedCallbacks to have been called once"
+  );
+  if (isCancel) {
+    ok(
+      sentinelCallbackSpy.notCalled,
+      "Expect sentinel callback to not be called"
+    );
+  } else {
+    ok(sentinelCallbackSpy.called, "Expect sentinel callback to be called");
+  }
+
+  is(
+    ColorwayClosetCard.closedModalCallbacks.size,
+    0,
+    "There should be 0 closedModalCallbacks in total"
+  );
+
+  // Verify UI again after closing the modal
+  enabledSection = getSection(pageDocument, "theme-enabled-section");
+  if (isCancel) {
+    ok(
+      !enabledSection.querySelector(
+        `addon-card[addon-id='${SOFT_COLORWAY_THEME_ID}']`
+      ),
+      "Selected colorway should not be in enabled section in about:addons"
+    );
+  } else {
+    ok(
+      enabledSection.querySelector(
+        `addon-card[addon-id='${SOFT_COLORWAY_THEME_ID}']`
+      ),
+      "Selected colorway should be in enabled section in about:addons"
+    );
+  }
+
+  await closeView(win);
+  await clearBuiltInThemesStubs();
+  await mockThemesUninstall();
+  sinonSandbox.restore();
+  Services.telemetry.clearEvents();
+}
+
 add_setup(async function() {
  registerMockCollectionL10nIds();
 });
@ -114,7 +371,7 @@ add_task(async function testColorwayClosetPrefEnabled() {
  );

  await closeView(win);
-  await addon.uninstall(true);
+  await addon.uninstall();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
 });
@ -138,7 +395,7 @@ add_task(async function testColorwayClosetSectionPrefDisabled() {
  ok(!colorwaySection, "colorway section should not be found");

  await closeView(win);
-  await addon.uninstall(true);
+  await addon.uninstall();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
 });
@ -174,7 +431,7 @@ add_task(async function testButtonOpenModal() {
  ok(ColorwayClosetOpener.openModal.calledOnce, "Got expected openModal call");

  await closeView(win);
-  await addon.uninstall(true);
+  await addon.uninstall();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
  clearColorwayClosetOpenerStubs();
@ -269,8 +526,8 @@ add_task(async function testColorwayClosetSectionOneRetainedOneUnexpired() {
  );

  await closeView(win);
-  await expiredAddon.uninstall(true);
-  await validAddon.uninstall(true);
+  await expiredAddon.uninstall();
+  await validAddon.uninstall();
  await SpecialPowers.popPrefEnv();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
@ -332,7 +589,7 @@ add_task(async function testColorwayNoActiveCollection() {
  );

  await closeView(win);
-  await expiredAddon.uninstall(true);
+  await expiredAddon.uninstall();
  await SpecialPowers.popPrefEnv();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
@ -406,8 +663,8 @@ add_task(async function testColorwayButtonTextWithColorwayEnabled() {
  await doc.l10n.translateFragment(colorwaySection);

  await closeView(win);
-  await validColorway.uninstall(true);
-  await expiredColorway.uninstall(true);
+  await validColorway.uninstall();
+  await expiredColorway.uninstall();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
 });
@ -450,7 +707,7 @@ add_task(async function test_telemetry_trycolorways_button() {
  );

  await closeView(win);
-  await addon.uninstall(true);
+  await addon.uninstall();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
  Services.telemetry.clearEvents();
@ -494,8 +751,36 @@ add_task(async function test_telemetry_changecolorway_button() {
  );

  await closeView(win);
-  await addon.uninstall(true);
+  await addon.uninstall();
  await SpecialPowers.popPrefEnv();
  clearBuiltInThemesStubs();
  Services.telemetry.clearEvents();
 });
+
+/**
+ * Tests that:
+ * - the about:addons page does not visually update when a colorway selection is made in the modal
+ * - the about:addons page does not visually update if the Cancel button is selected and the modal closes
+ */
+add_task(async function test_aboutaddons_modal_callbacks_onclosed_cancel() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.theme.colorway-closet", true]],
+  });
+  await testOpenModal(true);
+  await SpecialPowers.popPrefEnv();
+});
+
+/**
+ * Tests that:
+ * - the about:addons page does not visually update when a colorway selection is made in the modal
+ * - the about:addons page is visually updated if the Set Colorway button is selected and the modal closes
+ */
+add_task(
+  async function test_aboutaddons_modal_callbacks_onclosed_set_colorway() {
+    await SpecialPowers.pushPrefEnv({
+      set: [["browser.theme.colorway-closet", true]],
+    });
+    await testOpenModal(false);
+    await SpecialPowers.popPrefEnv();
+  }
+);