#1960 fix unexpected top menu focus (#1963)

* #1960 fix unexpected top menu focus

* #1960 test update

* #1960 improved test

* #1960 optimized algrithm to handle only Alt key to improve performance and avoid side effects

* #1960 tests update

* #1960 fixed 3 dit menu focus on workspace change with Alt+1, Alt+2

* #313 fix linter issue

* #1960 fix linter issue

* tests update

e2e/specs/menu_bar/menu.test.js

@@ -35,6 +35,9 @@ describe('menu/menu', function desc() {
             const mainWindow = this.app.windows().find((window) => window.url().includes('index'));
             mainWindow.should.not.be.null;
 
+            await mainWindow.bringToFront();
+            await mainWindow.click('#app');
+
             // Settings window should open if Alt works
             robot.keyTap('alt');
             robot.keyTap('enter');

src/main/views/MattermostView.test.js

@@ -56,7 +56,8 @@ const tabView = new MessagingTabView(server);
 
 describe('main/views/MattermostView', () => {
     describe('load', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
             mattermostView.loadSuccess = jest.fn();
@@ -102,7 +103,8 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('retry', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
             mattermostView.view.webContents.loadURL.mockImplementation(() => Promise.resolve());
@@ -153,7 +155,8 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('loadSuccess', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
             jest.useFakeTimers();
@@ -173,7 +176,7 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('show', () => {
-        const window = {addBrowserView: jest.fn(), removeBrowserView: jest.fn()};
+        const window = {addBrowserView: jest.fn(), removeBrowserView: jest.fn(), on: jest.fn()};
         const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
@@ -218,7 +221,7 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('destroy', () => {
-        const window = {removeBrowserView: jest.fn()};
+        const window = {removeBrowserView: jest.fn(), on: jest.fn()};
         const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
@@ -245,10 +248,11 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('handleInputEvents', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         it('should open three dot menu on pressing Alt', () => {
-            mattermostView.handleInputEvents(null, {key: 'Alt', type: 'keyDown'});
+            mattermostView.handleInputEvents(null, {key: 'Alt', type: 'keyDown', alt: true, shift: false, control: false, meta: false});
             mattermostView.handleInputEvents(null, {key: 'Alt', type: 'keyUp'});
             expect(WindowManager.focusThreeDotMenu).toHaveBeenCalled();
         });
@@ -268,7 +272,8 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('handleDidNavigate', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
             mattermostView.setBounds = jest.fn();
@@ -286,7 +291,8 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('handleUpdateTarget', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         beforeEach(() => {
             mattermostView.emit = jest.fn();
@@ -310,7 +316,8 @@ describe('main/views/MattermostView', () => {
     });
 
     describe('updateMentionsFromTitle', () => {
-        const mattermostView = new MattermostView(tabView, {}, {}, {});
+        const window = {on: jest.fn()};
+        const mattermostView = new MattermostView(tabView, {}, window, {});
 
         it('should parse mentions from title', () => {
             mattermostView.updateMentionsFromTitle('(7) Mattermost');

src/main/views/MattermostView.ts

@@ -62,14 +62,14 @@ export class MattermostView extends EventEmitter {
 
     currentFavicon?: string;
     hasBeenShown: boolean;
-    altTimeout?: number;
-    altLastPressed?: boolean;
     contextMenu: ContextMenu;
 
     status?: Status;
     retryLoad?: NodeJS.Timeout;
     maxRetries: number;
 
+    private altPressStatus: boolean;
+
     constructor(tab: TabView, serverInfo: ServerInfo, win: BrowserWindow, options: BrowserViewConstructorOptions) {
         super();
         this.tab = tab;
@@ -98,7 +98,6 @@ export class MattermostView extends EventEmitter {
         this.hasBeenShown = false;
 
         if (process.platform !== 'darwin') {
-            this.altLastPressed = false;
             this.view.webContents.on('before-input-event', this.handleInputEvents);
         }
 
@@ -118,6 +117,12 @@ export class MattermostView extends EventEmitter {
 
         this.contextMenu = new ContextMenu({}, this.view);
         this.maxRetries = MAX_SERVER_RETRIES;
+
+        this.altPressStatus = false;
+
+        this.window.on('blur', () => {
+            this.altPressStatus = false;
+        });
     }
 
     // use the same name as the server
@@ -288,24 +293,27 @@ export class MattermostView extends EventEmitter {
         return this.view.webContents;
     }
 
-    handleInputEvents = (_: Event, input: Input) => {
-        // Handler for pressing the Alt key to focus the 3-dot menu
-        if (input.key === 'Alt' && input.type === 'keyUp' && this.altLastPressed) {
-            this.altLastPressed = false;
-            clearTimeout(this.altTimeout);
-            WindowManager.focusThreeDotMenu();
-            return;
+    private registerAltKeyPressed = (input: Input) => {
+        const isAltPressed = input.key === 'Alt' && input.alt === true && input.control === false && input.shift === false && input.meta === false;
+
+        if (input.type === 'keyDown') {
+            this.altPressStatus = isAltPressed;
         }
 
-        // Hack to detect keyPress so that alt+<key> combinations don't default back to the 3-dot menu
-        if (input.key === 'Alt' && input.type === 'keyDown') {
-            this.altLastPressed = true;
-            this.altTimeout = setTimeout(() => {
-                this.altLastPressed = false;
-            }, 500) as unknown as number;
-        } else {
-            this.altLastPressed = false;
-            clearTimeout(this.altTimeout);
+        if (input.key !== 'Alt') {
+            this.altPressStatus = false;
+        }
+    };
+
+    private isAltKeyReleased = (input: Input) => {
+        return input.type === 'keyUp' && this.altPressStatus === true;
+    };
+
+    handleInputEvents = (_: Event, input: Input) => {
+        this.registerAltKeyPressed(input);
+
+        if (this.isAltKeyReleased(input)) {
+            WindowManager.focusThreeDotMenu();
         }
     }