Merge pull request #8702 from electron/async-menu-popup

Add async menu.popup option

atom/browser/api/atom_api_menu.cc

@@ -176,7 +176,8 @@ void Menu::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("isItemCheckedAt", &Menu::IsItemCheckedAt)
       .SetMethod("isEnabledAt", &Menu::IsEnabledAt)
       .SetMethod("isVisibleAt", &Menu::IsVisibleAt)
-      .SetMethod("popupAt", &Menu::PopupAt);
+      .SetMethod("popupAt", &Menu::PopupAt)
+      .SetMethod("closePopupAt", &Menu::ClosePopupAt);
 }
 
 }  // namespace api

atom/browser/api/atom_api_menu.h

@@ -53,9 +53,9 @@ class Menu : public mate::TrackableObject<Menu>,
   void ExecuteCommand(int command_id, int event_flags) override;
   void MenuWillShow(ui::SimpleMenuModel* source) override;
 
-  virtual void PopupAt(Window* window,
+  virtual void PopupAt(
-                       int x = -1, int y = -1,
+      Window* window, int x, int y, int positioning_item, bool async) = 0;
-                       int positioning_item = 0) = 0;
+  virtual void ClosePopupAt(int32_t window_id) = 0;
 
   std::unique_ptr<AtomMenuModel> model_;
   Menu* parent_;

atom/browser/api/atom_api_menu_mac.h

@@ -7,10 +7,13 @@
 
 #include "atom/browser/api/atom_api_menu.h"
 
+#include <map>
 #include <string>
 
 #import "atom/browser/ui/cocoa/atom_menu_controller.h"
 
+using base::scoped_nsobject;
+
 namespace atom {
 
 namespace api {
@@ -19,15 +22,25 @@ class MenuMac : public Menu {
  protected:
   MenuMac(v8::Isolate* isolate, v8::Local<v8::Object> wrapper);
 
-  void PopupAt(Window* window, int x, int y, int positioning_item) override;
+  void PopupAt(
-
+      Window* window, int x, int y, int positioning_item, bool async) override;
-  base::scoped_nsobject<AtomMenuController> menu_controller_;
+  void PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
+                 int32_t window_id, int x, int y, int positioning_item,
+                 bool async);
+  void ClosePopupAt(int32_t window_id) override;
 
  private:
   friend class Menu;
 
   static void SendActionToFirstResponder(const std::string& action);
 
+  scoped_nsobject<AtomMenuController> menu_controller_;
+
+  // window ID -> open context menu
+  std::map<int32_t, scoped_nsobject<AtomMenuController>> popup_controllers_;
+
+  base::WeakPtrFactory<MenuMac> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MenuMac);
 };
 

atom/browser/api/atom_api_menu_mac.mm

@@ -6,35 +6,58 @@
 
 #include "atom/browser/native_window.h"
 #include "atom/browser/unresponsive_suppressor.h"
+#include "base/mac/scoped_sending_event.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "brightray/browser/inspectable_web_contents.h"
 #include "brightray/browser/inspectable_web_contents_view.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 
 #include "atom/common/node_includes.h"
 
+using content::BrowserThread;
+
 namespace atom {
 
 namespace api {
 
 MenuMac::MenuMac(v8::Isolate* isolate, v8::Local<v8::Object> wrapper)
-    : Menu(isolate, wrapper) {
+    : Menu(isolate, wrapper),
+      weak_factory_(this) {
 }
 
-void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item) {
+void MenuMac::PopupAt(
+    Window* window, int x, int y, int positioning_item, bool async) {
   NativeWindow* native_window = window->window();
   if (!native_window)
     return;
+
+  auto popup = base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(),
+                          native_window->GetWeakPtr(), window->ID(), x, y,
+                          positioning_item, async);
+  if (async)
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, popup);
+  else
+    popup.Run();
+}
+
+void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
+                        int32_t window_id, int x, int y, int positioning_item,
+                        bool async) {
+  if (!native_window)
+    return;
   brightray::InspectableWebContents* web_contents =
       native_window->inspectable_web_contents();
   if (!web_contents)
     return;
 
-  base::scoped_nsobject<AtomMenuController> menu_controller(
+  auto close_callback = base::Bind(&MenuMac::ClosePopupAt,
-      [[AtomMenuController alloc] initWithModel:model_.get()
+                                   weak_factory_.GetWeakPtr(), window_id);
+  popup_controllers_[window_id] = base::scoped_nsobject<AtomMenuController>(
+      [[AtomMenuController alloc] initWithModel:model()
                           useDefaultAccelerator:NO]);
-  NSMenu* menu = [menu_controller menu];
+  NSMenu* menu = [popup_controllers_[window_id] menu];
   NSView* view = web_contents->GetView()->GetNativeView();
 
   // Which menu item to show.
@@ -69,11 +92,33 @@ void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item) {
   if (rightmostMenuPoint > screenRight)
     position.x = position.x - [menu size].width;
 
-  // Don't emit unresponsive event when showing menu.
-  atom::UnresponsiveSuppressor suppressor;
 
-  // Show the menu.
+  if (async) {
-  [menu popUpMenuPositioningItem:item atLocation:position inView:view];
+    [popup_controllers_[window_id] setCloseCallback:close_callback];
+    // Make sure events can be pumped while the menu is up.
+    base::MessageLoop::ScopedNestableTaskAllower allow(
+        base::MessageLoop::current());
+
+    // One of the events that could be pumped is |window.close()|.
+    // User-initiated event-tracking loops protect against this by
+    // setting flags in -[CrApplication sendEvent:], but since
+    // web-content menus are initiated by IPC message the setup has to
+    // be done manually.
+    base::mac::ScopedSendingEvent sendingEventScoper;
+
+    // Don't emit unresponsive event when showing menu.
+    atom::UnresponsiveSuppressor suppressor;
+    [menu popUpMenuPositioningItem:item atLocation:position inView:view];
+  } else {
+    // Don't emit unresponsive event when showing menu.
+    atom::UnresponsiveSuppressor suppressor;
+    [menu popUpMenuPositioningItem:item atLocation:position inView:view];
+    close_callback.Run();
+  }
+}
+
+void MenuMac::ClosePopupAt(int32_t window_id) {
+  popup_controllers_.erase(window_id);
 }
 
 // static

atom/browser/api/atom_api_menu_views.cc

@@ -8,17 +8,20 @@
 #include "atom/browser/unresponsive_suppressor.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "ui/display/screen.h"
-#include "ui/views/controls/menu/menu_runner.h"
+
+using views::MenuRunner;
 
 namespace atom {
 
 namespace api {
 
 MenuViews::MenuViews(v8::Isolate* isolate, v8::Local<v8::Object> wrapper)
-    : Menu(isolate, wrapper) {
+    : Menu(isolate, wrapper),
+      weak_factory_(this) {
 }
 
-void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
+void MenuViews::PopupAt(
+    Window* window, int x, int y, int positioning_item, bool async) {
   NativeWindow* native_window = static_cast<NativeWindow*>(window->window());
   if (!native_window)
     return;
@@ -38,14 +41,20 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
     location = gfx::Point(origin.x() + x, origin.y() + y);
   }
 
+  int flags = MenuRunner::CONTEXT_MENU | MenuRunner::HAS_MNEMONICS;
+  if (async)
+    flags |= MenuRunner::ASYNC;
+
   // Don't emit unresponsive event when showing menu.
   atom::UnresponsiveSuppressor suppressor;
 
   // Show the menu.
-  views::MenuRunner menu_runner(
+  int32_t window_id = window->ID();
-      model(),
+  auto close_callback = base::Bind(
-      views::MenuRunner::CONTEXT_MENU | views::MenuRunner::HAS_MNEMONICS);
+      &MenuViews::ClosePopupAt, weak_factory_.GetWeakPtr(), window_id);
-  ignore_result(menu_runner.RunMenuAt(
+  menu_runners_[window_id] = std::unique_ptr<MenuRunner>(new MenuRunner(
+      model(), flags, close_callback));
+  ignore_result(menu_runners_[window_id]->RunMenuAt(
       static_cast<NativeWindowViews*>(window->window())->widget(),
       NULL,
       gfx::Rect(location, gfx::Size()),
@@ -53,6 +62,10 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
       ui::MENU_SOURCE_MOUSE));
 }
 
+void MenuViews::ClosePopupAt(int32_t window_id) {
+  menu_runners_.erase(window_id);
+}
+
 // static
 mate::WrappableBase* Menu::New(mate::Arguments* args) {
   return new MenuViews(args->isolate(), args->GetThis());

atom/browser/api/atom_api_menu_views.h

@@ -5,8 +5,12 @@
 #ifndef ATOM_BROWSER_API_ATOM_API_MENU_VIEWS_H_
 #define ATOM_BROWSER_API_ATOM_API_MENU_VIEWS_H_
 
+#include <map>
+
 #include "atom/browser/api/atom_api_menu.h"
+#include "base/memory/weak_ptr.h"
 #include "ui/display/screen.h"
+#include "ui/views/controls/menu/menu_runner.h"
 
 namespace atom {
 
@@ -17,9 +21,16 @@ class MenuViews : public Menu {
   MenuViews(v8::Isolate* isolate, v8::Local<v8::Object> wrapper);
 
  protected:
-  void PopupAt(Window* window, int x, int y, int positioning_item) override;
+  void PopupAt(
+      Window* window, int x, int y, int positioning_item, bool async) override;
+  void ClosePopupAt(int32_t window_id) override;
 
  private:
+  // window ID -> open context menu
+  std::map<int32_t, std::unique_ptr<views::MenuRunner>> menu_runners_;
+
+  base::WeakPtrFactory<MenuViews> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MenuViews);
 };
 

atom/browser/api/atom_api_window.h

@@ -51,6 +51,8 @@ class Window : public mate::TrackableObject<Window>,
 
   NativeWindow* window() const { return window_.get(); }
 
+  int32_t ID() const;
+
  protected:
   Window(v8::Isolate* isolate, v8::Local<v8::Object> wrapper,
          const mate::Dictionary& options);
@@ -202,7 +204,6 @@ class Window : public mate::TrackableObject<Window>,
 
   void SetVibrancy(mate::Arguments* args);
 
-  int32_t ID() const;
   v8::Local<v8::Value> WebContents(v8::Isolate* isolate);
 
   // Remove this window from parent window's |child_windows_|.

atom/browser/ui/cocoa/atom_menu_controller.h

@@ -8,6 +8,7 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include "base/callback.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/strings/string16.h"
 
@@ -27,6 +28,7 @@ class AtomMenuModel;
   base::scoped_nsobject<NSMenu> menu_;
   BOOL isMenuOpen_;
   BOOL useDefaultAccelerator_;
+  base::Callback<void()> closeCallback;
 }
 
 @property(nonatomic, assign) atom::AtomMenuModel* model;
@@ -35,6 +37,8 @@ class AtomMenuModel;
 // to the contents of the model after calling this will not be noticed.
 - (id)initWithModel:(atom::AtomMenuModel*)model useDefaultAccelerator:(BOOL)use;
 
+- (void)setCloseCallback:(const base::Callback<void()>&)callback;
+
 // Populate current NSMenu with |model|.
 - (void)populateWithModel:(atom::AtomMenuModel*)model;
 

atom/browser/ui/cocoa/atom_menu_controller.mm

@@ -12,9 +12,12 @@
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/accelerators/platform_accelerator_cocoa.h"
 #include "ui/base/l10n/l10n_util_mac.h"
+#include "content/public/browser/browser_thread.h"
 #include "ui/events/cocoa/cocoa_event_utils.h"
 #include "ui/gfx/image/image.h"
 
+using content::BrowserThread;
+
 namespace {
 
 struct Role {
@@ -71,6 +74,10 @@ Role kRolesMap[] = {
   [super dealloc];
 }
 
+- (void)setCloseCallback:(const base::Callback<void()>&)callback {
+  closeCallback = callback;
+}
+
 - (void)populateWithModel:(atom::AtomMenuModel*)model {
   if (!menu_)
     return;
@@ -265,8 +272,13 @@ Role kRolesMap[] = {
 
 - (void)menuDidClose:(NSMenu*)menu {
   if (isMenuOpen_) {
-    model_->MenuWillClose();
     isMenuOpen_ = NO;
+    model_->MenuWillClose();
+
+    // Post async task so that itemSelected runs before the close callback
+    // deletes the controller from the map which deallocates it
+    if (!closeCallback.is_null())
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, closeCallback);
   }
 }
 

docs/api/menu.md

@@ -52,17 +52,28 @@ will become properties of the constructed menu items.
 
 The `menu` object has the following instance methods:
 
-#### `menu.popup([browserWindow, x, y, positioningItem])`
+#### `menu.popup([browserWindow, options])`
 
-* `browserWindow` BrowserWindow (optional) - Default is `BrowserWindow.getFocusedWindow()`.
+* `browserWindow` BrowserWindow (optional) - Default is the focused window.
-* `x` Number (optional) - Default is the current mouse cursor position.
+* `options` Object (optional)
-* `y` Number (**required** if `x` is used) - Default is the current mouse cursor position.
+  * `x` Number (optional) - Default is the current mouse cursor position.
-* `positioningItem` Number (optional) _macOS_ - The index of the menu item to
+  * `y` Number (**required** if `x` is used) - Default is the current mouse
-  be positioned under the mouse cursor at the specified coordinates. Default is
+    cursor position.
-  -1.
+  * `async` Boolean (optional) - Set to `true` to have this method return
+    immediately called, `false` to return after the menu has been selected
+    or closed. Defaults to `false`.
+  * `positioningItem` Number (optional) _macOS_ - The index of the menu item to
+    be positioned under the mouse cursor at the specified coordinates. Default
+    is -1.
 
 Pops up this menu as a context menu in the `browserWindow`.
 
+#### `menu.closePopup([browserWindow])`
+
+* `browserWindow` BrowserWindow (optional) - Default is the focused window.
+
+Closes the context menu in the `browserWindow`.
+
 #### `menu.append(menuItem)`
 
 * `menuItem` MenuItem

docs/tutorial/planned-breaking-changes.md

@@ -57,6 +57,15 @@ crashReporter.start({
 })
 ```
 
+## `menu`
+
+```js
+// Deprecated
+menu.popup(browserWindow, 100, 200, 2)
+// Replace with
+menu.popup(browserWindow, {x: 100, y: 200, positioningItem: 2})
+```
+
 ## `nativeImage`
 
 ```js

lib/browser/api/menu.js

@@ -144,6 +144,9 @@ Menu.prototype._init = function () {
 }
 
 Menu.prototype.popup = function (window, x, y, positioningItem) {
+  let asyncPopup = false
+
+  // menu.popup(x, y, positioningItem)
   if (typeof window !== 'object' || window.constructor !== BrowserWindow) {
     // Shift.
     positioningItem = y
@@ -152,6 +155,15 @@ Menu.prototype.popup = function (window, x, y, positioningItem) {
     window = BrowserWindow.getFocusedWindow()
   }
 
+  // menu.popup(window, {})
+  if (x != null && typeof x === 'object') {
+    const options = x
+    x = options.x
+    y = options.y
+    positioningItem = options.positioningItem
+    asyncPopup = options.async
+  }
+
   // Default to showing under mouse location.
   if (typeof x !== 'number') x = -1
   if (typeof y !== 'number') y = -1
@@ -159,7 +171,16 @@ Menu.prototype.popup = function (window, x, y, positioningItem) {
   // Default to not highlighting any item.
   if (typeof positioningItem !== 'number') positioningItem = -1
 
-  this.popupAt(window, x, y, positioningItem)
+  this.popupAt(window, x, y, positioningItem, asyncPopup)
+}
+
+Menu.prototype.closePopup = function (window) {
+  if (window == null || window.constructor !== BrowserWindow) {
+    window = BrowserWindow.getFocusedWindow()
+  }
+  if (window != null) {
+    this.closePopupAt(window.id)
+  }
 }
 
 Menu.prototype.append = function (item) {

spec/api-menu-spec.js

@@ -1,7 +1,8 @@
 const assert = require('assert')
 
 const {ipcRenderer, remote} = require('electron')
-const {Menu, MenuItem} = remote
+const {BrowserWindow, Menu, MenuItem} = remote
+const {closeWindow} = require('./window-helpers')
 
 describe('menu module', function () {
   describe('Menu.buildFromTemplate', function () {
@@ -216,6 +217,30 @@ describe('menu module', function () {
     })
   })
 
+  describe('Menu.popup', function () {
+    let w = null
+
+    afterEach(function () {
+      return closeWindow(w).then(function () { w = null })
+    })
+
+    describe('when called with async: true', function () {
+      it('returns immediately', function () {
+        w = new BrowserWindow({show: false, width: 200, height: 200})
+        const menu = Menu.buildFromTemplate([
+          {
+            label: '1'
+          }, {
+            label: '2'
+          }, {
+            label: '3'
+          }
+        ])
+        menu.popup(w, {x: 100, y: 100, async: true})
+        menu.closePopup(w)
+      })
+    })
+  })
   describe('MenuItem.click', function () {
     it('should be called with the item object passed', function (done) {
       var menu = Menu.buildFromTemplate([
@@ -429,26 +454,26 @@ describe('menu module', function () {
       assert.equal(item.getDefaultRoleAccelerator(), process.platform === 'win32' ? 'Control+Y' : 'Shift+CommandOrControl+Z')
     })
   })
-})
 
-describe('MenuItem with custom properties in constructor', function () {
+  describe('MenuItem with custom properties in constructor', function () {
-  it('preserves the custom properties', function () {
+    it('preserves the custom properties', function () {
-    var template = [{
+      var template = [{
-      label: 'menu 1',
+        label: 'menu 1',
-      customProp: 'foo',
+        customProp: 'foo',
-      submenu: []
+        submenu: []
-    }]
+      }]
 
-    var menu = Menu.buildFromTemplate(template)
+      var menu = Menu.buildFromTemplate(template)
-    menu.items[0].submenu.append(new MenuItem({
+      menu.items[0].submenu.append(new MenuItem({
-      label: 'item 1',
+        label: 'item 1',
-      customProp: 'bar',
+        customProp: 'bar',
-      overrideProperty: 'oops not allowed'
+        overrideProperty: 'oops not allowed'
-    }))
+      }))
 
-    assert.equal(menu.items[0].customProp, 'foo')
+      assert.equal(menu.items[0].customProp, 'foo')
-    assert.equal(menu.items[0].submenu.items[0].label, 'item 1')
+      assert.equal(menu.items[0].submenu.items[0].label, 'item 1')
-    assert.equal(menu.items[0].submenu.items[0].customProp, 'bar')
+      assert.equal(menu.items[0].submenu.items[0].customProp, 'bar')
-    assert.equal(typeof menu.items[0].submenu.items[0].overrideProperty, 'function')
+      assert.equal(typeof menu.items[0].submenu.items[0].overrideProperty, 'function')
+    })
   })
 })