From 1d87cca860c702c795cbf1b188585b57012e7668 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 Aug 2022 21:47:34 +0000 Subject: [PATCH] Bug 1786525 - Don't update untransformed anchor rect when moved by move-to-rect. r=stransky Otherwise, it changes the move-to-rect inputs, which can change the output as well, making us move the anchor all the way to the right. You could, I guess, consider this a mutter bug of sorts, because it feels weird that you pass it an anchor that has been a `move-to-rect` output and you get another rect as an output. But also, it's kinda silly that we're doing that to begin with, so avoid it by telling the popup frame whether it's been positioned / moved by move-to-rect (and keeping the anchor in that case). The reason this works on my setup without "Large text" is just dumb luck (the front-end computes a max-height for the panel that is small enough to fit on the screen). Differential Revision: https://phabricator.services.mozilla.com/D155406 --- layout/xul/nsMenuPopupFrame.cpp | 17 +++++++++++++---- layout/xul/nsMenuPopupFrame.h | 8 +++++++- layout/xul/nsXULPopupManager.cpp | 5 +++-- layout/xul/nsXULPopupManager.h | 4 ++-- view/nsView.cpp | 6 ++++-- view/nsView.h | 3 ++- widget/gtk/nsWindow.cpp | 7 +------ widget/nsBaseWidget.cpp | 5 +++-- widget/nsBaseWidget.h | 5 ++++- widget/nsIWidgetListener.cpp | 4 ++-- widget/nsIWidgetListener.h | 4 +++- xpfe/appshell/AppWindow.cpp | 6 ++---- xpfe/appshell/AppWindow.h | 3 ++- 13 files changed, 48 insertions(+), 29 deletions(-) diff --git a/layout/xul/nsMenuPopupFrame.cpp b/layout/xul/nsMenuPopupFrame.cpp index ab7c306292534..d48599b701242 100644 --- a/layout/xul/nsMenuPopupFrame.cpp +++ b/layout/xul/nsMenuPopupFrame.cpp @@ -819,6 +819,7 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent, mHFlip = false; mAlignmentOffset = 0; mPositionedOffset = 0; + mPositionedByMoveToRect = false; mAnchorType = aAnchorType; @@ -1537,12 +1538,16 @@ nsresult nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame, // tell us which axis the popup is flush against in case we have to move // it around later. The AdjustPositionForAnchorAlign method accounts for // the popup's margin. - mUntransformedAnchorRect = anchorRect; + if (!mPositionedByMoveToRect) { + mUntransformedAnchorRect = anchorRect; + } screenPoint = AdjustPositionForAnchorAlign(anchorRect, hFlip, vFlip); } else { // with no anchor, the popup is positioned relative to the root frame anchorRect = rootScreenRect; - mUntransformedAnchorRect = anchorRect; + if (!mPositionedByMoveToRect) { + mUntransformedAnchorRect = anchorRect; + } screenPoint = anchorRect.TopLeft() + nsPoint(margin.left, margin.top); } @@ -1579,7 +1584,9 @@ nsresult nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame, } else { screenPoint = mScreenRect.TopLeft(); anchorRect = nsRect(screenPoint, nsSize()); - mUntransformedAnchorRect = anchorRect; + if (!mPositionedByMoveToRect) { + mUntransformedAnchorRect = anchorRect; + } // Right-align RTL context menus, and apply margin and offsets as per the // platform conventions. @@ -2408,7 +2415,8 @@ nsMargin nsMenuPopupFrame::GetMargin() const { return margin; } -void nsMenuPopupFrame::MoveTo(const CSSPoint& aPos, bool aUpdateAttrs) { +void nsMenuPopupFrame::MoveTo(const CSSPoint& aPos, bool aUpdateAttrs, + bool aByMoveToRect) { nsIWidget* widget = GetWidget(); nsPoint appUnitsPos = CSSPixel::ToAppUnits(aPos); @@ -2433,6 +2441,7 @@ void nsMenuPopupFrame::MoveTo(const CSSPoint& aPos, bool aUpdateAttrs) { return; } + mPositionedByMoveToRect = aByMoveToRect; mScreenRect.MoveTo(appUnitsPos); if (mAnchorType == MenuPopupAnchorType_Rect) { // This ensures that the anchor width is still honored, to prevent it from diff --git a/layout/xul/nsMenuPopupFrame.h b/layout/xul/nsMenuPopupFrame.h index 19a94cf1c3dc2..66bbb404b52d9 100644 --- a/layout/xul/nsMenuPopupFrame.h +++ b/layout/xul/nsMenuPopupFrame.h @@ -318,7 +318,8 @@ class nsMenuPopupFrame final : public nsBoxFrame, // If aUpdateAttrs is true, and the popup already has left or top attributes, // then those attributes are updated to the new location. // The frame may be destroyed by this method. - void MoveTo(const mozilla::CSSPoint& aPos, bool aUpdateAttrs); + void MoveTo(const mozilla::CSSPoint& aPos, bool aUpdateAttrs, + bool aByMoveToRect = false); void MoveToAnchor(nsIContent* aAnchorContent, const nsAString& aPosition, int32_t aXPos, int32_t aYPos, bool aAttributesOverride); @@ -568,6 +569,11 @@ class nsMenuPopupFrame final : public nsBoxFrame, // without margins applied, as GTK is what takes care of determining how to // flip etc. on Wayland. nsRect mUntransformedAnchorRect; + + // Whether we were moved by the move-to-rect Wayland callback. In that case, + // we stop updating the anchor so that we can end up with a stable position. + bool mPositionedByMoveToRect = false; + // Store SizedToPopup attribute for MoveTo call to avoid // unwanted popup resize there. bool mSizedToPopup = false; diff --git a/layout/xul/nsXULPopupManager.cpp b/layout/xul/nsXULPopupManager.cpp index 2c66a1e0dca57..2a514fbe2de3e 100644 --- a/layout/xul/nsXULPopupManager.cpp +++ b/layout/xul/nsXULPopupManager.cpp @@ -558,7 +558,8 @@ static nsMenuPopupFrame* GetPopupToMoveOrResize(nsIFrame* aFrame) { return menuPopupFrame; } -void nsXULPopupManager::PopupMoved(nsIFrame* aFrame, nsIntPoint aPnt) { +void nsXULPopupManager::PopupMoved(nsIFrame* aFrame, nsIntPoint aPnt, + bool aByMoveToRect) { nsMenuPopupFrame* menuPopupFrame = GetPopupToMoveOrResize(aFrame); if (!menuPopupFrame) { return; @@ -591,7 +592,7 @@ void nsXULPopupManager::PopupMoved(nsIFrame* aFrame, nsIntPoint aPnt) { } else { CSSPoint cssPos = LayoutDeviceIntPoint::FromUnknownPoint(aPnt) / menuPopupFrame->PresContext()->CSSToDevPixelScale(); - menuPopupFrame->MoveTo(cssPos, false); + menuPopupFrame->MoveTo(cssPos, false, aByMoveToRect); } } diff --git a/layout/xul/nsXULPopupManager.h b/layout/xul/nsXULPopupManager.h index c3395b2bb13ef..f50394e21f40f 100644 --- a/layout/xul/nsXULPopupManager.h +++ b/layout/xul/nsXULPopupManager.h @@ -659,9 +659,9 @@ class nsXULPopupManager final : public nsIDOMEventListener, /** * Indicate that the popup associated with aView has been moved to the - * specified screen coordiates. + * specified screen coordinates. */ - void PopupMoved(nsIFrame* aFrame, nsIntPoint aPoint); + void PopupMoved(nsIFrame* aFrame, nsIntPoint aPoint, bool aByMoveToRect); /** * Indicate that the popup associated with aView has been resized to the diff --git a/view/nsView.cpp b/view/nsView.cpp index a1cdea2b67402..28aecf9ff4378 100644 --- a/view/nsView.cpp +++ b/view/nsView.cpp @@ -919,10 +919,12 @@ static bool IsPopupWidget(nsIWidget* aWidget) { PresShell* nsView::GetPresShell() { return GetViewManager()->GetPresShell(); } -bool nsView::WindowMoved(nsIWidget* aWidget, int32_t x, int32_t y) { +bool nsView::WindowMoved(nsIWidget* aWidget, int32_t x, int32_t y, + ByMoveToRect aByMoveToRect) { nsXULPopupManager* pm = nsXULPopupManager::GetInstance(); if (pm && IsPopupWidget(aWidget)) { - pm->PopupMoved(mFrame, nsIntPoint(x, y)); + pm->PopupMoved(mFrame, nsIntPoint(x, y), + aByMoveToRect == ByMoveToRect::Yes); return true; } diff --git a/view/nsView.h b/view/nsView.h index cac279047c663..d45fa678d1529 100644 --- a/view/nsView.h +++ b/view/nsView.h @@ -444,7 +444,8 @@ class nsView final : public nsIWidgetListener { // nsIWidgetListener virtual mozilla::PresShell* GetPresShell() override; virtual nsView* GetView() override { return this; } - virtual bool WindowMoved(nsIWidget* aWidget, int32_t x, int32_t y) override; + virtual bool WindowMoved(nsIWidget* aWidget, int32_t x, int32_t y, + ByMoveToRect) override; virtual bool WindowResized(nsIWidget* aWidget, int32_t aWidth, int32_t aHeight) override; #if defined(MOZ_WIDGET_ANDROID) diff --git a/widget/gtk/nsWindow.cpp b/widget/gtk/nsWindow.cpp index e69f6ba8248f6..4ac8f03e456a2 100644 --- a/widget/gtk/nsWindow.cpp +++ b/widget/gtk/nsWindow.cpp @@ -1891,12 +1891,7 @@ void nsWindow::WaylandPopupPropagateChangesToLayout(bool aMove, bool aResize) { } if (aMove) { LOG(" needPositionUpdate, bounds [%d, %d]", mBounds.x, mBounds.y); - NotifyWindowMoved(mBounds.x, mBounds.y); - if (nsMenuPopupFrame* popupFrame = GetMenuPopupFrame(GetFrame())) { - auto p = CSSIntPoint::Round( - mBounds.TopLeft() / popupFrame->PresContext()->CSSToDevPixelScale()); - popupFrame->MoveTo(p, true); - } + NotifyWindowMoved(mBounds.x, mBounds.y, ByMoveToRect::Yes); } } diff --git a/widget/nsBaseWidget.cpp b/widget/nsBaseWidget.cpp index e0c4b7d63d411..8800c7d5b5622 100644 --- a/widget/nsBaseWidget.cpp +++ b/widget/nsBaseWidget.cpp @@ -1641,9 +1641,10 @@ void nsBaseWidget::NotifyWindowDestroyed() { } } -void nsBaseWidget::NotifyWindowMoved(int32_t aX, int32_t aY) { +void nsBaseWidget::NotifyWindowMoved(int32_t aX, int32_t aY, + ByMoveToRect aByMoveToRect) { if (mWidgetListener) { - mWidgetListener->WindowMoved(this, aX, aY); + mWidgetListener->WindowMoved(this, aX, aY, aByMoveToRect); } if (mIMEHasFocus && IMENotificationRequestsRef().WantPositionChanged()) { diff --git a/widget/nsBaseWidget.h b/widget/nsBaseWidget.h index cf42a3480245e..eb188356a0bdb 100644 --- a/widget/nsBaseWidget.h +++ b/widget/nsBaseWidget.h @@ -355,7 +355,10 @@ class nsBaseWidget : public nsIWidget, public nsSupportsWeakReference { void NotifyWindowDestroyed(); void NotifySizeMoveDone(); - void NotifyWindowMoved(int32_t aX, int32_t aY); + + using ByMoveToRect = nsIWidgetListener::ByMoveToRect; + void NotifyWindowMoved(int32_t aX, int32_t aY, + ByMoveToRect = ByMoveToRect::No); void SetNativeData(uint32_t aDataType, uintptr_t aVal) override {} diff --git a/widget/nsIWidgetListener.cpp b/widget/nsIWidgetListener.cpp index b7f9fb335632f..c96baa4a73091 100644 --- a/widget/nsIWidgetListener.cpp +++ b/widget/nsIWidgetListener.cpp @@ -21,8 +21,8 @@ nsView* nsIWidgetListener::GetView() { return nullptr; } PresShell* nsIWidgetListener::GetPresShell() { return nullptr; } -bool nsIWidgetListener::WindowMoved(nsIWidget* aWidget, int32_t aX, - int32_t aY) { +bool nsIWidgetListener::WindowMoved(nsIWidget* aWidget, int32_t aX, int32_t aY, + ByMoveToRect) { return false; } diff --git a/widget/nsIWidgetListener.h b/widget/nsIWidgetListener.h index 878f3cb6754f4..3a8f5d2dafa4b 100644 --- a/widget/nsIWidgetListener.h +++ b/widget/nsIWidgetListener.h @@ -65,7 +65,9 @@ class nsIWidgetListener { * Called when a window is moved to location (x, y). Returns true if the * notification was handled. Coordinates are outer window screen coordinates. */ - virtual bool WindowMoved(nsIWidget* aWidget, int32_t aX, int32_t aY); + enum class ByMoveToRect : bool { No, Yes }; + virtual bool WindowMoved(nsIWidget* aWidget, int32_t aX, int32_t aY, + ByMoveToRect); /** * Called when a window is resized to (width, height). Returns true if the diff --git a/xpfe/appshell/AppWindow.cpp b/xpfe/appshell/AppWindow.cpp index 19f371d105873..9569d44b1875d 100644 --- a/xpfe/appshell/AppWindow.cpp +++ b/xpfe/appshell/AppWindow.cpp @@ -113,11 +113,8 @@ using dom::AutoNoJSAPI; using dom::BrowserHost; using dom::BrowsingContext; using dom::Document; -using dom::DocumentL10n; using dom::Element; using dom::EventTarget; -using dom::LoadURIOptions; -using dom::Promise; AppWindow::AppWindow(uint32_t aChromeFlags) : mChromeTreeOwner(nullptr), @@ -3330,7 +3327,8 @@ PresShell* AppWindow::WidgetListenerDelegate::GetPresShell() { } bool AppWindow::WidgetListenerDelegate::WindowMoved(nsIWidget* aWidget, - int32_t aX, int32_t aY) { + int32_t aX, int32_t aY, + ByMoveToRect) { RefPtr holder = mAppWindow; return holder->WindowMoved(aWidget, aX, aY); } diff --git a/xpfe/appshell/AppWindow.h b/xpfe/appshell/AppWindow.h index d089f7156715d..4ae07d226ac3d 100644 --- a/xpfe/appshell/AppWindow.h +++ b/xpfe/appshell/AppWindow.h @@ -91,7 +91,8 @@ class AppWindow final : public nsIBaseWindow, MOZ_CAN_RUN_SCRIPT_BOUNDARY virtual mozilla::PresShell* GetPresShell() override; MOZ_CAN_RUN_SCRIPT_BOUNDARY - virtual bool WindowMoved(nsIWidget* aWidget, int32_t x, int32_t y) override; + virtual bool WindowMoved(nsIWidget* aWidget, int32_t x, int32_t y, + ByMoveToRect) override; MOZ_CAN_RUN_SCRIPT_BOUNDARY virtual bool WindowResized(nsIWidget* aWidget, int32_t aWidth, int32_t aHeight) override;