From c5868066097abf083b18127ea1414ead6caf353c Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Mon, 27 Nov 2017 00:24:25 +0100 Subject: [PATCH 1/6] fix flash menu being unresponsive to commands --- atom/browser/api/atom_api_menu.cc | 5 +++++ atom/browser/api/atom_api_menu.h | 2 ++ atom/browser/api/atom_api_web_contents.cc | 6 ++++-- lib/browser/api/menu.js | 3 +++ lib/browser/api/web-contents.js | 6 +++++- 5 files changed, 19 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index da208b365911..202947092a8b 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -40,6 +40,7 @@ void Menu::AfterInit(v8::Isolate* isolate) { delegate.Get("getAcceleratorForCommandId", &get_accelerator_); delegate.Get("executeCommand", &execute_command_); delegate.Get("menuWillShow", &menu_will_show_); + delegate.Get("menuClosed", &menu_closed_); } bool Menu::IsCommandIdChecked(int command_id) const { @@ -75,6 +76,10 @@ void Menu::MenuWillShow(ui::SimpleMenuModel* source) { menu_will_show_.Run(); } +void Menu::MenuClosed(ui::SimpleMenuModel* source) { + menu_closed_.Run(); +} + void Menu::InsertItemAt( int index, int command_id, const base::string16& label) { model_->InsertItemAt(index, command_id, label); diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index f2316fa1893a..c1f0c600101b 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -52,6 +52,7 @@ class Menu : public mate::TrackableObject, ui::Accelerator* accelerator) const override; void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; + void MenuClosed(ui::SimpleMenuModel* source) override; virtual void PopupAt(Window* window, int x, int y, int positioning_item) = 0; virtual void ClosePopupAt(int32_t window_id) = 0; @@ -93,6 +94,7 @@ class Menu : public mate::TrackableObject, base::Callback(int, bool)> get_accelerator_; base::Callback, int)> execute_command_; base::Callback menu_will_show_; + base::Callback menu_closed_; DISALLOW_COPY_AND_ASSIGN(Menu); }; diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index aa9f539bd93a..2a9650160a57 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -653,8 +653,10 @@ void WebContents::RendererResponsive(content::WebContents* source) { bool WebContents::HandleContextMenu(const content::ContextMenuParams& params) { if (params.custom_context.is_pepper_menu) { - Emit("pepper-context-menu", std::make_pair(params, web_contents())); - web_contents()->NotifyContextMenuClosed(params.custom_context); + Emit("pepper-context-menu", + std::make_pair(params, web_contents()), + base::Bind(&content::WebContents::NotifyContextMenuClosed, + base::Unretained(web_contents()), params.custom_context)); } else { Emit("context-menu", std::make_pair(params, web_contents())); } diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 78f1a4466a84..bd3427bcd6a4 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -39,6 +39,9 @@ Menu.prototype._init = function () { const found = this.groupsMap[id].find(item => item.checked) || null if (!found) v8Util.setHiddenValue(this.groupsMap[id][0], 'checked', true) } + }, + menuClosed: () => { + this.emit('closed') } } } diff --git a/lib/browser/api/web-contents.js b/lib/browser/api/web-contents.js index f770dbb31b3c..8d73bbe26211 100644 --- a/lib/browser/api/web-contents.js +++ b/lib/browser/api/web-contents.js @@ -277,10 +277,14 @@ WebContents.prototype._init = function () { }) // Handle context menu action request from pepper plugin. - this.on('pepper-context-menu', function (event, params) { + this.on('pepper-context-menu', function (event, params, callback) { // Access Menu via electron.Menu to prevent circular require const menu = electron.Menu.buildFromTemplate(params.menu) menu.popup(event.sender.getOwnerBrowserWindow(), params.x, params.y) + + menu.on('closed', () => { + callback() + }) }) // The devtools requests the webContents to reload. From bcef6eb3dfddeac81c8761fdc0e991731fffc70c Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Mon, 27 Nov 2017 21:28:30 +0100 Subject: [PATCH 2/6] document menu closed event and fix styling issue --- atom/browser/api/atom_api_web_contents.cc | 7 +++---- docs/api/menu.md | 9 +++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 2a9650160a57..320741106cdb 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -653,10 +653,9 @@ void WebContents::RendererResponsive(content::WebContents* source) { bool WebContents::HandleContextMenu(const content::ContextMenuParams& params) { if (params.custom_context.is_pepper_menu) { - Emit("pepper-context-menu", - std::make_pair(params, web_contents()), - base::Bind(&content::WebContents::NotifyContextMenuClosed, - base::Unretained(web_contents()), params.custom_context)); + Emit("pepper-context-menu", std::make_pair(params, web_contents()), + base::Bind(&content::WebContents::NotifyContextMenuClosed, + base::Unretained(web_contents()), params.custom_context)); } else { Emit("context-menu", std::make_pair(params, web_contents())); } diff --git a/docs/api/menu.md b/docs/api/menu.md index cc4922e6e4d2..e01666ef1ec8 100644 --- a/docs/api/menu.md +++ b/docs/api/menu.md @@ -109,6 +109,15 @@ A `MenuItem[]` array containing the menu's items. Each `Menu` consists of multiple [`MenuItem`](menu-item.md)s and each `MenuItem` can have a submenu. +### Instance Events + +Objects created with `new Menu` or returned by `Menu.buildFromTemplate` emit +the following events: + +#### Event: 'closed' + +Emitted when the menu is closed. + ## Examples The `Menu` class is only available in the main process, but you can also use it From 14b65467d860741f8964726844f030c37fd2b45b Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 28 Nov 2017 00:13:07 +0100 Subject: [PATCH 3/6] add test for menu closed event --- spec/api-menu-spec.js | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 0e1050066855..dc73bf4bd9fb 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -325,6 +325,32 @@ describe('Menu module', () => { }) }) + describe('Menu.closePopup()', () => { + let w = null + let menu + + beforeEach(() => { + w = new BrowserWindow({show: false, width: 200, height: 200}) + menu = Menu.buildFromTemplate([ + { + label: '1' + } + ]) + menu.popup(w, {x: 100, y: 100, async: true}) + menu.closePopup(w) + }) + + afterEach(() => { + return closeWindow(w).then(() => { w = null }) + }) + + it('emits closed event', () => { + menu.on('closed', () => { + done() + }) + }) + }) + describe('Menu.setApplicationMenu', () => { it('sets a menu', () => { const menu = Menu.buildFromTemplate([ From ef7357dedcdd8cd04ac8668a8be0c124f4e714dd Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 28 Nov 2017 02:29:37 +0100 Subject: [PATCH 4/6] update tests for menu closed event and call cancel on closePopup --- atom/browser/api/atom_api_menu_views.cc | 2 ++ spec/api-menu-spec.js | 13 +++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 7a53d5d55307..62a785519fc9 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -60,6 +60,8 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) { } void MenuViews::ClosePopupAt(int32_t window_id) { + if (menu_runners_[window_id]) + menu_runners_[window_id]->Cancel(); menu_runners_.erase(window_id); } diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index dc73bf4bd9fb..f607e2b8773a 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -329,25 +329,30 @@ describe('Menu module', () => { let w = null let menu - beforeEach(() => { + beforeEach((done) => { w = new BrowserWindow({show: false, width: 200, height: 200}) menu = Menu.buildFromTemplate([ { label: '1' } ]) - menu.popup(w, {x: 100, y: 100, async: true}) - menu.closePopup(w) + + w.loadURL('data:text/html,teszt') + w.webContents.on('dom-ready', () => { + done() + }) }) afterEach(() => { return closeWindow(w).then(() => { w = null }) }) - it('emits closed event', () => { + it('emits closed event', (done) => { + menu.popup(w, {x: 100, y: 100, async: true}) menu.on('closed', () => { done() }) + menu.closePopup(w) }) }) From 5bf16c2495f95cc841e1ec639634cf8b6fb6c857 Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Mon, 4 Dec 2017 08:14:30 +0100 Subject: [PATCH 5/6] remove async:true from menu tests --- spec/api-menu-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index f607e2b8773a..efa79dd7b5d6 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -348,7 +348,7 @@ describe('Menu module', () => { }) it('emits closed event', (done) => { - menu.popup(w, {x: 100, y: 100, async: true}) + menu.popup(w, {x: 100, y: 100}) menu.on('closed', () => { done() }) From e4770c76045f2776b112d0b43a1a740fe4137981 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 20 Dec 2017 18:48:09 +0900 Subject: [PATCH 6/6] Coding style fixes --- atom/browser/api/atom_api_menu_views.cc | 5 +++-- atom/browser/api/atom_api_web_contents.cc | 7 ++++--- lib/browser/api/web-contents.js | 7 ++----- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 62a785519fc9..0a3b16183030 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -60,9 +60,10 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) { } void MenuViews::ClosePopupAt(int32_t window_id) { - if (menu_runners_[window_id]) + if (menu_runners_[window_id]) { menu_runners_[window_id]->Cancel(); - menu_runners_.erase(window_id); + menu_runners_.erase(window_id); + } } // static diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 320741106cdb..dbf53c860039 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -653,9 +653,10 @@ void WebContents::RendererResponsive(content::WebContents* source) { bool WebContents::HandleContextMenu(const content::ContextMenuParams& params) { if (params.custom_context.is_pepper_menu) { - Emit("pepper-context-menu", std::make_pair(params, web_contents()), - base::Bind(&content::WebContents::NotifyContextMenuClosed, - base::Unretained(web_contents()), params.custom_context)); + Emit("pepper-context-menu", + std::make_pair(params, web_contents()), + base::Bind(&content::WebContents::NotifyContextMenuClosed, + base::Unretained(web_contents()), params.custom_context)); } else { Emit("context-menu", std::make_pair(params, web_contents())); } diff --git a/lib/browser/api/web-contents.js b/lib/browser/api/web-contents.js index 8d73bbe26211..2d84e9f564af 100644 --- a/lib/browser/api/web-contents.js +++ b/lib/browser/api/web-contents.js @@ -278,13 +278,10 @@ WebContents.prototype._init = function () { // Handle context menu action request from pepper plugin. this.on('pepper-context-menu', function (event, params, callback) { - // Access Menu via electron.Menu to prevent circular require + // Access Menu via electron.Menu to prevent circular require. const menu = electron.Menu.buildFromTemplate(params.menu) + menu.once('closed', callback) menu.popup(event.sender.getOwnerBrowserWindow(), params.x, params.y) - - menu.on('closed', () => { - callback() - }) }) // The devtools requests the webContents to reload.