From ee1732e4ffad76be724e879233e67c6a2d18a843 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 5 Aug 2024 12:22:06 +0000 Subject: [PATCH] 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 | 58 +++++++++----------- layout/xul/nsMenuPopupFrame.h | 10 ++-- toolkit/modules/ClipboardContextMenu.sys.mjs | 12 ++-- 3 files changed, 37 insertions(+), 43 deletions(-) diff --git a/layout/xul/nsMenuPopupFrame.cpp b/layout/xul/nsMenuPopupFrame.cpp index ebea8bda4a404..b8e237f74e811 100644 --- a/layout/xul/nsMenuPopupFrame.cpp +++ b/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 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; } diff --git a/layout/xul/nsMenuPopupFrame.h b/layout/xul/nsMenuPopupFrame.h index f44ac74f94465..572fb543114e4 100644 --- a/layout/xul/nsMenuPopupFrame.h +++ b/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 diff --git a/toolkit/modules/ClipboardContextMenu.sys.mjs b/toolkit/modules/ClipboardContextMenu.sys.mjs index d66a2f466d5ba..db81da7b91070 100644 --- a/toolkit/modules/ClipboardContextMenu.sys.mjs +++ b/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); });