From: Jeremy Davis Date: Thu, 5 May 2022 12:30:36 +0000 (+0200) Subject: SONAR-16339 Replace keymaster in issues and hotspots X-Git-Tag: 9.5.0.56709~175 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=9ac71a6077ea468ab40086968cab2d1ae22ef766;p=sonarqube.git SONAR-16339 Replace keymaster in issues and hotspots --- diff --git a/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx b/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx index 05089203608..543bf234d68 100644 --- a/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx +++ b/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx @@ -18,7 +18,6 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ import styled from '@emotion/styled'; -import key from 'keymaster'; import { debounce, keyBy, omit, without } from 'lodash'; import * as React from 'react'; import { Helmet } from 'react-helmet-async'; @@ -240,61 +239,68 @@ export default class App extends React.PureComponent { } attachShortcuts() { - key.setScope('issues'); - key('up', 'issues', () => { - this.selectPreviousIssue(); - return false; - }); - key('down', 'issues', () => { - this.selectNextIssue(); - return false; - }); - key('right', 'issues', () => { - this.openSelectedIssue(); - return false; - }); - key('left', 'issues', () => { - if (this.state.query.issues.length !== 1) { - this.closeIssue(); - } - return false; - }); - window.addEventListener('keydown', this.handleKeyDown); - window.addEventListener('keyup', this.handleKeyUp); + document.addEventListener('keydown', this.handleKeyDown); + document.addEventListener('keyup', this.handleKeyUp); } detachShortcuts() { - key.deleteScope('issues'); - window.removeEventListener('keydown', this.handleKeyDown); - window.removeEventListener('keyup', this.handleKeyUp); + document.removeEventListener('keydown', this.handleKeyDown); + document.removeEventListener('keyup', this.handleKeyUp); } handleKeyDown = (event: KeyboardEvent) => { - if (key.getScope() !== 'issues') { + // Ignore if modal is open + if (this.state.bulkChangeModal) { return; } + if (event.key === KeyboardKeys.Alt) { event.preventDefault(); this.setState(actions.enableLocationsNavigator); - } else if (event.code === KeyboardCodes.DownArrow && event.altKey) { - event.preventDefault(); - this.selectNextLocation(); - } else if (event.code === KeyboardCodes.UpArrow && event.altKey) { - event.preventDefault(); - this.selectPreviousLocation(); - } else if (event.code === KeyboardCodes.LeftArrow && event.altKey) { - event.preventDefault(); - this.selectPreviousFlow(); - } else if (event.code === KeyboardCodes.RightArrow && event.altKey) { - event.preventDefault(); - this.selectNextFlow(); + return; + } + + switch (event.code) { + case KeyboardCodes.DownArrow: { + event.preventDefault(); + if (event.altKey) { + this.selectNextLocation(); + } else { + this.selectNextIssue(); + } + break; + } + case KeyboardCodes.UpArrow: { + event.preventDefault(); + if (event.altKey) { + this.selectPreviousLocation(); + } else { + this.selectPreviousIssue(); + } + break; + } + case KeyboardCodes.LeftArrow: { + event.preventDefault(); + if (event.altKey) { + this.selectPreviousFlow(); + } else { + this.closeIssue(); + } + break; + } + case KeyboardCodes.RightArrow: { + event.preventDefault(); + if (event.altKey) { + this.selectNextFlow(); + } else { + this.openSelectedIssue(); + } + break; + } } }; handleKeyUp = (event: KeyboardEvent) => { - if (key.getScope() !== 'issues') { - return; - } if (event.key === KeyboardKeys.Alt) { this.setState(actions.disableLocationsNavigator); } @@ -789,12 +795,10 @@ export default class App extends React.PureComponent { }; handleOpenBulkChange = () => { - key.setScope('issues-bulk-change'); this.setState({ bulkChangeModal: true }); }; handleCloseBulkChange = () => { - key.setScope('issues'); this.setState({ bulkChangeModal: false }); }; diff --git a/server/sonar-web/src/main/js/apps/issues/components/__tests__/IssuesApp-test.tsx b/server/sonar-web/src/main/js/apps/issues/components/__tests__/IssuesApp-test.tsx index 1727414d517..69bd17793da 100644 --- a/server/sonar-web/src/main/js/apps/issues/components/__tests__/IssuesApp-test.tsx +++ b/server/sonar-web/src/main/js/apps/issues/components/__tests__/IssuesApp-test.tsx @@ -18,7 +18,6 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ import { shallow } from 'enzyme'; -import key from 'keymaster'; import * as React from 'react'; import { searchIssues } from '../../../../api/issues'; import handleRequiredAuthentication from '../../../../helpers/handleRequiredAuthentication'; @@ -40,7 +39,7 @@ import { mockRawIssue, mockRouter } from '../../../../helpers/testMocks'; -import { KEYCODE_MAP, keydown, waitAndUpdate } from '../../../../helpers/testUtils'; +import { keydown, waitAndUpdate } from '../../../../helpers/testUtils'; import { ComponentQualifier } from '../../../../types/component'; import { ReferencedComponent } from '../../../../types/issues'; import { Issue, Paging } from '../../../../types/types'; @@ -65,27 +64,6 @@ jest.mock('../../../../helpers/pages', () => ({ jest.mock('../../../../helpers/handleRequiredAuthentication', () => jest.fn()); -jest.mock('keymaster', () => { - const key: any = (bindKey: string, _: string, callback: Function) => { - document.addEventListener('keydown', (event: KeyboardEvent) => { - const keymasterCode = event.code && KEYCODE_MAP[event.code as KeyboardCodes]; - if (keymasterCode && bindKey.split(',').includes(keymasterCode)) { - return callback(); - } - return true; - }); - }; - let scope = 'issues'; - - key.getScope = () => scope; - key.setScope = (newScope: string) => { - scope = newScope; - }; - key.deleteScope = jest.fn(); - - return key; -}); - jest.mock('../../../../api/issues', () => ({ searchIssues: jest.fn().mockResolvedValue({ facets: [], issues: [] }) })); @@ -107,17 +85,7 @@ const PAGING = { pageIndex: 1, pageSize: 100, total: 4 }; const referencedComponent: ReferencedComponent = { key: 'foo-key', name: 'bar', uuid: 'foo-uuid' }; -const originalAddEventListener = window.addEventListener; -const originalRemoveEventListener = window.removeEventListener; - beforeEach(() => { - Object.defineProperty(window, 'addEventListener', { - value: jest.fn() - }); - Object.defineProperty(window, 'removeEventListener', { - value: jest.fn() - }); - (searchIssues as jest.Mock).mockResolvedValue({ components: [referencedComponent], effortTotal: 1, @@ -131,13 +99,6 @@ beforeEach(() => { }); afterEach(() => { - Object.defineProperty(window, 'addEventListener', { - value: originalAddEventListener - }); - Object.defineProperty(window, 'removeEventListener', { - value: originalRemoveEventListener - }); - jest.clearAllMocks(); (searchIssues as jest.Mock).mockReset(); }); @@ -243,9 +204,12 @@ it('should switch to source view if an issue is selected', async () => { it('should correctly bind key events for issue navigation', async () => { const push = jest.fn(); + const addEventListenerSpy = jest.spyOn(document, 'addEventListener'); const wrapper = shallowRender({ router: mockRouter({ push }) }); await waitAndUpdate(wrapper); + expect(addEventListenerSpy).toBeCalledTimes(2); + expect(wrapper.state('selected')).toBe(ISSUES[0].key); keydown({ code: KeyboardCodes.DownArrow }); @@ -268,17 +232,20 @@ it('should correctly bind key events for issue navigation', async () => { keydown({ code: KeyboardCodes.LeftArrow }); expect(push).toBeCalledTimes(2); - expect(window.addEventListener).toBeCalledTimes(2); + + addEventListenerSpy.mockReset(); }); it('should correctly clean up on unmount', () => { + const removeEventListenerSpy = jest.spyOn(document, 'removeEventListener'); const wrapper = shallowRender(); wrapper.unmount(); - expect(key.deleteScope).toBeCalled(); expect(removeSideBarClass).toBeCalled(); expect(removeWhitePageClass).toBeCalled(); - expect(window.removeEventListener).toBeCalledTimes(2); + expect(removeEventListenerSpy).toBeCalledTimes(2); + + removeEventListenerSpy.mockReset(); }); it('should be able to bulk change specific issues', async () => { @@ -460,10 +427,6 @@ describe('keydown event handler', () => { jest.resetAllMocks(); }); - afterEach(() => { - key.setScope('issues'); - }); - it('should handle alt', () => { instance.handleKeyDown(mockEvent({ key: KeyboardKeys.Alt })); expect(instance.setState).toHaveBeenCalledWith(enableLocationsNavigator); @@ -484,8 +447,8 @@ describe('keydown event handler', () => { instance.handleKeyDown(mockEvent({ altKey: true, code: KeyboardCodes.RightArrow })); expect(instance.setState).toHaveBeenCalledWith(selectNextFlow); }); - it('should ignore different scopes', () => { - key.setScope('notissues'); + it('should ignore if modal is open', () => { + wrapper.setState({ bulkChangeModal: true }); instance.handleKeyDown(mockEvent({ key: KeyboardKeys.Alt })); expect(instance.setState).not.toHaveBeenCalled(); }); @@ -500,20 +463,10 @@ describe('keyup event handler', () => { jest.resetAllMocks(); }); - afterEach(() => { - key.setScope('issues'); - }); - it('should handle alt', () => { instance.handleKeyUp(mockEvent({ key: KeyboardKeys.Alt })); expect(instance.setState).toHaveBeenCalledWith(disableLocationsNavigator); }); - - it('should ignore different scopes', () => { - key.setScope('notissues'); - instance.handleKeyUp(mockEvent({ key: KeyboardKeys.Alt })); - expect(instance.setState).not.toHaveBeenCalled(); - }); }); it('should fetch more issues', async () => { diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/SecurityHotspotsApp.tsx b/server/sonar-web/src/main/js/apps/security-hotspots/SecurityHotspotsApp.tsx index f9814f838c7..38c3fa7920c 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/SecurityHotspotsApp.tsx +++ b/server/sonar-web/src/main/js/apps/security-hotspots/SecurityHotspotsApp.tsx @@ -18,7 +18,6 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ import { Location } from 'history'; -import key from 'keymaster'; import { flatMap, range } from 'lodash'; import * as React from 'react'; import { getMeasures } from '../../api/measures'; @@ -46,7 +45,6 @@ import SecurityHotspotsAppRenderer from './SecurityHotspotsAppRenderer'; import './styles.css'; import { getLocations, SECURITY_STANDARDS } from './utils'; -const HOTSPOT_KEYMASTER_SCOPE = 'hotspots-list'; const PAGE_SIZE = 500; interface DispatchProps { fetchBranchStatus: (branchLike: BranchLike, projectKey: string) => void; @@ -143,27 +141,34 @@ export class SecurityHotspotsApp extends React.PureComponent { } registerKeyboardEvents() { - key.setScope(HOTSPOT_KEYMASTER_SCOPE); - key('up', HOTSPOT_KEYMASTER_SCOPE, () => { - this.selectNeighboringHotspot(-1); - return false; - }); - key('down', HOTSPOT_KEYMASTER_SCOPE, () => { - this.selectNeighboringHotspot(+1); - return false; - }); - window.addEventListener('keydown', this.handleKeyDown); + document.addEventListener('keydown', this.handleKeyDown); } handleKeyDown = (event: KeyboardEvent) => { if (event.key === KeyboardKeys.Alt) { event.preventDefault(); - } else if (event.code === KeyboardCodes.DownArrow && event.altKey) { - event.preventDefault(); - this.selectNextLocation(); - } else if (event.code === KeyboardCodes.UpArrow && event.altKey) { - event.preventDefault(); - this.selectPreviousLocation(); + return; + } + + switch (event.code) { + case KeyboardCodes.DownArrow: { + event.preventDefault(); + if (event.altKey) { + this.selectNextLocation(); + } else { + this.selectNeighboringHotspot(+1); + } + break; + } + case KeyboardCodes.UpArrow: { + event.preventDefault(); + if (event.altKey) { + this.selectPreviousLocation(); + } else { + this.selectNeighboringHotspot(-1); + } + break; + } } }; @@ -218,8 +223,7 @@ export class SecurityHotspotsApp extends React.PureComponent { }; unregisterKeyboardEvents() { - key.deleteScope(HOTSPOT_KEYMASTER_SCOPE); - window.removeEventListener('keydown', this.handleKeyDown); + document.removeEventListener('keydown', this.handleKeyDown); } constructFiltersFromProps( diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/__tests__/SecurityHotspotsApp-test.tsx b/server/sonar-web/src/main/js/apps/security-hotspots/__tests__/SecurityHotspotsApp-test.tsx index 1ab4d4ade68..c5f74d94645 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/__tests__/SecurityHotspotsApp-test.tsx +++ b/server/sonar-web/src/main/js/apps/security-hotspots/__tests__/SecurityHotspotsApp-test.tsx @@ -36,7 +36,7 @@ import { mockLoggedInUser, mockRouter } from '../../../helpers/testMocks'; -import { KEYCODE_MAP, waitAndUpdate } from '../../../helpers/testUtils'; +import { waitAndUpdate } from '../../../helpers/testUtils'; import { SecurityStandard } from '../../../types/security'; import { HotspotResolution, @@ -46,28 +46,10 @@ import { import { SecurityHotspotsApp } from '../SecurityHotspotsApp'; import SecurityHotspotsAppRenderer from '../SecurityHotspotsAppRenderer'; -const originalAddEventListener = window.addEventListener; -const originalRemoveEventListener = window.removeEventListener; - beforeEach(() => { - Object.defineProperty(window, 'addEventListener', { - value: jest.fn() - }); - Object.defineProperty(window, 'removeEventListener', { - value: jest.fn() - }); jest.clearAllMocks(); }); -afterEach(() => { - Object.defineProperty(window, 'addEventListener', { - value: originalAddEventListener - }); - Object.defineProperty(window, 'removeEventListener', { - value: originalRemoveEventListener - }); -}); - jest.mock('../../../api/measures', () => ({ getMeasures: jest.fn().mockResolvedValue([]) })); @@ -85,27 +67,6 @@ jest.mock('../../../helpers/scrolling', () => ({ scrollToElement: jest.fn() })); -jest.mock('keymaster', () => { - const key: any = (bindKey: string, _: string, callback: Function) => { - document.addEventListener('keydown', (event: KeyboardEvent) => { - const keymasterCode = event.code && KEYCODE_MAP[event.code as KeyboardCodes]; - if (keymasterCode && bindKey.split(',').includes(keymasterCode)) { - return callback(); - } - return true; - }); - }; - let scope = 'hotspots-list'; - - key.getScope = () => scope; - key.setScope = (newScope: string) => { - scope = newScope; - }; - key.deleteScope = jest.fn(); - - return key; -}); - const branch = mockBranch(); it('should render correctly', () => { diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/components/HotspotSnippetContainerRenderer.tsx b/server/sonar-web/src/main/js/apps/security-hotspots/components/HotspotSnippetContainerRenderer.tsx index 2c6ab3fb8d3..8c6c1ba68fc 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/components/HotspotSnippetContainerRenderer.tsx +++ b/server/sonar-web/src/main/js/apps/security-hotspots/components/HotspotSnippetContainerRenderer.tsx @@ -53,7 +53,6 @@ export interface HotspotSnippetContainerRendererProps { } const noop = () => undefined; -const SCROLL_INTERVAL_DELAY = 10; const SCROLL_DELAY = 100; const EXPAND_ANIMATION_SPEED = 200; @@ -78,21 +77,6 @@ export function getScrollHandler(scrollableRef: React.RefObject) }; } -function scrollToFollowAnimation( - scrollableRef: React.RefObject, - targetHeight: number -) { - const scrollable = scrollableRef.current; - if (scrollable) { - const handler = setInterval(() => { - scrollable.scrollTo({ top: targetHeight, behavior: 'smooth' }); - }, SCROLL_INTERVAL_DELAY); - setTimeout(() => { - clearInterval(handler); - }, EXPAND_ANIMATION_SPEED); - } -} - /* Exported for testing */ export async function animateExpansion( scrollableRef: React.RefObject, @@ -135,7 +119,6 @@ export async function animateExpansion( // False positive: // eslint-disable-next-line require-atomic-updates wrapper.style.maxHeight = `${targetHeight}px`; - scrollToFollowAnimation(scrollableRef, targetHeight); } // after the animation is done, clear the applied styles @@ -218,6 +201,7 @@ export default function HotspotSnippetContainerRenderer( renderAdditionalChildInLine={renderHotspotBoxInLine} renderDuplicationPopup={noop} snippet={sourceLines} + scroll={getScrollHandler(scrollableRef)} /> )} diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/HotspotSnippetContainerRenderer-test.tsx b/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/HotspotSnippetContainerRenderer-test.tsx index b52c08df125..1e58285c233 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/HotspotSnippetContainerRenderer-test.tsx +++ b/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/HotspotSnippetContainerRenderer-test.tsx @@ -151,9 +151,6 @@ describe('expand', () => { expect(onExpandBlock).toBeCalledWith('down'); expect(snippet.style.maxHeight).toBe('112px'); - jest.advanceTimersByTime(250); - expect(scrollableNode.scrollTo).toBeCalled(); - jest.useRealTimers(); }); }); diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/__snapshots__/HotspotSnippetContainerRenderer-test.tsx.snap b/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/__snapshots__/HotspotSnippetContainerRenderer-test.tsx.snap index 2a46900c6ad..be77b3e2008 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/__snapshots__/HotspotSnippetContainerRenderer-test.tsx.snap +++ b/server/sonar-web/src/main/js/apps/security-hotspots/components/__tests__/__snapshots__/HotspotSnippetContainerRenderer-test.tsx.snap @@ -518,6 +518,7 @@ exports[`should render correctly: with sourcelines 1`] = ` openIssuesByLine={Object {}} renderAdditionalChildInLine={[Function]} renderDuplicationPopup={[Function]} + scroll={[Function]} snippet={ Array [ Object {