macOS: Use sheet window as modal window

diff --git a/atom/browser/api/atom_api_window.cc b/atom/browser/api/atom_api_window.cc
index 7068360..bfa56dc 100644 (file)
--- a/atom/browser/api/atom_api_window.cc
+++ b/atom/browser/api/atom_api_window.cc
@@ -76,8 +76,7 @@ v8::Local<v8::Value> 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<v8::Value> value,
                              mate::Arguments* args) {
+  if (IsModal()) {
+    args->ThrowError("Can not be called for modal window");
+    return;
+  }
+
   mate::Handle<Window> parent;
   if (value->IsNull()) {
     RemoveFromParentChildWindows();
@@ -721,43 +729,8 @@ std::vector<v8::Local<v8::Object>> 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<Window> 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<Window> 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<Window> sheet) {
-  window_->EndSheet(sheet->window_.get());
+  return window_->is_modal();
 }
 
 v8::Local<v8::Value> 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 80785d7..cd8b6dd 100644 (file)
--- 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<Window>,
   void SetParentWindow(v8::Local<v8::Value> value, mate::Arguments* args);
   v8::Local<v8::Value> GetParentWindow() const;
   std::vector<v8::Local<v8::Object>> GetChildWindows() const;
-  void SetModal(bool modal, mate::Arguments* args);
   bool IsModal() const;
-  void BeginSheet(mate::Handle<Window> sheet, mate::Arguments* args);
-  void EndSheet(mate::Handle<Window> sheet);
   v8::Local<v8::Value> GetNativeWindowHandle();
 
 #if defined(OS_WIN)
@@ -209,9 +206,6 @@ class Window : public mate::TrackableObject<Window>,
   v8::Global<v8::Value> parent_window_;
   KeyWeakMap<int> child_windows_;
 
-  // Is current window modal.
-  bool is_modal_;
-
   api::WebContents* api_web_contents_;
 
   std::unique_ptr<NativeWindow> window_;

diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc
index f58e026..42ba8d9 100644 (file)
--- 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 96c10ac..ca1ffe2 100644 (file)
--- 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 2dcd298..732a75e 100644 (file)
--- 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 b49f878..8a5308b 100644 (file)
--- 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 ab317f1..8d14210 100644 (file)
--- 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 e9813af..7310cd6 100644 (file)
--- 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)
       })
     })
   })