From e33e4be257439488e9ed72f3dda654183f6e0898 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 20 Jun 2016 14:49:24 +0900 Subject: [PATCH] macOS: Use sheet window as modal window --- atom/browser/api/atom_api_window.cc | 56 +++++++---------------------- atom/browser/api/atom_api_window.h | 6 ---- atom/browser/native_window.cc | 14 +++++--- atom/browser/native_window.h | 19 ++++++---- atom/browser/native_window_mac.h | 6 ---- atom/browser/native_window_mac.mm | 45 +++++++++++++---------- atom/browser/native_window_views.cc | 3 ++ spec/api-browser-window-spec.js | 44 +++++++++-------------- 8 files changed, 81 insertions(+), 112 deletions(-) diff --git a/atom/browser/api/atom_api_window.cc b/atom/browser/api/atom_api_window.cc index 7068360ef91f..bfa56dc5aa17 100644 --- a/atom/browser/api/atom_api_window.cc +++ b/atom/browser/api/atom_api_window.cc @@ -76,8 +76,7 @@ v8::Local ToBuffer(v8::Isolate* isolate, void* val, int size) { } // namespace -Window::Window(v8::Isolate* isolate, const mate::Dictionary& options) - : is_modal_(false) { +Window::Window(v8::Isolate* isolate, const mate::Dictionary& options) { // Use options.webPreferences to create WebContents. mate::Dictionary web_preferences = mate::Dictionary::CreateEmpty(isolate); options.Get(options::kWebPreferences, &web_preferences); @@ -312,6 +311,10 @@ void Window::Show() { } void Window::ShowInactive() { + // This method doesn't make sense for modal window.. + if (IsModal()) + return; + window_->ShowInactive(); } @@ -696,6 +699,11 @@ void Window::SetAspectRatio(double aspect_ratio, mate::Arguments* args) { void Window::SetParentWindow(v8::Local value, mate::Arguments* args) { + if (IsModal()) { + args->ThrowError("Can not be called for modal window"); + return; + } + mate::Handle parent; if (value->IsNull()) { RemoveFromParentChildWindows(); @@ -721,43 +729,8 @@ std::vector> Window::GetChildWindows() const { return child_windows_.Values(isolate()); } -void Window::SetModal(bool modal, mate::Arguments* args) { - if (parent_window_.IsEmpty()) { - args->ThrowError("setModal can only be called for child window"); - return; - } - - mate::Handle parent; - if (!mate::ConvertFromV8(isolate(), GetParentWindow(), &parent)) { - args->ThrowError("Invalid parent window"); // should never happen - return; - } - - if (modal == is_modal_) - return; - - if (modal) - parent->Disable(); - else - parent->Enable(); - window_->SetModal(modal); - is_modal_ = modal; -} - bool Window::IsModal() const { - return is_modal_; -} - -void Window::BeginSheet(mate::Handle sheet, mate::Arguments* args) { - if (sheet->IsVisible()) { - args->ThrowError("Sheet window must not be visible"); - return; - } - window_->BeginSheet(sheet->window_.get()); -} - -void Window::EndSheet(mate::Handle sheet) { - window_->EndSheet(sheet->window_.get()); + return window_->is_modal(); } v8::Local Window::GetNativeWindowHandle() { @@ -794,10 +767,8 @@ void Window::RemoveFromParentChildWindows() { return; parent->child_windows_.Remove(ID()); - if (IsModal()) { + if (IsModal()) parent->Enable(); - is_modal_ = false; - } } // static @@ -828,10 +799,7 @@ void Window::BuildPrototype(v8::Isolate* isolate, .SetMethod("setParentWindow", &Window::SetParentWindow) .SetMethod("getParentWindow", &Window::GetParentWindow) .SetMethod("getChildWindows", &Window::GetChildWindows) - .SetMethod("setModal", &Window::SetModal) .SetMethod("isModal", &Window::IsModal) - .SetMethod("beginSheet", &Window::BeginSheet) - .SetMethod("endSheet", &Window::EndSheet) .SetMethod("getNativeWindowHandle", &Window::GetNativeWindowHandle) .SetMethod("getBounds", &Window::GetBounds) .SetMethod("setBounds", &Window::SetBounds) diff --git a/atom/browser/api/atom_api_window.h b/atom/browser/api/atom_api_window.h index 80785d7673f9..cd8b6ddc42ed 100644 --- a/atom/browser/api/atom_api_window.h +++ b/atom/browser/api/atom_api_window.h @@ -170,10 +170,7 @@ class Window : public mate::TrackableObject, void SetParentWindow(v8::Local value, mate::Arguments* args); v8::Local GetParentWindow() const; std::vector> GetChildWindows() const; - void SetModal(bool modal, mate::Arguments* args); bool IsModal() const; - void BeginSheet(mate::Handle sheet, mate::Arguments* args); - void EndSheet(mate::Handle sheet); v8::Local GetNativeWindowHandle(); #if defined(OS_WIN) @@ -209,9 +206,6 @@ class Window : public mate::TrackableObject, v8::Global parent_window_; KeyWeakMap child_windows_; - // Is current window modal. - bool is_modal_; - api::WebContents* api_web_contents_; std::unique_ptr window_; diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc index f58e026cf494..42ba8d90237f 100644 --- a/atom/browser/native_window.cc +++ b/atom/browser/native_window.cc @@ -46,7 +46,8 @@ namespace atom { NativeWindow::NativeWindow( brightray::InspectableWebContents* inspectable_web_contents, - const mate::Dictionary& options) + const mate::Dictionary& options, + NativeWindow* parent) : content::WebContentsObserver(inspectable_web_contents->GetWebContents()), has_frame_(true), transparent_(false), @@ -57,12 +58,17 @@ NativeWindow::NativeWindow( sheet_offset_y_(0.0), aspect_ratio_(0.0), disable_count_(0), + parent_(parent), + is_modal_(false), inspectable_web_contents_(inspectable_web_contents), weak_factory_(this) { options.Get(options::kFrame, &has_frame_); options.Get(options::kTransparent, &transparent_); options.Get(options::kEnableLargerThanScreen, &enable_larger_than_screen_); + if (parent) + options.Get("modal", &is_modal_); + // Tell the content module to initialize renderer widget with transparent // mode. ui::GpuSwitchingManager::SetTransparent(transparent_); @@ -305,10 +311,8 @@ bool NativeWindow::HasModalDialog() { return has_dialog_attached_; } -void NativeWindow::BeginSheet(NativeWindow* sheet) { -} - -void NativeWindow::EndSheet(NativeWindow* sheet) { +void NativeWindow::SetParentWindow(NativeWindow* parent) { + parent_ = parent; } void NativeWindow::FocusOnWebView() { diff --git a/atom/browser/native_window.h b/atom/browser/native_window.h index 96c10ac5464a..ca1ffe2117e6 100644 --- a/atom/browser/native_window.h +++ b/atom/browser/native_window.h @@ -100,7 +100,7 @@ class NativeWindow : public base::SupportsUserData, virtual bool IsVisible() = 0; virtual void Disable(); virtual void Enable(); - virtual void SetEnabled(bool enable) = 0; // should not be used + virtual void SetEnabled(bool enable) = 0; // internal API, should not be used virtual bool IsEnabled() = 0; virtual void Maximize() = 0; virtual void Unmaximize() = 0; @@ -163,10 +163,7 @@ class NativeWindow : public base::SupportsUserData, virtual void SetFocusable(bool focusable); virtual void SetMenu(ui::MenuModel* menu); virtual bool HasModalDialog(); - virtual void SetParentWindow(NativeWindow* parent) = 0; - virtual void BeginSheet(NativeWindow* sheet); - virtual void EndSheet(NativeWindow* sheet); - virtual void SetModal(bool modal) = 0; + virtual void SetParentWindow(NativeWindow* parent); virtual gfx::NativeWindow GetNativeWindow() = 0; virtual gfx::AcceleratedWidget GetAcceleratedWidget() = 0; @@ -264,9 +261,13 @@ class NativeWindow : public base::SupportsUserData, has_dialog_attached_ = has_dialog_attached; } + NativeWindow* parent() const { return parent_; } + bool is_modal() const { return is_modal_; } + protected: NativeWindow(brightray::InspectableWebContents* inspectable_web_contents, - const mate::Dictionary& options); + const mate::Dictionary& options, + NativeWindow* parent); // Convert draggable regions in raw format to SkRegion format. Caller is // responsible for deleting the returned SkRegion instance. @@ -341,6 +342,12 @@ class NativeWindow : public base::SupportsUserData, // How many times the Disable has been called. int disable_count_; + // The parent window, it is guaranteed to be valid during this window's life. + NativeWindow* parent_; + + // Is this a modal window. + bool is_modal_; + // The page this window is viewing. brightray::InspectableWebContents* inspectable_web_contents_; diff --git a/atom/browser/native_window_mac.h b/atom/browser/native_window_mac.h index 2dcd29806d6d..732a75ee18ad 100644 --- a/atom/browser/native_window_mac.h +++ b/atom/browser/native_window_mac.h @@ -82,9 +82,6 @@ class NativeWindowMac : public NativeWindow { void SetIgnoreMouseEvents(bool ignore) override; bool HasModalDialog() override; void SetParentWindow(NativeWindow* parent) override; - void BeginSheet(NativeWindow* sheet) override; - void EndSheet(NativeWindow* sheet) override; - void SetModal(bool modal) override; gfx::NativeWindow GetNativeWindow() override; gfx::AcceleratedWidget GetAcceleratedWidget() override; void SetProgressBar(double progress) override; @@ -151,9 +148,6 @@ class NativeWindowMac : public NativeWindow { // The "titleBarStyle" option. TitleBarStyle title_bar_style_; - // Whether to hide the native toolbar under fullscreen mode. - bool should_hide_native_toolbar_in_fullscreen_; - DISALLOW_COPY_AND_ASSIGN(NativeWindowMac); }; diff --git a/atom/browser/native_window_mac.mm b/atom/browser/native_window_mac.mm index b49f878eacdd..8a5308b66172 100644 --- a/atom/browser/native_window_mac.mm +++ b/atom/browser/native_window_mac.mm @@ -455,7 +455,7 @@ NativeWindowMac::NativeWindowMac( brightray::InspectableWebContents* web_contents, const mate::Dictionary& options, NativeWindow* parent) - : NativeWindow(web_contents, options), + : NativeWindow(web_contents, options, parent), is_kiosk_(false), attention_request_id_(0), title_bar_style_(NORMAL) { @@ -527,7 +527,8 @@ NativeWindowMac::NativeWindowMac( window_delegate_.reset([[AtomNSWindowDelegate alloc] initWithShell:this]); [window_ setDelegate:window_delegate_]; - if (parent) { + // Only use native parent window for non-modal windows. + if (parent && !is_modal()) { SetParentWindow(parent); } @@ -625,6 +626,12 @@ NativeWindowMac::~NativeWindowMac() { } void NativeWindowMac::Close() { + // When this is a sheet showing, performClose won't work. + if (is_modal() && parent() && IsVisible()) { + CloseImmediately(); + return; + } + if (!IsClosable()) { WindowList::WindowCloseCancelled(this); return; @@ -654,6 +661,12 @@ bool NativeWindowMac::IsFocused() { } void NativeWindowMac::Show() { + if (is_modal() && parent()) { + [parent()->GetNativeWindow() beginSheet:window_ + completionHandler:^(NSModalResponse) {}]; + return; + } + // 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]; @@ -666,6 +679,12 @@ void NativeWindowMac::ShowInactive() { } void NativeWindowMac::Hide() { + if (is_modal() && parent()) { + [window_ orderOut:nil]; + [parent()->GetNativeWindow() endSheet:window_]; + return; + } + [window_ orderOut:nil]; } @@ -679,7 +698,7 @@ void NativeWindowMac::SetEnabled(bool enable) { } bool NativeWindowMac::IsEnabled() { - return ![window_ disableMouseEvents]; + return [window_ attachedSheet] == nil; } void NativeWindowMac::Maximize() { @@ -951,6 +970,11 @@ bool NativeWindowMac::HasModalDialog() { } void NativeWindowMac::SetParentWindow(NativeWindow* parent) { + if (is_modal()) + return; + + NativeWindow::SetParentWindow(parent); + // Remove current parent window. if ([window_ parentWindow]) [[window_ parentWindow] removeChildWindow:window_]; @@ -960,21 +984,6 @@ void NativeWindowMac::SetParentWindow(NativeWindow* parent) { [parent->GetNativeWindow() addChildWindow:window_ ordered:NSWindowAbove]; } -void NativeWindowMac::BeginSheet(NativeWindow* sheet) { - [window_ beginSheet:sheet->GetNativeWindow() - completionHandler:^(NSModalResponse) { - }]; -} - -void NativeWindowMac::EndSheet(NativeWindow* sheet) { - sheet->Hide(); - [window_ endSheet:sheet->GetNativeWindow()]; - sheet->CloseImmediately(); -} - -void NativeWindowMac::SetModal(bool modal) { -} - gfx::NativeWindow NativeWindowMac::GetNativeWindow() { return window_; } diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc index ab317f16d530..8d1421010945 100644 --- a/atom/browser/native_window_views.cc +++ b/atom/browser/native_window_views.cc @@ -806,6 +806,8 @@ void NativeWindowViews::SetMenu(ui::MenuModel* menu_model) { } void NativeWindowViews::SetParentWindow(NativeWindow* parent) { + NativeWindow::SetParentWindow(parent); + #if defined(USE_X11) XDisplay* xdisplay = gfx::GetXDisplay(); XSetTransientForHint( @@ -830,6 +832,7 @@ void NativeWindowViews::SetParentWindow(NativeWindow* parent) { } void NativeWindowViews::SetModal(bool modal) { + NativeWindow::SetModal(modal); #if defined(USE_X11) SetWindowType(GetAcceleratedWidget(), modal ? "dialog" : "normal"); Show(); diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index e9813af5b759..7310cd6a9044 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -902,10 +902,18 @@ describe('browser-window module', function () { }) }) - describe('win.setModal(modal)', function () { + describe('modal option', function () { + // The isEnabled API is not reliable on macOS. + if (process.platform === 'darwin') return + + beforeEach(function () { + if (c != null) c.destroy() + c = new BrowserWindow({show: false, parent: w, modal: true}) + }) + it('disables parent window', function () { assert.equal(w.isEnabled(), true) - c.setModal(true) + c.show() assert.equal(w.isEnabled(), false) }) @@ -914,38 +922,20 @@ describe('browser-window module', function () { assert.equal(w.isEnabled(), true) done() }) - c.setModal(true) + c.show() c.close() }) - it('enables parent window when setting not modal', function () { - assert.equal(w.isEnabled(), true) - c.setModal(true) - assert.equal(w.isEnabled(), false) - c.setModal(false) - assert.equal(w.isEnabled(), true) - }) - - it('enables parent window when removing parent', function () { - if (process.platform !== 'darwin') return - - assert.equal(w.isEnabled(), true) - c.setModal(true) - assert.equal(w.isEnabled(), false) - c.setParentWindow(null) - assert.equal(w.isEnabled(), true) - }) - it('disables parent window recursively', function () { - let c2 = new BrowserWindow({show: false, parent: w}) - c.setModal(true) - c2.setModal(true) + let c2 = new BrowserWindow({show: false, parent: w, modal: true}) + c.show() assert.equal(w.isEnabled(), false) - c.setModal(false) + c2.show() + assert.equal(w.isEnabled(), false) + c.destroy() assert.equal(w.isEnabled(), false) - c2.setModal(false) - assert.equal(w.isEnabled(), true) c2.destroy() + assert.equal(w.isEnabled(), true) }) }) })