Bug 1786525 - Don't update untransformed anchor rect when moved by mo…

…ve-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
iaxat · Aug 29, 2022 · 1d87cca · 1d87cca
1 parent e95183b
commit 1d87cca
Show file tree

Hide file tree

Showing 13 changed files with 48 additions and 29 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<AppWindow> holder = mAppWindow;
   return holder->WindowMoved(aWidget, aX, aY);
 }

diff --git 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;