From 5994bf6745d36c3ffb278e2067803fe0ea009322 Mon Sep 17 00:00:00 2001
From: Nitish Sakhawalkar <nitsakh@icloud.com>
Date: Fri, 9 Nov 2018 13:54:16 -0800
Subject: [PATCH] fix: Menu accelerators not working (#15094)

This change fixes the regression in the menu accelerators working in linux, on some environments.
---
 atom/browser/native_window_views.cc |  6 ++++-
 atom/browser/ui/views/root_view.cc  | 20 +++++++++-----
 atom/browser/ui/views/root_view.h   |  6 ++---
 spec/api-menu-spec.js               | 42 +++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc
index 8dfa0f26e8d1..c45dbb26cc97 100644
--- a/atom/browser/native_window_views.cc
+++ b/atom/browser/native_window_views.cc
@@ -900,14 +900,18 @@ void NativeWindowViews::SetFocusable(bool focusable) {
 
 void NativeWindowViews::SetMenu(AtomMenuModel* menu_model) {
 #if defined(USE_X11)
-  if (menu_model == nullptr)
+  if (menu_model == nullptr) {
     global_menu_bar_.reset();
+    root_view_->UnregisterAcceleratorsWithFocusManager();
+    return;
+  }
 
   if (!global_menu_bar_ && ShouldUseGlobalMenuBar())
     global_menu_bar_.reset(new GlobalMenuBarX11(this));
 
   // Use global application menu bar when possible.
   if (global_menu_bar_ && global_menu_bar_->IsServerStarted()) {
+    root_view_->RegisterAcceleratorsWithFocusManager(menu_model);
     global_menu_bar_->SetMenu(menu_model);
     return;
   }
diff --git a/atom/browser/ui/views/root_view.cc b/atom/browser/ui/views/root_view.cc
index db03674d2e51..d9fe02bbbd10 100644
--- a/atom/browser/ui/views/root_view.cc
+++ b/atom/browser/ui/views/root_view.cc
@@ -46,15 +46,14 @@ RootView::~RootView() {}
 void RootView::SetMenu(AtomMenuModel* menu_model) {
   if (menu_model == nullptr) {
     // Remove accelerators
-    accelerator_table_.clear();
-    GetFocusManager()->UnregisterAccelerators(this);
+    UnregisterAcceleratorsWithFocusManager();
     // and menu bar.
     SetMenuBarVisibility(false);
     menu_bar_.reset();
     return;
   }
 
-  RegisterAccelerators(menu_model);
+  RegisterAcceleratorsWithFocusManager(menu_model);
 
   // Do not show menu bar in frameless window.
   if (!window_->has_frame())
@@ -197,12 +196,13 @@ bool RootView::AcceleratorPressed(const ui::Accelerator& accelerator) {
                                                           accelerator);
 }
 
-void RootView::RegisterAccelerators(AtomMenuModel* menu_model) {
+void RootView::RegisterAcceleratorsWithFocusManager(AtomMenuModel* menu_model) {
+  if (!menu_model)
+    return;
   // Clear previous accelerators.
-  views::FocusManager* focus_manager = GetFocusManager();
-  accelerator_table_.clear();
-  focus_manager->UnregisterAccelerators(this);
+  UnregisterAcceleratorsWithFocusManager();
 
+  views::FocusManager* focus_manager = GetFocusManager();
   // Register accelerators with focus manager.
   accelerator_util::GenerateAcceleratorTable(&accelerator_table_, menu_model);
   for (const auto& iter : accelerator_table_) {
@@ -211,4 +211,10 @@ void RootView::RegisterAccelerators(AtomMenuModel* menu_model) {
   }
 }
 
+void RootView::UnregisterAcceleratorsWithFocusManager() {
+  views::FocusManager* focus_manager = GetFocusManager();
+  accelerator_table_.clear();
+  focus_manager->UnregisterAccelerators(this);
+}
+
 }  // namespace atom
diff --git a/atom/browser/ui/views/root_view.h b/atom/browser/ui/views/root_view.h
index b5b718f28fc9..72d22d16c0f4 100644
--- a/atom/browser/ui/views/root_view.h
+++ b/atom/browser/ui/views/root_view.h
@@ -36,6 +36,9 @@ class RootView : public views::View {
   void HandleKeyEvent(const content::NativeWebKeyboardEvent& event);
   void ResetAltState();
   void RestoreFocus();
+  // Register/Unregister accelerators supported by the menu model.
+  void RegisterAcceleratorsWithFocusManager(AtomMenuModel* menu_model);
+  void UnregisterAcceleratorsWithFocusManager();
 
   // views::View:
   void Layout() override;
@@ -44,9 +47,6 @@ class RootView : public views::View {
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
 
  private:
-  // Register accelerators supported by the menu model.
-  void RegisterAccelerators(AtomMenuModel* menu_model);
-
   // Parent window, weak ref.
   NativeWindow* window_;
 
diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js
index 8693290523a2..7b9a8a410b86 100644
--- a/spec/api-menu-spec.js
+++ b/spec/api-menu-spec.js
@@ -742,4 +742,46 @@ describe('Menu module', () => {
       expect(Menu.getApplicationMenu()).to.be.null()
     })
   })
+
+  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
+    }
+    const sendRobotjsKey = (key, modifiers = [], delay = 500) => {
+      return new Promise((resolve, reject) => {
+        require('robotjs').keyTap(key, modifiers)
+        setTimeout(() => {
+          resolve()
+        }, delay)
+      })
+    }
+
+    testFn('menu accelerators perform the specified action', async () => {
+      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()
+              },
+              id: 'test'
+            }
+          ]
+        }
+      ])
+      Menu.setApplicationMenu(menu)
+      expect(Menu.getApplicationMenu()).to.not.be.null()
+      await sendRobotjsKey('t', 'control')
+    })
+  })
 })