Bug 1901769 - Make nsMenuPopupFrame offset handling simpler. r=edgar,…

…NeilDeakin,TYLin

* Make mXPos/mYPos work in terms of margin (mExtraMargin).
 * When we have no anchor, explicitly upgrade them to anchored-to-point.

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

layout/xul/nsMenuPopupFrame.cpp

@@ -759,9 +759,22 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
 
   mPopupState = ePopupShowing;
   mAnchorContent = aAnchorContent;
+  mAnchorType = aAnchorType;
+  const nscoord auPerCssPx = AppUnitsPerCSSPixel();
+  const nsPoint pos = CSSPixel::ToAppUnits(CSSIntPoint(aXPos, aYPos));
+  // When converted back to CSSIntRect it is (-1, -1, 0, 0) - as expected in
+  // nsXULPopupManager::Rollup
+  mScreenRect = nsRect(-auPerCssPx, -auPerCssPx, 0, 0);
+  mExtraMargin = pos;
+  // If we have no anchor node, anchor to the given position instead.
+  if (mAnchorType == MenuPopupAnchorType::Node && !aAnchorContent) {
+    mAnchorType = MenuPopupAnchorType::Point;
+    mScreenRect = nsRect(
+        pos + PresShell()->GetRootFrame()->GetScreenRectInAppUnits().TopLeft(),
+        nsSize());
+    mExtraMargin = {};
+  }
   mTriggerContent = aTriggerContent;
-  mXPos = aXPos;
-  mYPos = aYPos;
   mIsNativeMenu = false;
   mIsTopLevelContextMenu = false;
   mVFlip = false;
@@ -771,8 +784,6 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
   mPositionedOffset = 0;
   mPositionedByMoveToRect = false;
 
-  mAnchorType = aAnchorType;
-
   // if aAttributesOverride is true, then the popupanchor, popupalign and
   // position attributes on the <menupopup> override those values passed in.
   // If false, those attributes are only used if the values passed in are empty
@@ -787,8 +798,6 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
       // the offset is used to adjust the position from the anchor point
       if (anchor.IsEmpty() && align.IsEmpty() && position.IsEmpty())
         position.Assign(aPosition);
-      else
-        mXPos = mYPos = 0;
     } else if (!aPosition.IsEmpty()) {
       position.Assign(aPosition);
     }
@@ -846,9 +855,6 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
       InitPositionFromAnchorAlign(anchor, align);
     }
   }
-  // When converted back to CSSIntRect it is (-1, -1, 0, 0) - as expected in
-  // nsXULPopupManager::Rollup
-  mScreenRect = nsRect(-AppUnitsPerCSSPixel(), -AppUnitsPerCSSPixel(), 0, 0);
 
   if (aAttributesOverride) {
     // Use |left| and |top| dimension attributes to position the popup if
@@ -884,9 +890,8 @@ void nsMenuPopupFrame::InitializePopupAtScreen(nsIContent* aTriggerContent,
   mAnchorContent = nullptr;
   mTriggerContent = aTriggerContent;
   mScreenRect =
-      nsRect(CSSPixel::ToAppUnits(aXPos), CSSPixel::ToAppUnits(aYPos), 0, 0);
-  mXPos = 0;
-  mYPos = 0;
+      nsRect(CSSPixel::ToAppUnits(CSSIntPoint(aXPos, aYPos)), nsSize());
+  mExtraMargin = {};
   mFlip = FlipFromAttribute(this);
   mPopupAnchor = POPUPALIGNMENT_NONE;
   mPopupAlignment = POPUPALIGNMENT_NONE;
@@ -905,9 +910,8 @@ void nsMenuPopupFrame::InitializePopupAsNativeContextMenu(
   mPopupState = ePopupShowing;
   mAnchorContent = nullptr;
   mScreenRect =
-      nsRect(CSSPixel::ToAppUnits(aXPos), CSSPixel::ToAppUnits(aYPos), 0, 0);
-  mXPos = 0;
-  mYPos = 0;
+      nsRect(CSSPixel::ToAppUnits(CSSIntPoint(aXPos, aYPos)), nsSize());
+  mExtraMargin = {};
   mFlip = FlipType_Default;
   mPopupAnchor = POPUPALIGNMENT_NONE;
   mPopupAlignment = POPUPALIGNMENT_NONE;
@@ -1446,7 +1450,7 @@ auto nsMenuPopupFrame::GetRects(const nsSize& aPrefSize) const -> Rects {
       // mAnchorContent might be a different document so its presshell must be
       // used.
       nsIFrame* anchorFrame = GetAnchorFrame();
-      if (!anchorFrame) {
+      if (NS_WARN_IF(!anchorFrame)) {
         return rootScreenRect;
       }
       return ComputeAnchorRect(rootPc, anchorFrame);
@@ -1470,20 +1474,6 @@ auto nsMenuPopupFrame::GetRects(const nsSize& aPrefSize) const -> Rects {
       result.mUsedRect.MoveTo(result.mAnchorRect.TopLeft() +
                               nsPoint(margin.left, margin.top));
     }
-
-    // mXPos and mYPos specify an additional offset passed to OpenPopup that
-    // should be added to the position.  We also add the offset to the anchor
-    // pos so a later flip/resize takes the offset into account.
-    // FIXME(emilio): Wayland doesn't seem to be accounting for this offset
-    // anywhere, and it probably should.
-    {
-      nsPoint offset(CSSPixel::ToAppUnits(mXPos), CSSPixel::ToAppUnits(mYPos));
-      if (IsDirectionRTL()) {
-        offset.x = -offset.x;
-      }
-      result.mUsedRect.MoveBy(offset);
-      result.mAnchorRect.MoveBy(offset);
-    }
   } else {
     // Not anchored, use mScreenRect
     result.mUsedRect.MoveTo(mScreenRect.TopLeft());
@@ -2170,6 +2160,13 @@ nsMargin nsMenuPopupFrame::GetMargin() const {
     margin.top += auOffset;
     margin.bottom += auOffset;
   }
+  margin.top += mExtraMargin.y;
+  margin.left += mExtraMargin.x;
+  // TODO(emilio): We should consider make these properly mirrored (that is,
+  // changing -= to += here), but some tests rely on the old behavior of the
+  // anchor moving physically to the bottom / right regardless of alignment...
+  margin.bottom -= mExtraMargin.y;
+  margin.right -= mExtraMargin.x;
   return margin;
 }
 
@@ -2209,7 +2206,6 @@ void nsMenuPopupFrame::MoveTo(const CSSPoint& aPos, bool aUpdateAttrs,
     // appUnitsPos.
     mPopupAlignment = POPUPALIGNMENT_TOPLEFT;
     mPopupAnchor = POPUPALIGNMENT_BOTTOMLEFT;
-    mXPos = mYPos = 0;
   } else {
     mAnchorType = MenuPopupAnchorType::Point;
   }

layout/xul/nsMenuPopupFrame.h

@@ -565,11 +565,11 @@ class nsMenuPopupFrame final : public nsBlockFrame {
   // would be before resizing. Computations are performed using this size.
   nsSize mPrefSize{-1, -1};
 
-  // The position of the popup, in CSS pixels.
-  // The screen coordinates, if set to values other than -1,
-  // override mXPos and mYPos.
-  int32_t mXPos = 0;
-  int32_t mYPos = 0;
+  // A point with extra offsets to apply in the horizontal and vertical axes. We
+  // don't use an nsMargin because the values would be the same for the same
+  // axis.
+  nsPoint mExtraMargin;
+
   nsRect mScreenRect;
   // Used for store rectangle which the popup is going to be anchored to, we
   // need that for Wayland. It's important that this rect is unflipped, and

toolkit/modules/ClipboardContextMenu.sys.mjs

@@ -135,13 +135,11 @@ export var ClipboardContextMenu = {
       // on the same `_menupopup` object. If the popup is already open,
       // `openPopup` is a no-op. When the popup is clicked or dismissed both
       // actor parents will receive the corresponding event.
-      this._menupopup.openPopup(
-        null,
-        "overlap" /* options */,
-        mouseXInCSSPixels.value,
-        mouseYInCSSPixels.value,
-        true /* isContextMenu */
-      );
+      this._menupopup.openPopup(null, {
+        isContextMenu: true,
+        x: mouseXInCSSPixels.value,
+        y: mouseYInCSSPixels.value,
+      });
 
       this._refreshDelayTimer(document);
     });