Bug 1732829 - Propagate popup position / size change after widget moves, not before. r=hiro

Bug 1696718 landed a fix to propagate the position change from layout.
However that's not correct, because the widget isn't resized until
nsView::ProcessPendingUpdatesForView is resized (and even that in some
platforms it might be async).

So the right place to propagate the position change is in
nsXULPopupManager (which we call into from the view system which listens
itself to the widget).

Let's try to enable the test for that bug everywhere with this fixed.

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

browser/components/extensions/test/browser/browser.ini

@@ -201,9 +201,6 @@ https_first_disabled = true
 [browser_ext_popup_select.js]
 skip-if = debug || os != 'win' # FIXME: re-enable on debug build (bug 1442822)
 [browser_ext_popup_select_in_oopif.js]
-skip-if =
-  tsan # bug 1699341
-  !fission # bug 1699367
 [browser_ext_popup_sendMessage.js]
 [browser_ext_popup_shutdown.js]
 [browser_ext_port_disconnect_on_crash.js]

layout/base/nsLayoutUtils.cpp

@@ -2617,10 +2617,11 @@ LayoutDeviceIntPoint nsLayoutUtils::WidgetToWidgetOffset(nsIWidget* aFrom,
   nsIWidget* toRoot;
   LayoutDeviceIntPoint toOffset = GetWidgetOffset(aTo, toRoot);
 
-  if (fromRoot == toRoot) {
-    return fromOffset - toOffset;
+  if (fromRoot != toRoot) {
+    fromOffset = aFrom->WidgetToScreenOffset();
+    toOffset = aTo->WidgetToScreenOffset();
   }
-  return aFrom->WidgetToScreenOffset() - aTo->WidgetToScreenOffset();
+  return fromOffset - toOffset;
 }
 
 nsPoint nsLayoutUtils::TranslateWidgetToView(nsPresContext* aPresContext,

layout/xul/nsMenuPopupFrame.cpp

@@ -1394,22 +1394,6 @@ nsRect nsMenuPopupFrame::ComputeAnchorRect(nsPresContext* aRootPresContext,
       PresContext()->AppUnitsPerDevPixel());
 }
 
-static void NotifyPositionUpdatedForRemoteContents(nsIContent* aContent) {
-  for (nsIContent* content = aContent->GetFirstChild(); content;
-       content = content->GetNextSibling()) {
-    if (content->IsXULElement(nsGkAtoms::browser) &&
-        content->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::remote,
-                                          nsGkAtoms::_true, eIgnoreCase)) {
-      if (dom::BrowserParent* browserParent =
-              dom::BrowserParent::GetFrom(content)) {
-        browserParent->NotifyPositionUpdatedForContentsInPopup();
-      }
-    } else {
-      NotifyPositionUpdatedForRemoteContents(content);
-    }
-  }
-}
-
 nsresult nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame,
                                             bool aIsMove, bool aSizedToPopup) {
   if (!mShouldAutoPosition) return NS_OK;
@@ -1752,15 +1736,42 @@ nsresult nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame,
     }
   }
 
+  // NOTE(emilio): This call below is kind of a workaround, but we need to do
+  // this here because some position changes don't go through the
+  // view system -> popup manager, like:
+  //
+  //   https://searchfox.org/mozilla-central/rev/477950cf9ca9c9bb5ff6f34e0d0f6ca4718ea798/widget/gtk/nsWindow.cpp#3847
+  //
+  // So this might be the last chance we have to set the remote browser's
+  // position.
+  //
+  // Ultimately this probably wants to get fixed in the widget size of things,
+  // but given this is worst-case a redundant DOM traversal, and that popups
+  // usually don't have all that much content, this is probably an ok
+  // workaround.
+  WidgetPositionOrSizeDidChange();
+
+  return NS_OK;
+}
+
+void nsMenuPopupFrame::WidgetPositionOrSizeDidChange() {
   // In the case this popup has remote contents having OOP iframes, it's
   // possible that OOP iframe's nsSubDocumentFrame has been already reflowed
   // thus, we will never have a chance to tell this parent browser's position
   // update to the OOP documents without notifying it explicitly.
-  if (HasRemoteContent()) {
-    NotifyPositionUpdatedForRemoteContents(mContent);
+  if (!HasRemoteContent()) {
+    return;
+  }
+  for (nsIContent* content = mContent->GetFirstChild(); content;
+       content = content->GetNextNode(mContent)) {
+    if (content->IsXULElement(nsGkAtoms::browser) &&
+        content->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::remote,
+                                          nsGkAtoms::_true, eIgnoreCase)) {
+      if (auto* browserParent = dom::BrowserParent::GetFrom(content)) {
+        browserParent->NotifyPositionUpdatedForContentsInPopup();
+      }
+    }
   }
-
-  return NS_OK;
 }
 
 void nsMenuPopupFrame::GenerateFrames() {

layout/xul/nsMenuPopupFrame.h

@@ -528,6 +528,8 @@ class nsMenuPopupFrame final : public nsBoxFrame,
   int GetPopupAnchor() { return mPopupAnchor; }
   FlipType GetFlipType() { return mFlip; }
 
+  void WidgetPositionOrSizeDidChange();
+
  protected:
   nsString mIncrementalString;  // for incremental typing navigation
 

layout/xul/nsXULPopupManager.cpp

@@ -555,10 +555,16 @@ static nsMenuPopupFrame* GetPopupToMoveOrResize(nsIFrame* aFrame) {
 
 void nsXULPopupManager::PopupMoved(nsIFrame* aFrame, nsIntPoint aPnt) {
   nsMenuPopupFrame* menuPopupFrame = GetPopupToMoveOrResize(aFrame);
-  if (!menuPopupFrame) return;
+  if (!menuPopupFrame) {
+    return;
+  }
 
   nsView* view = menuPopupFrame->GetView();
-  if (!view) return;
+  if (!view) {
+    return;
+  }
+
+  menuPopupFrame->WidgetPositionOrSizeDidChange();
 
   // Don't do anything if the popup is already at the specified location. This
   // prevents recursive calls when a popup is positioned.
@@ -587,15 +593,22 @@ void nsXULPopupManager::PopupMoved(nsIFrame* aFrame, nsIntPoint aPnt) {
 void nsXULPopupManager::PopupResized(nsIFrame* aFrame,
                                      LayoutDeviceIntSize aSize) {
   nsMenuPopupFrame* menuPopupFrame = GetPopupToMoveOrResize(aFrame);
-  if (!menuPopupFrame) return;
+  if (!menuPopupFrame) {
+    return;
+  }
+
+  menuPopupFrame->WidgetPositionOrSizeDidChange();
 
   nsView* view = menuPopupFrame->GetView();
-  if (!view) return;
+  if (!view) {
+    return;
+  }
 
   LayoutDeviceIntRect curDevSize = view->CalcWidgetBounds(eWindowType_popup);
   // If the size is what we think it is, we have nothing to do.
-  if (curDevSize.width == aSize.width && curDevSize.height == aSize.height)
+  if (curDevSize.width == aSize.width && curDevSize.height == aSize.height) {
     return;
+  }
 
   Element* popup = menuPopupFrame->GetContent()->AsElement();