Merge pull request #6170 from electron/click-event

Add "event" parameter for "click" handler of MenuItem

atom/browser/api/atom_api_menu.cc

@@ -61,8 +61,10 @@ bool Menu::GetAcceleratorForCommandId(int command_id,
   return mate::ConvertFromV8(isolate(), val, accelerator);
 }
 
-void Menu::ExecuteCommand(int command_id, int event_flags) {
+void Menu::ExecuteCommand(int command_id, int flags) {
-  execute_command_.Run(command_id);
+  execute_command_.Run(
+      mate::internal::CreateEventFromFlags(isolate(), flags),
+      command_id);
 }
 
 void Menu::MenuWillShow(ui::SimpleMenuModel* source) {

atom/browser/api/atom_api_menu.h

@@ -90,7 +90,7 @@ class Menu : public mate::TrackableObject<Menu>,
   base::Callback<bool(int)> is_enabled_;
   base::Callback<bool(int)> is_visible_;
   base::Callback<v8::Local<v8::Value>(int)> get_accelerator_;
-  base::Callback<void(int)> execute_command_;
+  base::Callback<void(v8::Local<v8::Value>, int)> execute_command_;
   base::Callback<void()> menu_will_show_;
 
   DISALLOW_COPY_AND_ASSIGN(Menu);

atom/browser/api/atom_api_tray.cc

@@ -16,7 +16,6 @@
 #include "atom/common/node_includes.h"
 #include "native_mate/constructor.h"
 #include "native_mate/dictionary.h"
-#include "ui/events/event_constants.h"
 #include "ui/gfx/image/image.h"
 
 namespace atom {
@@ -44,24 +43,15 @@ mate::WrappableBase* Tray::New(v8::Isolate* isolate,
 }
 
 void Tray::OnClicked(const gfx::Rect& bounds, int modifiers) {
-  v8::Locker locker(isolate());
+  EmitWithFlags("click", modifiers, bounds);
-  v8::HandleScope handle_scope(isolate());
-  EmitCustomEvent("click",
-                  ModifiersToObject(isolate(), modifiers), bounds);
 }
 
 void Tray::OnDoubleClicked(const gfx::Rect& bounds, int modifiers) {
-  v8::Locker locker(isolate());
+  EmitWithFlags("double-click", modifiers, bounds);
-  v8::HandleScope handle_scope(isolate());
-  EmitCustomEvent("double-click",
-                  ModifiersToObject(isolate(), modifiers), bounds);
 }
 
 void Tray::OnRightClicked(const gfx::Rect& bounds, int modifiers) {
-  v8::Locker locker(isolate());
+  EmitWithFlags("right-click", modifiers, bounds);
-  v8::HandleScope handle_scope(isolate());
-  EmitCustomEvent("right-click",
-                  ModifiersToObject(isolate(), modifiers), bounds);
 }
 
 void Tray::OnBalloonShow() {
@@ -163,16 +153,6 @@ gfx::Rect Tray::GetBounds() {
   return tray_icon_->GetBounds();
 }
 
-v8::Local<v8::Object> Tray::ModifiersToObject(v8::Isolate* isolate,
-                                              int modifiers) {
-  mate::Dictionary obj(isolate, v8::Object::New(isolate));
-  obj.Set("shiftKey", static_cast<bool>(modifiers & ui::EF_SHIFT_DOWN));
-  obj.Set("ctrlKey", static_cast<bool>(modifiers & ui::EF_CONTROL_DOWN));
-  obj.Set("altKey", static_cast<bool>(modifiers & ui::EF_ALT_DOWN));
-  obj.Set("metaKey", static_cast<bool>(modifiers & ui::EF_COMMAND_DOWN));
-  return obj.GetHandle();
-}
-
 // static
 void Tray::BuildPrototype(v8::Isolate* isolate,
                           v8::Local<v8::ObjectTemplate> prototype) {

atom/browser/api/atom_api_tray.h

@@ -68,8 +68,6 @@ class Tray : public mate::TrackableObject<Tray>,
   gfx::Rect GetBounds();
 
  private:
-  v8::Local<v8::Object> ModifiersToObject(v8::Isolate* isolate, int modifiers);
-
   v8::Global<v8::Object> menu_;
   std::unique_ptr<TrayIcon> tray_icon_;
 

atom/browser/api/event_emitter.cc

@@ -8,6 +8,7 @@
 #include "native_mate/arguments.h"
 #include "native_mate/dictionary.h"
 #include "native_mate/object_template_builder.h"
+#include "ui/events/event_constants.h"
 
 namespace mate {
 
@@ -65,6 +66,15 @@ v8::Local<v8::Object> CreateCustomEvent(
   return event;
 }
 
+v8::Local<v8::Object> CreateEventFromFlags(v8::Isolate* isolate, int flags) {
+  mate::Dictionary obj = mate::Dictionary::CreateEmpty(isolate);
+  obj.Set("shiftKey", static_cast<bool>(flags & ui::EF_SHIFT_DOWN));
+  obj.Set("ctrlKey", static_cast<bool>(flags & ui::EF_CONTROL_DOWN));
+  obj.Set("altKey", static_cast<bool>(flags & ui::EF_ALT_DOWN));
+  obj.Set("metaKey", static_cast<bool>(flags & ui::EF_COMMAND_DOWN));
+  return obj.GetHandle();
+}
+
 }  // namespace internal
 
 }  // namespace mate

atom/browser/api/event_emitter.h

@@ -30,6 +30,7 @@ v8::Local<v8::Object> CreateCustomEvent(
     v8::Isolate* isolate,
     v8::Local<v8::Object> object,
     v8::Local<v8::Object> event);
+v8::Local<v8::Object> CreateEventFromFlags(v8::Isolate* isolate, int flags);
 
 }  // namespace internal
 
@@ -54,6 +55,16 @@ class EventEmitter : public Wrappable<T> {
         internal::CreateCustomEvent(isolate(), GetWrapper(), event), args...);
   }
 
+  // this.emit(name, new Event(flags), args...);
+  template<typename... Args>
+  bool EmitWithFlags(const base::StringPiece& name,
+                     int flags,
+                     const Args&... args) {
+    return EmitCustomEvent(
+        name,
+        internal::CreateEventFromFlags(isolate(), flags), args...);
+  }
+
   // this.emit(name, new Event(), args...);
   template<typename... Args>
   bool Emit(const base::StringPiece& name, const Args&... args) {

docs/api/menu-item.md

@@ -2,7 +2,7 @@
 
 > Add items to native application menus and context menus.
 
-See [`menu`](menu.md) for examples.
+See [`Menu`](menu.md) for examples.
 
 ## Class: MenuItem
 
@@ -11,12 +11,12 @@ Create a new `MenuItem` with the following method:
 ### new MenuItem(options)
 
 * `options` Object
-  * `click` Function - Will be called with `click(menuItem, browserWindow)` when
+  * `click` Function - Will be called with
-     the menu item is clicked
+    `click(menuItem, browserWindow, event)` when the menu item is clicked.
-  * `role` String - Define the action of the menu item; when specified the
+  * `role` String - Define the action of the menu item, when specified the
-     `click` property will be ignored
+    `click` property will be ignored.
   * `type` String - Can be `normal`, `separator`, `submenu`, `checkbox` or
-     `radio`
+    `radio`.
   * `label` String
   * `sublabel` String
   * `accelerator` [Accelerator](accelerator.md)
@@ -25,15 +25,15 @@ Create a new `MenuItem` with the following method:
     unclickable.
   * `visible` Boolean - If false, the menu item will be entirely hidden.
   * `checked` Boolean - Should only be specified for `checkbox` or `radio` type
-      menu items.
+    menu items.
   * `submenu` Menu - Should be specified for `submenu` type menu items. If
-     `submenu` is specified, the `type: 'submenu'` can be omitted. If the value
+    `submenu` is specified, the `type: 'submenu'` can be omitted. If the value
-     is not a `Menu` then it will be automatically converted to one using
+    is not a `Menu` then it will be automatically converted to one using
-     `Menu.buildFromTemplate`.
+    `Menu.buildFromTemplate`.
   * `id` String - Unique within a single menu. If defined then it can be used
-     as a reference to this item by the position attribute.
+    as a reference to this item by the position attribute.
   * `position` String - This field allows fine-grained definition of the
-     specific location within a given menu.
+    specific location within a given menu.
 
 It is best to specify `role` for any menu item that matches a standard role,
 rather than trying to manually implement the behavior in a `click` function.
@@ -69,19 +69,29 @@ On macOS `role` can also have following additional values:
 When specifying `role` on macOS, `label` and `accelerator` are the only options
 that will affect the MenuItem. All other options will be ignored.
 
-## Instance Properties
+### Instance Properties
 
-The following properties (and no others) can be updated on an existing `MenuItem`:
+The following properties are available on instances of `MenuItem`:
 
-  * `enabled` Boolean
+#### `menuItem.enabled`
-  * `visible` Boolean
-  * `checked` Boolean
 
-Their meanings are as described above.
+A Boolean indicating whether the item is enabled, this property can be
+dynamically changed.
 
-A `checkbox` menu item will toggle its `checked` property on and off when
+#### `menuItem.visible`
-selected. You can add a `click` function to do additional work.
+
+A Boolean indicating whether the item is visible, this property can be
+dynamically changed.
+
+#### `menuItem.checked`
+
+A Boolean indicating whether the item is checked, this property can be
+dynamically changed.
+
+A `checkbox` menu item will toggle the `checked` property on and off when
+selected.
 
 A `radio` menu item will turn on its `checked` property when clicked, and
-will turn off that property for all adjacent items in the same menu. Again,
+will turn off that property for all adjacent items in the same menu.
-you can add a `click` function for additional behavior.
+
+You can add a `click` function for additional behavior.

docs/api/session.md

@@ -566,3 +566,4 @@ app.on('ready', function () {
       console.error('Failed to register protocol')
   })
 })
+```

lib/browser/api/menu-item.js

@@ -72,7 +72,7 @@ const MenuItem = (function () {
       throw new Error('Unknown menu type ' + this.type)
     }
     this.commandId = ++nextCommandId
-    this.click = (focusedWindow) => {
+    this.click = (event, focusedWindow) => {
       // Manually flip the checked flags when clicked.
       if (this.type === 'checkbox' || this.type === 'radio') {
         this.checked = !this.checked
@@ -91,7 +91,7 @@ const MenuItem = (function () {
           return webContents != null ? webContents[methodName]() : void 0
         }
       } else if (typeof click === 'function') {
-        return click(this, focusedWindow)
+        return click(this, focusedWindow, event)
       } else if (typeof this.selector === 'string' && process.platform === 'darwin') {
         return Menu.sendActionToFirstResponder(this.selector)
       }

lib/browser/api/menu.js

@@ -114,9 +114,9 @@ Menu.prototype._init = function () {
       var command = this.commandsMap[commandId]
       return command != null ? command.icon : undefined
     },
-    executeCommand: (commandId) => {
+    executeCommand: (event, commandId) => {
       var command = this.commandsMap[commandId]
-      return command != null ? command.click(BrowserWindow.getFocusedWindow()) : undefined
+      return command != null ? command.click(event, BrowserWindow.getFocusedWindow()) : undefined
     },
     menuWillShow: () => {
       // Make sure radio groups have at least one menu item seleted.

spec/api-menu-spec.js

@@ -231,7 +231,7 @@ describe('menu module', function () {
           }
         }
       ])
-      menu.delegate.executeCommand(menu.items[0].commandId)
+      menu.delegate.executeCommand({}, menu.items[0].commandId)
     })
   })
 
@@ -244,7 +244,7 @@ describe('menu module', function () {
         }
       ])
       assert.equal(menu.items[0].checked, false)
-      menu.delegate.executeCommand(menu.items[0].commandId)
+      menu.delegate.executeCommand({}, menu.items[0].commandId)
       assert.equal(menu.items[0].checked, true)
     })
 
@@ -255,9 +255,9 @@ describe('menu module', function () {
           type: 'radio'
         }
       ])
-      menu.delegate.executeCommand(menu.items[0].commandId)
+      menu.delegate.executeCommand({}, menu.items[0].commandId)
       assert.equal(menu.items[0].checked, true)
-      menu.delegate.executeCommand(menu.items[0].commandId)
+      menu.delegate.executeCommand({}, menu.items[0].commandId)
       assert.equal(menu.items[0].checked, true)
     })