Handle multiple modal windows correctly

atom/browser/api/atom_api_window.cc

@@ -76,7 +76,9 @@ v8::Local<v8::Value> ToBuffer(v8::Isolate* isolate, void* val, int size) {
 }  // namespace
 
 
-Window::Window(v8::Isolate* isolate, const mate::Dictionary& options) {
+Window::Window(v8::Isolate* isolate, const mate::Dictionary& options)
+    : disable_count_(0),
+      is_modal_(false) {
   // Use options.webPreferences to create WebContents.
   mate::Dictionary web_preferences = mate::Dictionary::CreateEmpty(isolate);
   options.Get(options::kWebPreferences, &web_preferences);
@@ -294,11 +296,15 @@ bool Window::IsVisible() {
 }
 
 void Window::Disable() {
-  window_->Disable();
+  ++disable_count_;
+  if (disable_count_ == 1)
+    window_->Disable();
 }
 
 void Window::Enable() {
-  window_->Enable();
+  --disable_count_;
+  if (disable_count_ == 0)
+    window_->Enable();
 }
 
 bool Window::IsEnabled() {
@@ -680,17 +686,43 @@ void Window::SetParentWindow(v8::Local<v8::Value> value,
   }
 }
 
-v8::Local<v8::Value> Window::GetParentWindow() {
+v8::Local<v8::Value> Window::GetParentWindow() const {
   if (parent_window_.IsEmpty())
     return v8::Null(isolate());
   else
     return v8::Local<v8::Value>::New(isolate(), parent_window_);
 }
 
-std::vector<v8::Local<v8::Object>> Window::GetChildWindows() {
+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();
+  is_modal_ = modal;
+}
+
+bool Window::IsModal() const {
+  return is_modal_;
+}
+
 v8::Local<v8::Value> Window::GetNativeWindowHandle() {
   gfx::AcceleratedWidget handle = window_->GetAcceleratedWidget();
   return ToBuffer(
@@ -721,8 +753,14 @@ void Window::RemoveFromParentChildWindows() {
     return;
 
   mate::Handle<Window> parent;
-  if (mate::ConvertFromV8(isolate(), GetParentWindow(), &parent))
-    parent->child_windows_.Remove(ID());
+  if (!mate::ConvertFromV8(isolate(), GetParentWindow(), &parent))
+    return;
+
+  parent->child_windows_.Remove(ID());
+  if (IsModal()) {
+    parent->Enable();
+    is_modal_ = false;
+  }
 }
 
 // static
@@ -753,6 +791,8 @@ 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("getNativeWindowHandle", &Window::GetNativeWindowHandle)
       .SetMethod("getBounds", &Window::GetBounds)
       .SetMethod("setBounds", &Window::SetBounds)

atom/browser/api/atom_api_window.h

@@ -164,8 +164,10 @@ class Window : public mate::TrackableObject<Window>,
   bool IsMenuBarVisible();
   void SetAspectRatio(double aspect_ratio, mate::Arguments* args);
   void SetParentWindow(v8::Local<v8::Value> value, mate::Arguments* args);
-  v8::Local<v8::Value> GetParentWindow();
-  std::vector<v8::Local<v8::Object>> GetChildWindows();
+  v8::Local<v8::Value> GetParentWindow() const;
+  std::vector<v8::Local<v8::Object>> GetChildWindows() const;
+  void SetModal(bool modal, mate::Arguments* args);
+  bool IsModal() const;
   v8::Local<v8::Value> GetNativeWindowHandle();
 
 #if defined(OS_WIN)
@@ -188,7 +190,7 @@ class Window : public mate::TrackableObject<Window>,
   int32_t ID() const;
   v8::Local<v8::Value> WebContents(v8::Isolate* isolate);
 
-  // Helpers.
+  // Remove this window from parent window's |child_windows_|.
   void RemoveFromParentChildWindows();
 
 #if defined(OS_WIN)
@@ -201,6 +203,12 @@ class Window : public mate::TrackableObject<Window>,
   v8::Global<v8::Value> parent_window_;
   KeyWeakMap<int> child_windows_;
 
+  // How many times the Disable has been called.
+  int disable_count_;
+
+  // Is current window modal.
+  bool is_modal_;
+
   api::WebContents* api_web_contents_;
 
   std::unique_ptr<NativeWindow> window_;

atom/common/key_weak_map.h

@@ -53,7 +53,7 @@ class KeyWeakMap {
   }
 
   // Returns all objects.
-  std::vector<v8::Local<v8::Object>> Values(v8::Isolate* isolate) {
+  std::vector<v8::Local<v8::Object>> Values(v8::Isolate* isolate) const {
     std::vector<v8::Local<v8::Object>> keys;
     keys.reserve(map_.size());
     for (const auto& iter : map_) {

lib/browser/api/browser-window.js

@@ -97,23 +97,6 @@ BrowserWindow.prototype._init = function () {
   })
 }
 
-BrowserWindow.prototype.setModal = function (modal) {
-  const parent = this.getParentWindow()
-  if (!parent) {
-    throw new Error('setModal can only be called for child window')
-  }
-
-  let closeListener = () => parent.enable()
-  if (modal) {
-    parent.disable()
-    this.once('closed', closeListener)
-    this.show()
-  } else {
-    parent.enable()
-    this.removeListener('closed', closeListener)
-  }
-}
-
 BrowserWindow.getFocusedWindow = () => {
   for (let window of BrowserWindow.getAllWindows()) {
     if (window.isFocused()) return window