Merge pull request #9343 from xaviergonz/fix-osx-child-window-show

Make OSX child windows honor the show option to fix #8836

atom/browser/native_window_mac.h

@@ -158,6 +158,7 @@ class NativeWindowMac : public NativeWindow,
   void UpdateDraggableRegions(
       const std::vector<DraggableRegion>& regions) override;
 
+  void InternalSetParentWindow(NativeWindow* parent, bool attach);
   void ShowWindowButton(NSWindowButton button);
 
   void InstallView();

atom/browser/native_window_mac.mm

@@ -1086,6 +1086,10 @@ void NativeWindowMac::Show() {
     return;
   }
 
+  // Reattach the window to the parent to actually show it.
+  if (parent())
+    InternalSetParentWindow(parent(), true);
+
   // This method is supposed to put focus on window, however if the app does not
   // have focus then "makeKeyAndOrderFront" will only show the window.
   [NSApp activateIgnoringOtherApps:YES];
@@ -1094,6 +1098,10 @@ void NativeWindowMac::Show() {
 }
 
 void NativeWindowMac::ShowInactive() {
+  // Reattach the window to the parent to actually show it.
+  if (parent())
+    InternalSetParentWindow(parent(), true);
+
   [window_ orderFrontRegardless];
 }
 
@@ -1104,6 +1112,10 @@ void NativeWindowMac::Hide() {
     return;
   }
 
+  // Deattach the window from the parent before.
+  if (parent())
+    InternalSetParentWindow(parent(), false);
+
   [window_ orderOut:nil];
 }
 
@@ -1537,18 +1549,7 @@ void NativeWindowMac::SetBrowserView(NativeBrowserView* browser_view) {
 }
 
 void NativeWindowMac::SetParentWindow(NativeWindow* parent) {
-  if (is_modal())
+  InternalSetParentWindow(parent, IsVisible());
-    return;
-
-  NativeWindow::SetParentWindow(parent);
-
-  // Remove current parent window.
-  if ([window_ parentWindow])
-    [[window_ parentWindow] removeChildWindow:window_];
-
-  // Set new current window.
-  if (parent)
-    [parent->GetNativeWindow() addChildWindow:window_ ordered:NSWindowAbove];
 }
 
 gfx::NativeView NativeWindowMac::GetNativeView() const {
@@ -1797,6 +1798,26 @@ void NativeWindowMac::UpdateDraggableRegions(
   UpdateDraggableRegionViews(regions);
 }
 
+void NativeWindowMac::InternalSetParentWindow(NativeWindow* parent, bool attach) {
+  if (is_modal())
+    return;
+
+  NativeWindow::SetParentWindow(parent);
+
+  // Do not remove/add if we are already properly attached.
+  if (attach && parent && [window_ parentWindow] == parent->GetNativeWindow())
+    return;
+
+  // Remove current parent window.
+  if ([window_ parentWindow])
+    [[window_ parentWindow] removeChildWindow:window_];
+
+  // Set new parent window.
+  // Note that this method will force the window to become visible.
+  if (parent && attach)
+    [parent->GetNativeWindow() addChildWindow:window_ ordered:NSWindowAbove];
+}
+
 void NativeWindowMac::ShowWindowButton(NSWindowButton button) {
   auto view = [window_ standardWindowButton:button];
   [view.superview addSubview:view positioned:NSWindowAbove relativeTo:nil];

spec/api-browser-window-spec.js

@@ -1273,8 +1273,12 @@ describe('BrowserWindow module', function () {
         const initialWebContents = webContents.getAllWebContents().map((i) => i.id)
         ipcRenderer.send('prevent-next-new-window', w.webContents.id)
         w.webContents.once('new-window', () => {
-          assert.deepEqual(webContents.getAllWebContents().map((i) => i.id), initialWebContents)
+          // We need to give it some time so the windows get properly disposed (at least on OSX).
+          setTimeout(() => {
+            const currentWebContents = webContents.getAllWebContents().map((i) => i.id)
+            assert.deepEqual(currentWebContents, initialWebContents)
             done()
+          }, 100)
         })
         w.loadURL('file://' + path.join(fixtures, 'pages', 'window-open.html'))
       })
@@ -2365,6 +2369,11 @@ describe('BrowserWindow module', function () {
         })
         c.close()
       })
+
+      it('should not affect the show option', function () {
+        assert.equal(c.isVisible(), false)
+        assert.equal(c.getParentWindow().isVisible(), false)
+      })
     })
 
     describe('win.setParentWindow(parent)', function () {