Bug 1936194 - Rebuild popups when swapped into another frameloader. r=tnikkel,layout-reviewers

That is, recreate it if we're parented to the wrong widget. This can
happen with swapFrameLoaders.

We could (and should, maybe) do this in SwapFrameLoaders instead. But
we already have logic to re-show widgets on some situations in
nsMenuPopupFrame, so seems worth reusing that.

Fully move parent widget computation to nsMenuPopupFrame.

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

Author: emilio

layout/xul/nsMenuPopupFrame.cpp

@@ -238,15 +238,17 @@ widget::PopupLevel nsMenuPopupFrame::GetPopupLevel(bool aIsNoAutoHide) const {
   return sDefaultLevelIsTop ? PopupLevel::Top : PopupLevel::Parent;
 }
 
-void nsMenuPopupFrame::PrepareWidget(bool aRecreate) {
+void nsMenuPopupFrame::PrepareWidget(bool aForceRecreate) {
   nsView* ourView = GetView();
-  if (aRecreate) {
-    if (auto* widget = GetWidget()) {
+  if (auto* widget = GetWidget()) {
+    nsCOMPtr<nsIWidget> parent = ComputeParentWidget();
+    if (aForceRecreate || widget->GetParent() != parent ||
+        widget->NeedsRecreateToReshow()) {
       // Widget's WebRender resources needs to be cleared before creating new
       // widget.
       widget->ClearCachedWebrenderResources();
+      ourView->DestroyWidget();
     }
-    ourView->DestroyWidget();
   }
   if (!ourView->HasWidget()) {
     CreateWidgetForView(ourView);
@@ -255,6 +257,35 @@ void nsMenuPopupFrame::PrepareWidget(bool aRecreate) {
   }
 }
 
+already_AddRefed<nsIWidget> nsMenuPopupFrame::ComputeParentWidget() const {
+  auto popupLevel = GetPopupLevel(IsNoAutoHide());
+  // Panels which have a parent level need a parent widget. This allows them to
+  // always appear in front of the parent window but behind other windows that
+  // should be in front of it.
+  nsCOMPtr<nsIWidget> parentWidget;
+  if (popupLevel != PopupLevel::Top) {
+    nsCOMPtr<nsIDocShellTreeItem> dsti = PresContext()->GetDocShell();
+    if (!dsti) {
+      return nullptr;
+    }
+
+    nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
+    dsti->GetTreeOwner(getter_AddRefs(treeOwner));
+    if (!treeOwner) {
+      return nullptr;
+    }
+
+    nsCOMPtr<nsIBaseWindow> baseWindow(do_QueryInterface(treeOwner));
+    if (baseWindow) {
+      baseWindow->GetMainWidget(getter_AddRefs(parentWidget));
+    }
+  }
+  if (!parentWidget && mView && mView->GetParent()) {
+    parentWidget = mView->GetParent()->GetNearestWidget(nullptr);
+  }
+  return parentWidget.forget();
+}
+
 nsresult nsMenuPopupFrame::CreateWidgetForView(nsView* aView) {
   // Create a widget for ourselves.
   widget::InitData widgetData;
@@ -273,33 +304,16 @@ nsresult nsMenuPopupFrame::CreateWidgetForView(nsView* aView) {
     }
   }
 
-  bool remote = HasRemoteContent();
+  const bool remote = HasRemoteContent();
 
   const auto mode = nsLayoutUtils::GetFrameTransparency(this, this);
   widgetData.mHasRemoteContent = remote;
   widgetData.mTransparencyMode = mode;
   widgetData.mPopupLevel = GetPopupLevel(widgetData.mNoAutoHide);
 
-  // Panels which have a parent level need a parent widget. This allows them to
-  // always appear in front of the parent window but behind other windows that
-  // should be in front of it.
-  nsCOMPtr<nsIWidget> parentWidget;
-  if (widgetData.mPopupLevel != PopupLevel::Top) {
-    nsCOMPtr<nsIDocShellTreeItem> dsti = PresContext()->GetDocShell();
-    if (!dsti) {
-      return NS_ERROR_FAILURE;
-    }
-
-    nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
-    dsti->GetTreeOwner(getter_AddRefs(treeOwner));
-    if (!treeOwner) {
-      return NS_ERROR_FAILURE;
-    }
-
-    nsCOMPtr<nsIBaseWindow> baseWindow(do_QueryInterface(treeOwner));
-    if (baseWindow) {
-      baseWindow->GetMainWidget(getter_AddRefs(parentWidget));
-    }
+  nsCOMPtr<nsIWidget> parentWidget = ComputeParentWidget();
+  if (NS_WARN_IF(!parentWidget)) {
+    return NS_ERROR_FAILURE;
   }
 
   nsresult rv = aView->CreateWidgetForPopup(&widgetData, parentWidget);
@@ -308,8 +322,9 @@ nsresult nsMenuPopupFrame::CreateWidgetForView(nsView* aView) {
   }
 
   nsIWidget* widget = aView->GetWidget();
-  widget->SetTransparencyMode(mode);
+  MOZ_ASSERT(widget->GetParent() == parentWidget);
 
+  widget->SetTransparencyMode(mode);
   PropagateStyleToWidget();
 
   return NS_OK;
@@ -757,9 +772,7 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
                                        int32_t aXPos, int32_t aYPos,
                                        MenuPopupAnchorType aAnchorType,
                                        bool aAttributesOverride) {
-  auto* widget = GetWidget();
-  bool recreateWidget = widget && widget->NeedsRecreateToReshow();
-  PrepareWidget(recreateWidget);
+  PrepareWidget();
 
   mPopupState = ePopupShowing;
   mAnchorContent = aAnchorContent;
@@ -887,9 +900,7 @@ void nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
 void nsMenuPopupFrame::InitializePopupAtScreen(nsIContent* aTriggerContent,
                                                int32_t aXPos, int32_t aYPos,
                                                bool aIsContextMenu) {
-  auto* widget = GetWidget();
-  bool recreateWidget = widget && widget->NeedsRecreateToReshow();
-  PrepareWidget(recreateWidget);
+  PrepareWidget();
 
   mPopupState = ePopupShowing;
   mAnchorContent = nullptr;
@@ -2080,10 +2091,12 @@ nsresult nsMenuPopupFrame::AttributeChanged(int32_t aNameSpaceID,
     MoveToAttributePosition();
   }
 
-  if (aAttribute == nsGkAtoms::remote) {
+  if (aAttribute == nsGkAtoms::remote && GetWidget()) {
     // When the remote attribute changes, we need to create a new widget to
     // ensure that it has the correct compositor and transparency settings to
-    // match the new value.
+    // match the new value. Do that only if we already have a widget.
+    // TODO(emilio): We should consider doing it only when we get re-shown or
+    // so.
     PrepareWidget(true);
   }
 

layout/xul/nsMenuPopupFrame.h

@@ -173,6 +173,7 @@ class nsMenuPopupFrame final : public nsBlockFrame {
               nsReflowStatus& aStatus) override;
 
   nsIWidget* GetWidget() const;
+  already_AddRefed<nsIWidget> ComputeParentWidget() const;
 
   enum class WidgetStyle : uint8_t {
     ColorScheme,
@@ -213,9 +214,10 @@ class nsMenuPopupFrame final : public nsBlockFrame {
   PopupLevel GetPopupLevel() const { return GetPopupLevel(IsNoAutoHide()); }
 
   // Ensure that a widget has already been created for this view, and create
-  // one if it hasn't. If aRecreate is true, destroys any existing widget and
-  // creates a new one, regardless of whether one has already been created.
-  void PrepareWidget(bool aRecreate = false);
+  // one if it hasn't. If aForceRecreate is true, destroys any existing widget
+  // and creates a new one, regardless of whether one has already been created.
+  // Otherwise does so only if needed.
+  void PrepareWidget(bool aForceRecreate = false);
 
   MOZ_CAN_RUN_SCRIPT void EnsureActiveMenuListItemIsVisible();
 

view/nsView.cpp

@@ -526,19 +526,10 @@ nsresult nsView::CreateWidgetForPopup(widget::InitData* aWidgetInitData,
   MOZ_ASSERT(aWidgetInitData, "Widget init data required");
   MOZ_ASSERT(aWidgetInitData->mWindowType == WindowType::Popup,
              "Use one of the other CreateWidget methods");
+  MOZ_ASSERT(aParent);
 
   LayoutDeviceIntRect trect = CalcWidgetBounds(
       aWidgetInitData->mWindowType, aWidgetInitData->mTransparencyMode);
-
-  if (!aParent && GetParent()) {
-    aParent = GetParent()->GetNearestWidget(nullptr);
-  }
-  if (!aParent) {
-    NS_ERROR("nsView::CreateWidgetForPopup without suitable parent widget??");
-    // Without a parent, we can't make a popup. This used to be able to happen
-    // when printing, apparently.
-    return NS_ERROR_FAILURE;
-  }
   mWindow = aParent->CreateChild(trect, *aWidgetInitData);
   if (!mWindow) {
     return NS_ERROR_FAILURE;