fix: correct triggeredByAccelerator Event property behavior (#18865)

Fixes #18808

Previously, the triggeredByAccelerator flag would be entirely coupled with whether or not the modifier keys were being used or not.

This PR swaps out the ui::EventFlagsFromModifiers([event modifierFlags])) call in the macOS code to ui::EventFlagsFromNSEventWithModifiers(event, [event modifierFlags])). The latter outputs flags that take into account mouse click events on top of modifier flags (see Chromium documentation).

The business logic to detect triggeredByAccelerator is then changed to exclude any mouse click flags.

shell/browser/api/event_emitter.cc

@@ -77,12 +77,17 @@ v8::Local<v8::Object> CreateCustomEvent(v8::Isolate* isolate,
 }
 
 v8::Local<v8::Object> CreateEventFromFlags(v8::Isolate* isolate, int flags) {
+  const int mouse_button_flags =
+      (ui::EF_RIGHT_MOUSE_BUTTON | ui::EF_LEFT_MOUSE_BUTTON |
+       ui::EF_MIDDLE_MOUSE_BUTTON | ui::EF_BACK_MOUSE_BUTTON |
+       ui::EF_FORWARD_MOUSE_BUTTON);
+  const int is_mouse_click = static_cast<bool>(flags & mouse_button_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));
-  obj.Set("triggeredByAccelerator", static_cast<bool>(flags));
+  obj.Set("triggeredByAccelerator", !is_mouse_click);
   return obj.GetHandle();
 }
 

shell/browser/ui/cocoa/atom_menu_controller.mm

@@ -345,7 +345,7 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
   if (model) {
     NSEvent* event = [NSApp currentEvent];
     model->ActivatedAt(modelIndex,
-                       ui::EventFlagsFromModifiers([event modifierFlags]));
+                       ui::EventFlagsFromNSEventWithModifiers(event, [event modifierFlags]));
   }
 }
 

spec/api-menu-spec.js

@@ -2,13 +2,15 @@ const chai = require('chai')
 const dirtyChai = require('dirty-chai')
 
 const { ipcRenderer, remote } = require('electron')
-const { BrowserWindow, Menu, MenuItem } = remote
+const { BrowserWindow, globalShortcut, Menu, MenuItem } = remote
 const { sortMenuItems } = require('../lib/browser/api/menu-utils')
 const { closeWindow } = require('./window-helpers')
 
 const { expect } = chai
 chai.use(dirtyChai)
 
+const isCi = remote.getGlobal('isCi')
+
 describe('Menu module', () => {
   describe('Menu.buildFromTemplate', () => {
     it('should be able to attach extra fields', () => {
@@ -835,36 +837,63 @@ describe('Menu module', () => {
     })
   })
 
-  describe('menu accelerators', () => {
-    let testFn = it
-    try {
-      // We have other tests that check if native modules work, if we fail to require
-      // robotjs let's skip this test to avoid false negatives
-      require('robotjs')
-    } catch (err) {
-      testFn = it.skip
-    }
+  describe('menu accelerators', async () => {
     const sendRobotjsKey = (key, modifiers = [], delay = 500) => {
       return new Promise((resolve, reject) => {
-        require('robotjs').keyTap(key, modifiers)
-        setTimeout(() => {
-          resolve()
-        }, delay)
+        try {
+          require('robotjs').keyTap(key, modifiers)
+          setTimeout(() => {
+            resolve()
+          }, delay)
+        } catch (e) {
+          reject(e)
+        }
       })
     }
 
-    testFn('menu accelerators perform the specified action', async () => {
+    before(async function () {
+      // --ci flag breaks accelerator and robotjs interaction
+      if (isCi) {
+        this.skip()
+      }
+
+      // before accelerator tests, use globalShortcut to test if
+      // RobotJS is working at all
+      let isKeyPressed = false
+      globalShortcut.register('q', () => {
+        isKeyPressed = true
+      })
+      try {
+        await sendRobotjsKey('q')
+      } catch (e) {
+        this.skip()
+      }
+
+      if (!isKeyPressed) {
+        this.skip()
+      }
+
+      globalShortcut.unregister('q')
+    })
+
+    it('should perform the specified action', async () => {
+      let hasBeenClicked = false
       const menu = Menu.buildFromTemplate([
         {
           label: 'Test',
           submenu: [
             {
               label: 'Test Item',
-              accelerator: 'Ctrl+T',
-              click: () => {
-                // Test will succeed, only when the menu accelerator action
-                // is triggered
-                Promise.resolve()
+              accelerator: 'T',
+              click: (a, b, event) => {
+                hasBeenClicked = true
+                expect(event).to.deep.equal({
+                  shiftKey: false,
+                  ctrlKey: false,
+                  altKey: false,
+                  metaKey: false,
+                  triggeredByAccelerator: true
+                })
               },
               id: 'test'
             }
@@ -873,7 +902,31 @@ describe('Menu module', () => {
       ])
       Menu.setApplicationMenu(menu)
       expect(Menu.getApplicationMenu()).to.not.be.null()
-      await sendRobotjsKey('t', 'control')
+      await sendRobotjsKey('t')
+      expect(hasBeenClicked).to.equal(true)
+    })
+
+    it('should not activate upon clicking another key combination', async () => {
+      let hasBeenClicked = false
+      const menu = Menu.buildFromTemplate([
+        {
+          label: 'Test',
+          submenu: [
+            {
+              label: 'Test Item',
+              accelerator: 'T',
+              click: (a, b, event) => {
+                hasBeenClicked = true
+              },
+              id: 'test'
+            }
+          ]
+        }
+      ])
+      Menu.setApplicationMenu(menu)
+      expect(Menu.getApplicationMenu()).to.not.be.null()
+      await sendRobotjsKey('t', 'shift')
+      expect(hasBeenClicked).to.equal(false)
     })
   })
 })