fix: DCHECK minimizing parent window with non-modal child (#38508)

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

shell/browser/native_window.h

@@ -148,6 +148,9 @@ class NativeWindow : public base::SupportsUserData,
   virtual std::string GetAlwaysOnTopLevel() = 0;
   virtual void SetActive(bool is_key) = 0;
   virtual bool IsActive() const = 0;
+  virtual void RemoveChildWindow(NativeWindow* child) = 0;
+  virtual void AttachChildren() = 0;
+  virtual void DetachChildren() = 0;
 #endif
 
   // Ability to augment the window title for the screen readers.
@@ -381,6 +384,10 @@ class NativeWindow : public base::SupportsUserData,
 
   int32_t window_id() const { return next_id_; }
 
+  void add_child_window(NativeWindow* child) {
+    child_windows_.push_back(child);
+  }
+
   int NonClientHitTest(const gfx::Point& point);
   void AddDraggableRegionProvider(DraggableRegionProvider* provider);
   void RemoveDraggableRegionProvider(DraggableRegionProvider* provider);
@@ -421,6 +428,8 @@ class NativeWindow : public base::SupportsUserData,
   FullScreenTransitionType fullscreen_transition_type_ =
       FullScreenTransitionType::NONE;
 
+  std::list<NativeWindow*> child_windows_;
+
  private:
   std::unique_ptr<views::Widget> widget_;
 

shell/browser/native_window_mac.h

@@ -155,6 +155,12 @@ class NativeWindowMac : public NativeWindow,
   void NotifyWindowLeaveFullScreen() override;
   void SetActive(bool is_key) override;
   bool IsActive() const override;
+  // Remove the specified child window without closing it.
+  void RemoveChildWindow(NativeWindow* child) override;
+  // Attach child windows, if the window is visible.
+  void AttachChildren() override;
+  // Detach window from parent without destroying it.
+  void DetachChildren() override;
 
   void NotifyWindowWillEnterFullScreen();
   void NotifyWindowWillLeaveFullScreen();

shell/browser/native_window_mac.mm

@@ -473,14 +473,7 @@ void NativeWindowMac::Hide() {
     return;
   }
 
-  // Hide all children of the current window before hiding the window.
-  // components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm
-  // expects this when window visibility changes.
-  if ([window_ childWindows]) {
-    for (NSWindow* child in [window_ childWindows]) {
-      [child orderOut:nil];
-    }
-  }
+  DetachChildren();
 
   // Detach the window from the parent before.
   if (parent())
@@ -600,6 +593,37 @@ bool NativeWindowMac::HandleDeferredClose() {
   return false;
 }
 
+void NativeWindowMac::RemoveChildWindow(NativeWindow* child) {
+  child_windows_.remove_if([&child](NativeWindow* w) { return (w == child); });
+
+  [window_ removeChildWindow:child->GetNativeWindow().GetNativeNSWindow()];
+}
+
+void NativeWindowMac::AttachChildren() {
+  for (auto* child : child_windows_) {
+    auto* child_nswindow = child->GetNativeWindow().GetNativeNSWindow();
+    if ([child_nswindow parentWindow] == window_)
+      continue;
+
+    // Attaching a window as a child window resets its window level, so
+    // save and restore it afterwards.
+    NSInteger level = window_.level;
+    [window_ addChildWindow:child_nswindow ordered:NSWindowAbove];
+    [window_ setLevel:level];
+  }
+}
+
+void NativeWindowMac::DetachChildren() {
+  DCHECK(child_windows_.size() == [[window_ childWindows] count]);
+
+  // Hide all children before hiding/minimizing the window.
+  // NativeWidgetNSWindowBridge::NotifyVisibilityChangeDown()
+  // will DCHECK otherwise.
+  for (auto* child : child_windows_) {
+    [child->GetNativeWindow().GetNativeNSWindow() orderOut:nil];
+  }
+}
+
 void NativeWindowMac::SetFullScreen(bool fullscreen) {
   if (!has_frame() && !HasStyleMask(NSWindowStyleMaskTitled))
     return;
@@ -1769,18 +1793,12 @@ void NativeWindowMac::InternalSetParentWindow(NativeWindow* parent,
 
   // Remove current parent window.
   if ([window_ parentWindow])
-    [[window_ parentWindow] removeChildWindow:window_];
+    parent->RemoveChildWindow(this);
 
   // Set new parent window.
-  // Note that this method will force the window to become visible.
   if (parent && attach) {
-    // Attaching a window as a child window resets its window level, so
-    // save and restore it afterwards.
-    NSInteger level = window_.level;
-    [parent->GetNativeWindow().GetNativeNSWindow()
-        addChildWindow:window_
-               ordered:NSWindowAbove];
-    [window_ setLevel:level];
+    parent->add_child_window(this);
+    parent->AttachChildren();
   }
 }
 

shell/browser/ui/cocoa/electron_ns_window_delegate.mm

@@ -239,6 +239,7 @@ using FullScreenTransitionState =
   level_ = [window level];
   shell_->SetWindowLevel(NSNormalWindowLevel);
   shell_->UpdateWindowOriginalFrame();
+  shell_->DetachChildren();
 }
 
 - (void)windowDidMiniaturize:(NSNotification*)notification {
@@ -248,6 +249,7 @@ using FullScreenTransitionState =
 
 - (void)windowDidDeminiaturize:(NSNotification*)notification {
   [super windowDidDeminiaturize:notification];
+  shell_->AttachChildren();
   shell_->SetWindowLevel(level_);
   shell_->NotifyWindowRestore();
 }

spec/api-browser-window-spec.ts

@@ -4249,11 +4249,13 @@ describe('BrowserWindow module', () => {
         const c = new BrowserWindow({ show: false, parent: w });
         expect(c.getParentWindow()).to.equal(w);
       });
+
       it('adds window to child windows of parent', () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false, parent: w });
         expect(w.getChildWindows()).to.deep.equal([c]);
       });
+
       it('removes from child windows of parent when window is closed', async () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false, parent: w });
@@ -4265,6 +4267,59 @@ describe('BrowserWindow module', () => {
         expect(w.getChildWindows().length).to.equal(0);
       });
 
+      ifit(process.platform === 'darwin')('child window matches visibility when visibility changes', async () => {
+        const w = new BrowserWindow({ show: false });
+        const c = new BrowserWindow({ show: false, parent: w });
+
+        const wShow = emittedOnce(w, 'show');
+        const cShow = emittedOnce(c, 'show');
+
+        w.show();
+        c.show();
+
+        await Promise.all([wShow, cShow]);
+
+        const minimized = emittedOnce(w, 'minimize');
+        w.minimize();
+        await minimized;
+
+        expect(w.isVisible()).to.be.false('parent is visible');
+        expect(c.isVisible()).to.be.false('child is visible');
+
+        const restored = emittedOnce(w, 'restore');
+        w.restore();
+        await restored;
+
+        expect(w.isVisible()).to.be.true('parent is visible');
+        expect(c.isVisible()).to.be.true('child is visible');
+      });
+
+      ifit(process.platform === 'darwin')('matches child window visibility when visibility changes', async () => {
+        const w = new BrowserWindow({ show: false });
+        const c = new BrowserWindow({ show: false, parent: w });
+
+        const wShow = emittedOnce(w, 'show');
+        const cShow = emittedOnce(c, 'show');
+
+        w.show();
+        c.show();
+
+        await Promise.all([wShow, cShow]);
+
+        const minimized = emittedOnce(c, 'minimize');
+        c.minimize();
+        await minimized;
+
+        expect(c.isVisible()).to.be.false('child is visible');
+
+        const restored = emittedOnce(c, 'restore');
+        c.restore();
+        await restored;
+
+        expect(w.isVisible()).to.be.true('parent is visible');
+        expect(c.isVisible()).to.be.true('child is visible');
+      });
+
       it('closes a grandchild window when a middle child window is destroyed', (done) => {
         const w = new BrowserWindow();