fix: all children showing when showing child window (#40105)

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

shell/browser/native_window_mac.h

@@ -189,6 +189,9 @@ class NativeWindowMac : public NativeWindow,
     has_deferred_window_close_ = defer_close;
   }
 
+  void set_wants_to_be_visible(bool visible) { wants_to_be_visible_ = visible; }
+  bool wants_to_be_visible() const { return wants_to_be_visible_; }
+
   enum class VisualEffectState {
     kFollowWindow,
     kActive,
@@ -255,6 +258,10 @@ class NativeWindowMac : public NativeWindow,
   // transition is complete.
   bool has_deferred_window_close_ = false;
 
+  // If true, the window is either visible, or wants to be visible but is
+  // currently hidden due to having a hidden parent.
+  bool wants_to_be_visible_ = false;
+
   NSInteger attention_request_id_ = 0;  // identifier from requestUserAttention
 
   // The presentation options before entering kiosk mode.

shell/browser/native_window_mac.mm

@@ -496,6 +496,8 @@ void NativeWindowMac::Show() {
     return;
   }
 
+  set_wants_to_be_visible(true);
+
   // Reattach the window to the parent to actually show it.
   if (parent())
     InternalSetParentWindow(parent(), true);
@@ -528,6 +530,10 @@ void NativeWindowMac::Hide() {
     return;
   }
 
+  // If the window wants to be visible and has a parent, then the parent may
+  // order it back in (in the period between orderOut: and close).
+  set_wants_to_be_visible(false);
+
   DetachChildren();
 
   // Detach the window from the parent before.
@@ -663,6 +669,9 @@ void NativeWindowMac::RemoveChildFromParentWindow() {
 
 void NativeWindowMac::AttachChildren() {
   for (auto* child : child_windows_) {
+    if (!static_cast<NativeWindowMac*>(child)->wants_to_be_visible())
+      continue;
+
     auto* child_nswindow = child->GetNativeWindow().GetNativeNSWindow();
     if ([child_nswindow parentWindow] == window_)
       continue;
@@ -676,8 +685,6 @@ void NativeWindowMac::AttachChildren() {
 }
 
 void NativeWindowMac::DetachChildren() {
-  DCHECK(child_windows_.size() == [[window_ childWindows] count]);
-
   // Hide all children before hiding/minimizing the window.
   // NativeWidgetNSWindowBridge::NotifyVisibilityChangeDown()
   // will DCHECK otherwise.

shell/browser/ui/cocoa/electron_ns_window_delegate.mm

@@ -262,6 +262,7 @@ using FullScreenTransitionState =
   [super windowDidMiniaturize:notification];
   is_minimized_ = true;
 
+  shell_->set_wants_to_be_visible(false);
   shell_->NotifyWindowMinimize();
 }
 
@@ -269,6 +270,7 @@ using FullScreenTransitionState =
   [super windowDidDeminiaturize:notification];
   is_minimized_ = false;
 
+  shell_->set_wants_to_be_visible(true);
   shell_->AttachChildren();
   shell_->SetWindowLevel(level_);
   shell_->NotifyWindowRestore();

spec/api-browser-window-spec.ts

@@ -4725,7 +4725,27 @@ describe('BrowserWindow module', () => {
         expect(w.getChildWindows().length).to.equal(0);
       });
 
-      ifit(process.platform === 'darwin')('child window matches visibility when visibility changes', async () => {
+      ifit(process.platform === 'darwin')('only shows the intended window when a child with siblings is shown', async () => {
+        const w = new BrowserWindow({ show: false });
+        const childOne = new BrowserWindow({ show: false, parent: w });
+        const childTwo = new BrowserWindow({ show: false, parent: w });
+
+        const parentShown = once(w, 'show');
+        w.show();
+        await parentShown;
+
+        expect(childOne.isVisible()).to.be.false('childOne is visible');
+        expect(childTwo.isVisible()).to.be.false('childTwo is visible');
+
+        const childOneShown = once(childOne, 'show');
+        childOne.show();
+        await childOneShown;
+
+        expect(childOne.isVisible()).to.be.true('childOne is not visible');
+        expect(childTwo.isVisible()).to.be.false('childTwo is visible');
+      });
+
+      ifit(process.platform === 'darwin')('child matches parent visibility when parent visibility changes', async () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false, parent: w });
 
@@ -4752,7 +4772,7 @@ describe('BrowserWindow module', () => {
         expect(c.isVisible()).to.be.true('child is visible');
       });
 
-      ifit(process.platform === 'darwin')('matches child window visibility when visibility changes', async () => {
+      ifit(process.platform === 'darwin')('parent matches child visibility when child visibility changes', async () => {
         const w = new BrowserWindow({ show: false });
         const c = new BrowserWindow({ show: false, parent: w });