Backed out changeset ab219eb65432 (bug 1803660) for causing failures in /browser_newtab_ping.js CLOSED TREE

browser/base/content/tabbrowser.js

@@ -249,8 +249,6 @@
 
     _hoverTabTimer: null,
 
-    _featureCallout: null,
-
     get tabContainer() {
       delete this.tabContainer;
       return (this.tabContainer = document.getElementById("tabbrowser-tabs"));
@@ -324,31 +322,6 @@
       return this._selectedBrowser;
     },
 
-    get featureCallout() {
-      return this._featureCallout;
-    },
-
-    set featureCallout(val) {
-      this._featureCallout = val;
-    },
-
-    get instantiateFeatureCalloutTour() {
-      return this._instantiateFeatureCalloutTour;
-    },
-
-    _instantiateFeatureCalloutTour(location) {
-      // Show Feature Callout in browser chrome when applicable
-      const { FeatureCallout } = ChromeUtils.importESModule(
-        "chrome://browser/content/featureCallout.mjs"
-      );
-      // Note - once we have additional browser chrome messages,
-      // only use PDF.js pref value when navigating to PDF viewer
-      this._featureCallout = new FeatureCallout({
-        win: window,
-        prefName: "browser.pdfjs.feature-tour",
-        source: location.spec,
-      });
-    },
     _setupInitialBrowserAndTab() {
       // See browser.js for the meaning of window.arguments.
       // Bug 1485961 covers making this more sane.
@@ -1087,16 +1060,6 @@
 
       let newTab = this.getTabForBrowser(newBrowser);
 
-      if (this._featureCallout) {
-        this._featureCallout._endTour(true);
-        this._featureCallout = null;
-      }
-
-      if (newBrowser.currentURI.spec.endsWith(".pdf")) {
-        this._instantiateFeatureCalloutTour(newBrowser.currentURI.spec);
-        window.gBrowser.featureCallout.showFeatureCallout();
-      }
-
       if (!aForceUpdate) {
         TelemetryStopwatch.start("FX_TAB_SWITCH_UPDATE_MS");
 
@@ -6836,17 +6799,6 @@
             gBrowser._tabLayerCache.splice(tabCacheIndex, 1);
             gBrowser._getSwitcher().cleanUpTabAfterEviction(this.mTab);
           }
-        } else if (aLocation.spec.endsWith(".pdf")) {
-          // For now, only check for Feature Callout messages
-          // when viewing PDFs. Later, we can expand this to check
-          // for callout messages on every change of tab location.
-          if (window.gBrowser.featureCallout) {
-            window.gBrowser.featureCallout._endTour(true);
-            window.gBrowser.featureCallout = null;
-          }
-
-          window.gBrowser.instantiateFeatureCalloutTour(aLocation);
-          window.gBrowser.featureCallout.showFeatureCallout();
         }
       }
 
@@ -6873,6 +6825,28 @@
         this.mBrowser.lastURI = aLocation;
         this.mBrowser.lastLocationChange = Date.now();
       }
+
+      // For now, only check for Feature Callout messages
+      // when viewing PDFs. Later, we can expand this to check
+      // for callout messages on every change of tab location.
+      if (aLocation.spec.endsWith(".pdf")) {
+        this.showFeatureCalloutIfApplicable(aLocation);
+      }
+    }
+
+    showFeatureCalloutIfApplicable(location) {
+      // Show Feature Callout in browser chrome when applicable
+      const { FeatureCallout } = ChromeUtils.importESModule(
+        "chrome://browser/content/featureCallout.mjs"
+      );
+      // Note - once we have additional browser chrome messages,
+      // only use PDF.js pref value when navigating to PDF viewer
+      let Callout = new FeatureCallout({
+        win: window,
+        prefName: "browser.pdfjs.feature-tour",
+        source: location.spec,
+      });
+      Callout.showFeatureCallout();
     }
 
     onStatusChange(aWebProgress, aRequest, aStatus, aMessage) {

browser/components/firefoxview/featureCallout.mjs

@@ -108,7 +108,7 @@ export class FeatureCallout {
   }
 
   async _handlePrefChange() {
-    if (this.doc.visibilityState === "hidden" || !this.featureTourProgress) {
+    if (this.doc.visibilityState === "hidden") {
       return;
     }
 
@@ -573,7 +573,7 @@ export class FeatureCallout {
     windowFuncs.forEach(func => delete this.win[func]);
   }
 
-  _endTour(skipFadeOut = false) {
+  _endTour() {
     // We don't want focus events that happen during teardown to effect
     // this.savedActiveElement
     this.win.removeEventListener("focus", this.focusHandler, {
@@ -581,31 +581,23 @@ export class FeatureCallout {
     });
     this.win.pageEventManager?.clear();
 
-    // We're deleting featureTourProgress here to ensure that the
-    // reference is freed for garbage collection. This prevents errors
-    // caused by lingering instances when instantiating and removing
-    // multiple feature tour instances in succession.
-    delete this.featureTourProgress;
     this.ready = false;
     // wait for fade out transition
     let container = this.doc.getElementById(CONTAINER_ID);
     container?.classList.add("hidden");
     this._clearWindowFunctions();
-    this.win.setTimeout(
+    this.win.setTimeout(() => {
-      () => {
+      container?.remove();
-        container?.remove();
+      this.renderObserver?.disconnect();
-        this.renderObserver?.disconnect();
+      // Put the focus back to the last place the user focused outside of the
-        // Put the focus back to the last place the user focused outside of the
+      // featureCallout windows.
-        // featureCallout windows.
+      if (this.savedActiveElement) {
-        if (this.savedActiveElement) {
+        this.savedActiveElement.focus({ focusVisible: true });
-          this.savedActiveElement.focus({ focusVisible: true });
+      }
-        }
+    }, TRANSITION_MS);
-      },
-      skipFadeOut ? 0 : TRANSITION_MS
-    );
   }
 
-  async _addScriptsAndRender() {
+  async _addScriptsAndRender(container) {
     const reactSrc = "resource://activity-stream/vendor/react.js";
     const domSrc = "resource://activity-stream/vendor/react-dom.js";
     // Add React script
@@ -613,7 +605,7 @@ export class FeatureCallout {
       return new Promise(resolve => {
         let reactScript = this.doc.createElement("script");
         reactScript.src = reactSrc;
-        this.doc.head.appendChild(reactScript);
+        container.appendChild(reactScript);
         reactScript.addEventListener("load", resolve);
       });
     };
@@ -622,7 +614,7 @@ export class FeatureCallout {
       return new Promise(resolve => {
         let domScript = this.doc.createElement("script");
         domScript.src = domSrc;
-        this.doc.head.appendChild(domScript);
+        container.appendChild(domScript);
         domScript.addEventListener("load", resolve);
       });
     };
@@ -637,7 +629,7 @@ export class FeatureCallout {
     let bundleScript = this.doc.createElement("script");
     bundleScript.src =
       "resource://activity-stream/aboutwelcome/aboutwelcome.bundle.js";
-    this.doc.head.appendChild(bundleScript);
+    container.appendChild(bundleScript);
   }
 
   _observeRender(container) {
@@ -683,7 +675,7 @@ export class FeatureCallout {
     let container = this._createContainer();
     if (container) {
       // This results in rendering the Feature Callout
-      await this._addScriptsAndRender();
+      await this._addScriptsAndRender(container);
       this._observeRender(container);
       this._addPositionListeners();
     }

browser/components/newtab/test/browser/browser_feature_callout_in_chrome.js

@@ -30,57 +30,53 @@ async function openURLInWindow(window, url) {
   await BrowserTestUtils.browserLoaded(selectedBrowser, false, url);
 }
 
-async function openURLInNewTab(window, url) {
-  return BrowserTestUtils.openNewForegroundTab(window.gBrowser, url);
-}
-
 const pdfMatch = sinon.match(val => {
   return (
     val?.id === "featureCalloutCheck" && val?.context?.source === PDF_TEST_URL
   );
 });
 
-const testMessage = {
+add_task(async function feature_callout_renders_in_browser_chrome_for_pdf() {
-  message: {
+  const testMessage = {
-    id: "TEST_MESSAGE",
+    message: {
-    template: "feature_callout",
-    content: {
       id: "TEST_MESSAGE",
-      template: "multistage",
+      template: "feature_callout",
-      backdrop: "transparent",
+      content: {
-      transitions: false,
+        id: "TEST_MESSAGE",
-      screens: [
+        template: "multistage",
-        {
+        backdrop: "transparent",
-          id: "TEST_MESSAGE_1",
+        transitions: false,
-          parent_selector: "#urlbar-container",
+        screens: [
-          content: {
+          {
-            position: "callout",
+            id: "TEST_MESSAGE_1",
-            arrow_position: "top-end",
+            parent_selector: "#urlbar-container",
-            title: {
+            content: {
-              raw: "Test title",
+              position: "callout",
-            },
+              arrow_position: "top-end",
-            subtitle: {
+              title: {
-              raw: "Test subtitle",
+                raw: "Test title",
-            },
-            primary_button: {
-              label: {
-                raw: "Done",
               },
-              action: {
+              subtitle: {
-                navigate: true,
+                raw: "Test subtitle",
+              },
+              primary_button: {
+                label: {
+                  raw: "Done",
+                },
+                action: {
+                  navigate: true,
+                },
               },
             },
           },
-        },
+        ],
-      ],
+      },
+      priority: 1,
+      targeting: "true",
+      trigger: { id: "featureCalloutCheck" },
     },
-    priority: 1,
+  };
-    targeting: "true",
-    trigger: { id: "featureCalloutCheck" },
-  },
-};
 
-add_task(async function feature_callout_renders_in_browser_chrome_for_pdf() {
   const sandbox = sinon.createSandbox();
   const sendTriggerStub = sandbox.stub(ASRouter, "sendTriggerMessage");
   sendTriggerStub.withArgs(pdfMatch).resolves(testMessage);
@@ -107,59 +103,3 @@ add_task(async function feature_callout_renders_in_browser_chrome_for_pdf() {
   await BrowserTestUtils.closeWindow(win);
   sandbox.restore();
 });
-
-add_task(
-  async function feature_callout_renders_and_hides_in_chrome_when_switching_tabs() {
-    const sandbox = sinon.createSandbox();
-    const sendTriggerStub = sandbox.stub(ASRouter, "sendTriggerMessage");
-    sendTriggerStub.withArgs(pdfMatch).resolves(testMessage);
-    sendTriggerStub.callThrough();
-
-    let win = await BrowserTestUtils.openNewBrowserWindow();
-
-    // Test that callout shows up on first PDF tab
-    let doc = win.document;
-    let tab1 = await BrowserTestUtils.openNewForegroundTab(
-      win.gBrowser,
-      PDF_TEST_URL
-    );
-    tab1.focus();
-    await waitForCalloutScreen(doc, testMessage.message.content.screens[0].id);
-    ok(
-      doc.querySelector(`.${testMessage.message.content.screens[0].id}`),
-      "Feature callout rendered when opening a new tab with PDF url"
-    );
-
-    // Test that callout is removed when swapping tabs
-    let tab2 = await openURLInNewTab(win, "about:preferences");
-    tab2.focus();
-    // We hang up on newtab around here
-    await BrowserTestUtils.waitForCondition(() => {
-      return !doc.body.querySelector("#root.featureCallout");
-    });
-
-    ok(
-      !doc.querySelector(`.${testMessage.message.content.screens[0].id}`),
-      "Feature callout removed when tab without PDF URL is navigated to"
-    );
-
-    // Test that callout shows up when tabbing back to pdf
-    let tab3 = await openURLInNewTab(win, PDF_TEST_URL);
-    tab3.focus();
-    await waitForCalloutScreen(doc, testMessage.message.content.screens[0].id);
-    ok(
-      doc.querySelector(`.${testMessage.message.content.screens[0].id}`),
-      "Feature callout still renders when opening a new tab with PDF url after being initially rendered on another tab"
-    );
-
-    // Test that callout remains when tabbing from one pdf tab to another
-    tab1.focus();
-    await waitForCalloutScreen(doc, testMessage.message.content.screens[0].id);
-    ok(
-      doc.querySelector(`.${testMessage.message.content.screens[0].id}`),
-      "Feature callout rendered on original tab after switching tabs multiple times"
-    );
-
-    await BrowserTestUtils.closeWindow(win);
-  }
-);