From 40e724e5ddd92879a73fd3eb53af2b128947e9d6 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 31 May 2023 11:57:44 +0200 Subject: [PATCH] fix: DCHECK minimizing parent window with non-modal child (#38460) --- shell/browser/native_window.h | 9 +++ shell/browser/native_window_mac.h | 6 ++ shell/browser/native_window_mac.mm | 52 ++++++++++++------ .../ui/cocoa/electron_ns_window_delegate.mm | 2 + spec/api-browser-window-spec.ts | 55 +++++++++++++++++++ 5 files changed, 107 insertions(+), 17 deletions(-) diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h index 098fa2fcf457..2394be0016f1 100644 --- a/shell/browser/native_window.h +++ b/shell/browser/native_window.h @@ -149,6 +149,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. @@ -382,6 +385,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); @@ -422,6 +429,8 @@ class NativeWindow : public base::SupportsUserData, FullScreenTransitionType fullscreen_transition_type_ = FullScreenTransitionType::kNone; + std::list child_windows_; + private: std::unique_ptr widget_; diff --git a/shell/browser/native_window_mac.h b/shell/browser/native_window_mac.h index b669f4963ec1..e8237460dfbf 100644 --- a/shell/browser/native_window_mac.h +++ b/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(); diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 026f166db9a9..86f8188c5f70 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -472,14 +472,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()) @@ -599,6 +592,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; @@ -1771,18 +1795,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(); } } diff --git a/shell/browser/ui/cocoa/electron_ns_window_delegate.mm b/shell/browser/ui/cocoa/electron_ns_window_delegate.mm index f46707d29d9d..d718cabc9e4f 100644 --- a/shell/browser/ui/cocoa/electron_ns_window_delegate.mm +++ b/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(); } diff --git a/spec/api-browser-window-spec.ts b/spec/api-browser-window-spec.ts index cc0267e1173e..450e503f5c58 100644 --- a/spec/api-browser-window-spec.ts +++ b/spec/api-browser-window-spec.ts @@ -4659,11 +4659,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 }); @@ -4675,6 +4677,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 = once(w, 'show'); + const cShow = once(c, 'show'); + + w.show(); + c.show(); + + await Promise.all([wShow, cShow]); + + const minimized = once(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 = once(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 = once(w, 'show'); + const cShow = once(c, 'show'); + + w.show(); + c.show(); + + await Promise.all([wShow, cShow]); + + const minimized = once(c, 'minimize'); + c.minimize(); + await minimized; + + expect(c.isVisible()).to.be.false('child is visible'); + + const restored = once(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();