Bug 1927111 - Don't rely on static position (top: auto) for urlbar po…

…sition. r=Gijs,sfoster,desktop-theme-reviewers,urlbar-reviewers,mak, a=dmeehan

The static position of these boxes is not great to depend upon, see:

  w3c/csswg-drafts#9939

Right now, we are relying on the static pos to be properly set which is
going to go away, eventually, and it is bogus, because it doesn't update
if e.g. only transform changes.

Use getBoxQuads to get the content box bounds properly. Also, make
breakout position: absolute explicitly, to prevent confusion.

Differential Revision: https://phabricator.services.mozilla.com/D226932

browser/components/urlbar/UrlbarInput.sys.mjs

@@ -13,6 +13,7 @@ ChromeUtils.defineESModuleGetters(lazy, {
   BrowserSearchTelemetry: "resource:///modules/BrowserSearchTelemetry.sys.mjs",
   BrowserUIUtils: "resource:///modules/BrowserUIUtils.sys.mjs",
   BrowserUtils: "resource://gre/modules/BrowserUtils.sys.mjs",
+  CustomizableUI: "resource:///modules/CustomizableUI.sys.mjs",
   ExtensionSearchHandler:
     "resource://gre/modules/ExtensionSearchHandler.sys.mjs",
   ObjectUtils: "resource://gre/modules/ObjectUtils.sys.mjs",
@@ -264,11 +265,14 @@ export class UrlbarInput {
     // recording abandonment events when the command causes a blur event.
     this.view.panel.addEventListener("command", this, true);
 
+    lazy.CustomizableUI.addListener(this);
+    this.window.addEventListener("unload", this);
+
     this.window.gBrowser.tabContainer.addEventListener("TabSelect", this);
 
     this.window.addEventListener("customizationstarting", this);
     this.window.addEventListener("aftercustomization", this);
-
+    this.window.addEventListener("toolbarvisibilitychange", this);
     const menubar = this.window.document.getElementById("toolbar-menubar");
     if (menubar) {
       menubar.addEventListener("DOMMenuBarInactive", this);
@@ -2120,7 +2124,7 @@ export class UrlbarInput {
       );
       return;
     }
-    await this._updateLayoutBreakoutDimensions();
+    await this.#updateLayoutBreakoutDimensions();
   }
 
   startLayoutExtend() {
@@ -2148,8 +2152,6 @@ export class UrlbarInput {
 
     this.setAttribute("breakout-extend", "true");
 
-    this.textbox.showPopover();
-
     // Enable the animation only after the first extend call to ensure it
     // doesn't run when opening a new window.
     if (!this.hasAttribute("breakout-extend-animate")) {
@@ -2162,7 +2164,6 @@ export class UrlbarInput {
   }
 
   endLayoutExtend() {
-    this.#updateTextboxPosition();
     // If reduce motion is enabled, we want to collapse the Urlbar here so the
     // user sees only sees two states: not expanded, and expanded with the view
     // open.
@@ -2171,6 +2172,7 @@ export class UrlbarInput {
     }
 
     this.removeAttribute("breakout-extend");
+    this.#updateTextboxPosition();
   }
 
   /**
@@ -2388,17 +2390,21 @@ export class UrlbarInput {
   }
 
   #updateTextboxPosition() {
-    if (this.view.isOpen) {
-      // We need to adjust the position of the textbox by measuring its container
-      this.textbox.style.top = px(
-        getBoundsWithoutFlushing(this.textbox.parentNode).top
-      );
-    } else {
+    if (!this.hasAttribute("breakout")) {
       this.textbox.style.top = "";
+      return;
     }
+    // We want to align to the urlbar border box if open, or content box if not.
+    let box = this.view.isOpen ? "border" : "content";
+    this.textbox.style.top = px(
+      this.textbox.parentNode.getBoxQuads({ box, flush: false })[0].p1.y
+    );
   }
 
   #updateTextboxPositionNextFrame() {
+    if (!this.hasAttribute("breakout")) {
+      return;
+    }
     // Allow for any layout changes to take place (e.g. when the menubar becomes
     // inactive) before re-measuring to position the textbox
     this.window.requestAnimationFrame(() => {
@@ -2408,20 +2414,25 @@ export class UrlbarInput {
     });
   }
 
-  async _updateLayoutBreakoutDimensions() {
-    // When this method gets called a second time before the first call
-    // finishes, we need to disregard the first one.
-    let updateKey = {};
-    this._layoutBreakoutUpdateKey = updateKey;
-
+  #stopBreakout() {
     this.removeAttribute("breakout");
+    this.textbox.parentNode.removeAttribute("breakout");
+    this.textbox.style.top = "";
     try {
       this.textbox.hidePopover();
     } catch (ex) {
       // No big deal if not a popover already.
     }
+    this._layoutBreakoutUpdateKey = {};
+  }
 
-    this.textbox.parentNode.removeAttribute("breakout");
+  async #updateLayoutBreakoutDimensions() {
+    this.#stopBreakout();
+
+    // When this method gets called a second time before the first call
+    // finishes, we need to disregard the first one.
+    let updateKey = {};
+    this._layoutBreakoutUpdateKey = updateKey;
 
     await this.window.promiseDocumentFlushed(() => {});
     await new Promise(resolve => {
@@ -2441,6 +2452,8 @@ export class UrlbarInput {
 
         this.setAttribute("breakout", "true");
         this.textbox.parentNode.setAttribute("breakout", "true");
+        this.textbox.showPopover();
+        this.#updateTextboxPosition();
 
         resolve();
       });
@@ -3520,6 +3533,8 @@ export class UrlbarInput {
   _initStripOnShare() {
     let contextMenu = this.querySelector("moz-input-box").menupopup;
     let insertLocation = this.#findMenuItemLocation("cmd_copy");
+    // FIXME(bug 1927220): This check is wrong, !getAttribute() is a
+    // boolean.
     if (!insertLocation.getAttribute("cmd") == "cmd_copy") {
       return;
     }
@@ -4637,27 +4652,49 @@ export class UrlbarInput {
 
   _on_customizationstarting() {
     this.blur();
+    this.#stopBreakout();
 
     this.inputField.controllers.removeController(this._copyCutController);
     delete this._copyCutController;
   }
 
+  // TODO(emilio, bug 1927942): Consider removing this listener and using
+  // onCustomizeEnd.
   _on_aftercustomization() {
+    this.updateLayoutBreakout();
     this._initCopyCutController();
     this._initPasteAndGo();
     this._initStripOnShare();
   }
 
-  _on_DOMMenuBarActive() {
-    if (this.hasAttribute("breakout")) {
-      this.#updateTextboxPositionNextFrame();
+  // CustomizableUI might unbind and bind us again, which makes us lose the
+  // popover state, which this fixes up. This can easily happen outside of
+  // customize mode with a call to CustomizableUI.reset().
+  // TODO(emilio): Do we need some of the on-aftercustomization fixups here?
+  onWidgetAfterDOMChange(aNode) {
+    if (aNode != this.textbox.parentNode || !this.hasAttribute("breakout")) {
+      return;
     }
+    if (!this.textbox.matches(":popover-open")) {
+      this.textbox.showPopover();
+    }
+    this.#updateTextboxPositionNextFrame();
+  }
+
+  _on_unload() {
+    lazy.CustomizableUI.removeListener(this);
+  }
+
+  _on_toolbarvisibilitychange() {
+    this.#updateTextboxPositionNextFrame();
+  }
+
+  _on_DOMMenuBarActive() {
+    this.#updateTextboxPositionNextFrame();
   }
 
   _on_DOMMenuBarInactive() {
-    if (this.hasAttribute("breakout")) {
-      this.#updateTextboxPositionNextFrame();
-    }
+    this.#updateTextboxPositionNextFrame();
   }
 
   #allTextSelectedOnKeyDown = false;

browser/components/urlbar/tests/browser/browser_add_search_engine.js

@@ -291,6 +291,13 @@ add_task(async function context_after_customize() {
     gCustomizeMode.exit();
     await promise;
 
+    // Wait for the urlbar to pop out again before trying to show the context
+    // menu. Otherwise the reframing might hide the context menu (this is a
+    // long-standing XUL issue).
+    await TestUtils.waitForCondition(() => {
+      return window.gURLBar.textbox.hasAttribute("breakout");
+    });
+
     await UrlbarTestUtils.withContextMenu(window, async popup => {
       info("The separator and the add engine item should be present.");
       let elt = popup.parentNode.getMenuItem("add-engine-separator");

browser/themes/shared/urlbar-searchbar.css

@@ -267,6 +267,9 @@
 
 #urlbar[breakout] {
   display: block;
+  /* This is technically not needed, because popover takes care of it, but
+   * helps for clarity. */
+  position: absolute;
   height: var(--urlbar-height);
   width: var(--urlbar-width);