From 1c71579bbe3453c791774fb556652b746708e632 Mon Sep 17 00:00:00 2001 From: Profesor08 Date: Tue, 8 Feb 2022 06:23:51 -0800 Subject: [PATCH] #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 | 3 ++ src/main/views/MattermostView.test.js | 27 ++++++++++------ src/main/views/MattermostView.ts | 46 ++++++++++++++++----------- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/e2e/specs/menu_bar/menu.test.js b/e2e/specs/menu_bar/menu.test.js index 6a64266c..953f07d9 100644 --- a/e2e/specs/menu_bar/menu.test.js +++ b/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'); diff --git a/src/main/views/MattermostView.test.js b/src/main/views/MattermostView.test.js index 3d77d863..7426a177 100644 --- a/src/main/views/MattermostView.test.js +++ b/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'); diff --git a/src/main/views/MattermostView.ts b/src/main/views/MattermostView.ts index c266c843..6c928c24 100644 --- a/src/main/views/MattermostView.ts +++ b/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+ 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(); } }