From 636ef0fd29b7351422732017ff6378c0ca55fc0e Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 15 Feb 2017 11:44:25 -0800 Subject: [PATCH 01/16] Add async menu.popup on macOS --- atom/browser/api/atom_api_menu_mac.h | 4 +++ atom/browser/api/atom_api_menu_mac.mm | 36 +++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index d318b7bdc05..4539efb26ce 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -20,6 +20,8 @@ class MenuMac : public Menu { MenuMac(v8::Isolate* isolate, v8::Local wrapper); void PopupAt(Window* window, int x, int y, int positioning_item) override; + void PopupOnUI(const base::WeakPtr& native_window, + int x, int y, int positioning_item); base::scoped_nsobject menu_controller_; @@ -28,6 +30,8 @@ class MenuMac : public Menu { static void SendActionToFirstResponder(const std::string& action); + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(MenuMac); }; diff --git a/atom/browser/api/atom_api_menu_mac.mm b/atom/browser/api/atom_api_menu_mac.mm index 9e901d10980..ed4dae372ee 100644 --- a/atom/browser/api/atom_api_menu_mac.mm +++ b/atom/browser/api/atom_api_menu_mac.mm @@ -6,10 +6,12 @@ #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" @@ -19,13 +21,24 @@ namespace atom { namespace api { MenuMac::MenuMac(v8::Isolate* isolate, v8::Local wrapper) - : Menu(isolate, wrapper) { + : Menu(isolate, wrapper), + weak_factory_(this) { } void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item) { NativeWindow* native_window = window->window(); if (!native_window) return; + + content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, + base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(), + native_window->GetWeakPtr(), x, y, positioning_item)); +} + +void MenuMac::PopupOnUI(const base::WeakPtr& native_window, + int x, int y, int positioning_item) { + if (!native_window) + return; brightray::InspectableWebContents* web_contents = native_window->inspectable_web_contents(); if (!web_contents) @@ -69,11 +82,24 @@ 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; + { + // Make sure events can be pumped while the menu is up. + base::MessageLoop::ScopedNestableTaskAllower allow( + base::MessageLoop::current()); - // Show the menu. - [menu popUpMenuPositioningItem:item atLocation:position inView:view]; + // 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; + + // Show the menu. + [menu popUpMenuPositioningItem:item atLocation:position inView:view]; + } } // static From b091d104f5a3b997210ebdb991652f938f576d2a Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 15 Feb 2017 12:44:39 -0800 Subject: [PATCH 02/16] Add async menu.popup on Windows/Linux --- atom/browser/api/atom_api_menu_views.cc | 17 ++++++++++++----- atom/browser/api/atom_api_menu_views.h | 6 ++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 8a72247c9d0..7a5edb96214 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -8,14 +8,16 @@ #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 wrapper) - : Menu(isolate, wrapper) { + : Menu(isolate, wrapper), + weak_factory_(this) { } void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) { @@ -42,10 +44,11 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) { atom::UnresponsiveSuppressor suppressor; // Show the menu. - views::MenuRunner menu_runner( + menu_runner_.reset(new MenuRunner( model(), - views::MenuRunner::CONTEXT_MENU | views::MenuRunner::HAS_MNEMONICS); - ignore_result(menu_runner.RunMenuAt( + MenuRunner::CONTEXT_MENU | MenuRunner::HAS_MNEMONICS | MenuRunner::ASYNC, + base::Bind(&MenuViews::OnMenuClosed, weak_factory_.GetWeakPtr()))); + ignore_result(menu_runner_->RunMenuAt( static_cast(window->window())->widget(), NULL, gfx::Rect(location, gfx::Size()), @@ -53,6 +56,10 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) { ui::MENU_SOURCE_MOUSE)); } +void MenuViews::OnMenuClosed() { + menu_runner_.reset(); +} + // static mate::WrappableBase* Menu::New(mate::Arguments* args) { return new MenuViews(args->isolate(), args->GetThis()); diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index 1e7abd1372e..c17dee35362 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -6,7 +6,9 @@ #define ATOM_BROWSER_API_ATOM_API_MENU_VIEWS_H_ #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 { @@ -18,8 +20,12 @@ class MenuViews : public Menu { protected: void PopupAt(Window* window, int x, int y, int positioning_item) override; + void OnMenuClosed(); private: + std::unique_ptr menu_runner_; + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(MenuViews); }; From 4430927f98877f8b11d784ee7d7d9629466cff31 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 15 Feb 2017 15:57:36 -0800 Subject: [PATCH 03/16] Add async option to menu.popup --- atom/browser/api/atom_api_menu.h | 5 ++--- atom/browser/api/atom_api_menu_mac.h | 5 +++-- atom/browser/api/atom_api_menu_mac.mm | 26 ++++++++++++++++++-------- atom/browser/api/atom_api_menu_views.h | 3 ++- lib/browser/api/menu.js | 14 +++++++++++++- 5 files changed, 38 insertions(+), 15 deletions(-) diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 97b63600492..772b2f3c088 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -53,9 +53,8 @@ class Menu : public mate::TrackableObject, void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; - virtual void PopupAt(Window* window, - int x = -1, int y = -1, - int positioning_item = 0) = 0; + virtual void PopupAt(Window* window, int x, int y, int positioning_item, + bool async) = 0; std::unique_ptr model_; Menu* parent_; diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index 4539efb26ce..0c5c7f7f98c 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -19,9 +19,10 @@ class MenuMac : public Menu { protected: MenuMac(v8::Isolate* isolate, v8::Local 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; void PopupOnUI(const base::WeakPtr& native_window, - int x, int y, int positioning_item); + int x, int y, int positioning_item, bool async); base::scoped_nsobject menu_controller_; diff --git a/atom/browser/api/atom_api_menu_mac.mm b/atom/browser/api/atom_api_menu_mac.mm index ed4dae372ee..84a6b8d9df5 100644 --- a/atom/browser/api/atom_api_menu_mac.mm +++ b/atom/browser/api/atom_api_menu_mac.mm @@ -16,6 +16,8 @@ #include "atom/common/node_includes.h" +using content::BrowserThread; + namespace atom { namespace api { @@ -25,18 +27,23 @@ MenuMac::MenuMac(v8::Isolate* isolate, v8::Local 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; - content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, - base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(), - native_window->GetWeakPtr(), x, y, positioning_item)); + auto popup = base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(), + native_window->GetWeakPtr(), x, y, positioning_item, + async); + if (async) + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, popup); + else + popup.Run(); } void MenuMac::PopupOnUI(const base::WeakPtr& native_window, - int x, int y, int positioning_item) { + int x, int y, int positioning_item, bool async) { if (!native_window) return; brightray::InspectableWebContents* web_contents = @@ -82,7 +89,8 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, if (rightmostMenuPoint > screenRight) position.x = position.x - [menu size].width; - { + + if (async) { // Make sure events can be pumped while the menu is up. base::MessageLoop::ScopedNestableTaskAllower allow( base::MessageLoop::current()); @@ -96,8 +104,10 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, // Don't emit unresponsive event when showing menu. atom::UnresponsiveSuppressor suppressor; - - // Show the menu. + [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]; } } diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index c17dee35362..5e5dc8e93dc 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -19,7 +19,8 @@ class MenuViews : public Menu { MenuViews(v8::Isolate* isolate, v8::Local 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 OnMenuClosed(); private: diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 167e9a744ec..e4fda6d5b07 100644 --- a/lib/browser/api/menu.js +++ b/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 (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,7 @@ 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.append = function (item) { From 66b6b4f1cb5f8c73b81561284262a24d059ea86a Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 15 Feb 2017 16:07:46 -0800 Subject: [PATCH 04/16] Map async option to MenuRunner::ASYNC flag --- atom/browser/api/atom_api_menu_views.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 7a5edb96214..3be2a17a279 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -20,7 +20,8 @@ MenuViews::MenuViews(v8::Isolate* isolate, v8::Local 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(window->window()); if (!native_window) return; @@ -40,13 +41,17 @@ 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. menu_runner_.reset(new MenuRunner( model(), - MenuRunner::CONTEXT_MENU | MenuRunner::HAS_MNEMONICS | MenuRunner::ASYNC, + flags, base::Bind(&MenuViews::OnMenuClosed, weak_factory_.GetWeakPtr()))); ignore_result(menu_runner_->RunMenuAt( static_cast(window->window())->widget(), From d686cf77e985b129035a91b7ada79226d298b544 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 08:34:39 -0800 Subject: [PATCH 05/16] Update menu.popup docs to take options object --- docs/api/menu.md | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/docs/api/menu.md b/docs/api/menu.md index b9bdc360648..6969c4457eb 100644 --- a/docs/api/menu.md +++ b/docs/api/menu.md @@ -52,14 +52,20 @@ 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()`. -* `x` Number (optional) - Default is the current mouse cursor position. -* `y` Number (**required** if `x` is used) - Default is the current mouse cursor position. -* `positioningItem` Number (optional) _macOS_ - The index of the menu item to - be positioned under the mouse cursor at the specified coordinates. Default is - -1. +* `browserWindow` BrowserWindow (optional) - Default is + `BrowserWindow.getFocusedWindow()`. +* `options` Object (optional) + * `x` Number (optional) - Default is the current mouse cursor position. + * `y` Number (**required** if `x` is used) - Default is the current mouse + cursor position. + * `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`. From 55f90b4a4bcdfdb7f02482ec95fb32d9250a76ac Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 08:35:01 -0800 Subject: [PATCH 06/16] Add new menu.popup signature to planned breaking changes --- docs/tutorial/planned-breaking-changes.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/tutorial/planned-breaking-changes.md b/docs/tutorial/planned-breaking-changes.md index 57505d0b66f..b5771feea5c 100644 --- a/docs/tutorial/planned-breaking-changes.md +++ b/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 From 947556a23f91449244d25044b2d7e8c7275c4fe8 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 09:14:31 -0800 Subject: [PATCH 07/16] Move MenuItem spec into root describe --- spec/api-menu-spec.js | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index bf5c9871cd3..0c3cb50e4d4 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -429,26 +429,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 () { - it('preserves the custom properties', function () { - var template = [{ - label: 'menu 1', - customProp: 'foo', - submenu: [] - }] + describe('MenuItem with custom properties in constructor', function () { + it('preserves the custom properties', function () { + var template = [{ + label: 'menu 1', + customProp: 'foo', + submenu: [] + }] - var menu = Menu.buildFromTemplate(template) - menu.items[0].submenu.append(new MenuItem({ - label: 'item 1', - customProp: 'bar', - overrideProperty: 'oops not allowed' - })) + var menu = Menu.buildFromTemplate(template) + menu.items[0].submenu.append(new MenuItem({ + label: 'item 1', + customProp: 'bar', + overrideProperty: 'oops not allowed' + })) - 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].customProp, 'bar') - assert.equal(typeof menu.items[0].submenu.items[0].overrideProperty, 'function') + 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].customProp, 'bar') + assert.equal(typeof menu.items[0].submenu.items[0].overrideProperty, 'function') + }) }) }) From 6a023dc4fe43605a8a03453f62005d0ce7fcdb22 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 10:58:02 -0800 Subject: [PATCH 08/16] Add Menu.closePopup API on macOS --- atom/browser/api/atom_api_menu.cc | 3 ++- atom/browser/api/atom_api_menu.h | 1 + atom/browser/api/atom_api_menu_mac.h | 10 +++++++-- atom/browser/api/atom_api_menu_mac.mm | 21 +++++++++++++------ atom/browser/api/atom_api_window.h | 3 ++- atom/browser/ui/cocoa/atom_menu_controller.h | 4 ++++ atom/browser/ui/cocoa/atom_menu_controller.mm | 8 ++++++- lib/browser/api/menu.js | 9 ++++++++ 8 files changed, 48 insertions(+), 11 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 627ce601f54..da208b36591 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/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 diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 772b2f3c088..59312e19a76 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -55,6 +55,7 @@ class Menu : public mate::TrackableObject, virtual void PopupAt(Window* window, int x, int y, int positioning_item, bool async) = 0; + virtual void ClosePopupAt(int32_t window_id) = 0; std::unique_ptr model_; Menu* parent_; diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index 0c5c7f7f98c..7c41cd73e4b 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -7,10 +7,13 @@ #include "atom/browser/api/atom_api_menu.h" +#include #include #import "atom/browser/ui/cocoa/atom_menu_controller.h" +using base::scoped_nsobject; + namespace atom { namespace api { @@ -22,9 +25,12 @@ class MenuMac : public Menu { void PopupAt( Window* window, int x, int y, int positioning_item, bool async) override; void PopupOnUI(const base::WeakPtr& native_window, - int x, int y, int positioning_item, bool async); + int32_t window_id, int x, int y, int positioning_item, + bool async); + void ClosePopupAt(int32_t window_id) override; - base::scoped_nsobject menu_controller_; + scoped_nsobject menu_controller_; + std::map> popup_controllers_; private: friend class Menu; diff --git a/atom/browser/api/atom_api_menu_mac.mm b/atom/browser/api/atom_api_menu_mac.mm index 84a6b8d9df5..9741202dd72 100644 --- a/atom/browser/api/atom_api_menu_mac.mm +++ b/atom/browser/api/atom_api_menu_mac.mm @@ -34,8 +34,8 @@ void MenuMac::PopupAt( return; auto popup = base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(), - native_window->GetWeakPtr(), x, y, positioning_item, - async); + native_window->GetWeakPtr(), window->ID(), x, y, + positioning_item, async); if (async) BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, popup); else @@ -43,7 +43,8 @@ void MenuMac::PopupAt( } void MenuMac::PopupOnUI(const base::WeakPtr& native_window, - int x, int y, int positioning_item, bool async) { + int32_t window_id, int x, int y, int positioning_item, + bool async) { if (!native_window) return; brightray::InspectableWebContents* web_contents = @@ -51,10 +52,12 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, if (!web_contents) return; - base::scoped_nsobject menu_controller( - [[AtomMenuController alloc] initWithModel:model_.get() + auto close_callback = base::Bind(&MenuMac::ClosePopupAt, + weak_factory_.GetWeakPtr(), window_id); + popup_controllers_[window_id] = base::scoped_nsobject( + [[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. @@ -91,6 +94,7 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, if (async) { + [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()); @@ -109,9 +113,14 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, // 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 void Menu::SetApplicationMenu(Menu* base_menu) { MenuMac* menu = static_cast(base_menu); diff --git a/atom/browser/api/atom_api_window.h b/atom/browser/api/atom_api_window.h index 9908feedbcd..80af78a5b03 100644 --- a/atom/browser/api/atom_api_window.h +++ b/atom/browser/api/atom_api_window.h @@ -51,6 +51,8 @@ class Window : public mate::TrackableObject, NativeWindow* window() const { return window_.get(); } + int32_t ID() const; + protected: Window(v8::Isolate* isolate, v8::Local wrapper, const mate::Dictionary& options); @@ -202,7 +204,6 @@ class Window : public mate::TrackableObject, void SetVibrancy(mate::Arguments* args); - int32_t ID() const; v8::Local WebContents(v8::Isolate* isolate); // Remove this window from parent window's |child_windows_|. diff --git a/atom/browser/ui/cocoa/atom_menu_controller.h b/atom/browser/ui/cocoa/atom_menu_controller.h index af0b2769618..a230437f536 100644 --- a/atom/browser/ui/cocoa/atom_menu_controller.h +++ b/atom/browser/ui/cocoa/atom_menu_controller.h @@ -8,6 +8,7 @@ #import +#include "base/callback.h" #include "base/mac/scoped_nsobject.h" #include "base/strings/string16.h" @@ -27,6 +28,7 @@ class AtomMenuModel; base::scoped_nsobject menu_; BOOL isMenuOpen_; BOOL useDefaultAccelerator_; + base::Callback 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&)callback; + // Populate current NSMenu with |model|. - (void)populateWithModel:(atom::AtomMenuModel*)model; diff --git a/atom/browser/ui/cocoa/atom_menu_controller.mm b/atom/browser/ui/cocoa/atom_menu_controller.mm index 6bdaa4c7805..2286b6b0ce9 100644 --- a/atom/browser/ui/cocoa/atom_menu_controller.mm +++ b/atom/browser/ui/cocoa/atom_menu_controller.mm @@ -71,6 +71,10 @@ Role kRolesMap[] = { [super dealloc]; } +- (void)setCloseCallback:(const base::Callback&)callback { + closeCallback = callback; +} + - (void)populateWithModel:(atom::AtomMenuModel*)model { if (!menu_) return; @@ -265,8 +269,10 @@ Role kRolesMap[] = { - (void)menuDidClose:(NSMenu*)menu { if (isMenuOpen_) { - model_->MenuWillClose(); isMenuOpen_ = NO; + model_->MenuWillClose(); + if (!closeCallback.is_null()) + closeCallback.Run(); } } diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index e4fda6d5b07..30f3df70c57 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -174,6 +174,15 @@ Menu.prototype.popup = function (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) { return this.insert(this.getItemCount(), item) } From 0a5ccdccb43653b2911fb5f4f9e2619a3d1b8e8b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 10:58:12 -0800 Subject: [PATCH 09/16] Add spec for async Menu.popup --- spec/api-menu-spec.js | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 0c3cb50e4d4..176f1eafae5 100644 --- a/spec/api-menu-spec.js +++ b/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([ From 91d1af053f996e288d8910c48230f5f22129ce72 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 11:04:42 -0800 Subject: [PATCH 10/16] Implement Menu.closePopup on Windows/Linux --- atom/browser/api/atom_api_menu_views.cc | 15 ++++++--------- atom/browser/api/atom_api_menu_views.h | 6 ++++-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 3be2a17a279..d3d4b2cbbc6 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -49,11 +49,12 @@ void MenuViews::PopupAt( atom::UnresponsiveSuppressor suppressor; // Show the menu. - menu_runner_.reset(new MenuRunner( - model(), - flags, - base::Bind(&MenuViews::OnMenuClosed, weak_factory_.GetWeakPtr()))); - ignore_result(menu_runner_->RunMenuAt( + int32_t window_id = window->ID(); + auto close_callback = base::Bind( + &MenuViews::ClosePopupAt, weak_factory_.GetWeakPtr(), window_id); + menu_runners_[window_id] = std::unique_ptr(new MenuRunner( + model(), flags, close_callback)); + ignore_result(menu_runners_[window_id]->RunMenuAt( static_cast(window->window())->widget(), NULL, gfx::Rect(location, gfx::Size()), @@ -61,10 +62,6 @@ void MenuViews::PopupAt( ui::MENU_SOURCE_MOUSE)); } -void MenuViews::OnMenuClosed() { - menu_runner_.reset(); -} - // static mate::WrappableBase* Menu::New(mate::Arguments* args) { return new MenuViews(args->isolate(), args->GetThis()); diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index 5e5dc8e93dc..2e4c84a03b7 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -5,6 +5,8 @@ #ifndef ATOM_BROWSER_API_ATOM_API_MENU_VIEWS_H_ #define ATOM_BROWSER_API_ATOM_API_MENU_VIEWS_H_ +#include + #include "atom/browser/api/atom_api_menu.h" #include "base/memory/weak_ptr.h" #include "ui/display/screen.h" @@ -21,10 +23,10 @@ class MenuViews : public Menu { protected: void PopupAt( Window* window, int x, int y, int positioning_item, bool async) override; - void OnMenuClosed(); + void ClosePopupAt(int32_t window_id); private: - std::unique_ptr menu_runner_; + std::map> menu_runners_; base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(MenuViews); From ce5ac1b056229565ea52e7184c8e19c2d9d0afc8 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 11:15:05 -0800 Subject: [PATCH 11/16] Implement ClosePopupAt on Windows/Linux --- atom/browser/api/atom_api_menu_views.cc | 4 ++++ atom/browser/api/atom_api_menu_views.h | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index d3d4b2cbbc6..59fdac39491 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -62,6 +62,10 @@ void MenuViews::PopupAt( 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()); diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index 2e4c84a03b7..f59e9d5b892 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -23,7 +23,7 @@ class MenuViews : public Menu { protected: void PopupAt( Window* window, int x, int y, int positioning_item, bool async) override; - void ClosePopupAt(int32_t window_id); + void ClosePopupAt(int32_t window_id) override; private: std::map> menu_runners_; From 6c6506e5aa735a6dd81d5418a568605ddae2b069 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 16 Feb 2017 11:18:13 -0800 Subject: [PATCH 12/16] Document menu.closePopup --- docs/api/menu.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/api/menu.md b/docs/api/menu.md index 6969c4457eb..466d226a168 100644 --- a/docs/api/menu.md +++ b/docs/api/menu.md @@ -54,8 +54,7 @@ The `menu` object has the following instance methods: #### `menu.popup([browserWindow, options])` -* `browserWindow` BrowserWindow (optional) - Default is - `BrowserWindow.getFocusedWindow()`. +* `browserWindow` BrowserWindow (optional) - Default is the focused window. * `options` Object (optional) * `x` Number (optional) - Default is the current mouse cursor position. * `y` Number (**required** if `x` is used) - Default is the current mouse @@ -69,6 +68,12 @@ The `menu` object has the following instance methods: 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 From 2006e22aa45d17b98251acece9c31b4c06abca78 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Fri, 17 Feb 2017 12:09:29 -0800 Subject: [PATCH 13/16] :art: --- atom/browser/api/atom_api_menu.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 59312e19a76..df50640b85a 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -53,8 +53,8 @@ class Menu : public mate::TrackableObject, void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; - virtual void PopupAt(Window* window, int x, int y, int positioning_item, - bool async) = 0; + virtual void PopupAt( + Window* window, int x, int y, int positioning_item, bool async) = 0; virtual void ClosePopupAt(int32_t window_id) = 0; std::unique_ptr model_; From a8d1a7aed485485b355bf9bcb8b119f838953694 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Fri, 17 Feb 2017 12:16:29 -0800 Subject: [PATCH 14/16] Make variables private instead of protected --- atom/browser/api/atom_api_menu_mac.h | 8 +++++--- atom/browser/api/atom_api_menu_views.h | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index 7c41cd73e4b..bc70e5b5ae5 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -29,14 +29,16 @@ class MenuMac : public Menu { bool async); void ClosePopupAt(int32_t window_id) override; - scoped_nsobject menu_controller_; - std::map> popup_controllers_; - private: friend class Menu; static void SendActionToFirstResponder(const std::string& action); + scoped_nsobject menu_controller_; + + // window ID -> open context menu + std::map> popup_controllers_; + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(MenuMac); diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index f59e9d5b892..a974f75bc76 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -26,7 +26,9 @@ class MenuViews : public Menu { void ClosePopupAt(int32_t window_id) override; private: + // window ID -> open context menu std::map> menu_runners_; + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(MenuViews); From d0b07d5c368e74657056f6edc95a63e5337a2309 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Feb 2017 10:49:25 -0800 Subject: [PATCH 15/16] Check that x is non-null --- lib/browser/api/menu.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 30f3df70c57..14e87d3d74f 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -156,7 +156,7 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { } // menu.popup(window, {}) - if (typeof x === 'object') { + if (x != null && typeof x === 'object') { const options = x x = options.x y = options.y From 211bedf9104edf5cd19a0d879bba78de590ed6d2 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 22 Feb 2017 11:47:58 -0800 Subject: [PATCH 16/16] Invoke close callback after itemSelected runs --- atom/browser/ui/cocoa/atom_menu_controller.mm | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/atom/browser/ui/cocoa/atom_menu_controller.mm b/atom/browser/ui/cocoa/atom_menu_controller.mm index 2286b6b0ce9..b3e293153f9 100644 --- a/atom/browser/ui/cocoa/atom_menu_controller.mm +++ b/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 { @@ -271,8 +274,11 @@ Role kRolesMap[] = { if (isMenuOpen_) { 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()) - closeCallback.Run(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, closeCallback); } }