Fix Enter shortcut for comments within file-list Change 321425 has introduced a regression by removing the shouldSuppress check from the ENTER shortcut on file lists. Also allow "normal" keyboard handlers (outside) of shortcuts-service to check common scenarios of suppression, e.g. focus being in an <input> element. Change-Id: I4e7cba15a94b12f1484cc52f8048b46811ed5539
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts index cc76145..f0d8935 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -421,7 +421,9 @@ }); this.cleanups.push( addGlobalShortcut({key: Key.ESC}, _ => this._handleEscKey()), - addShortcut(this, {key: Key.ENTER}, _ => this.handleOpenFile()) + addShortcut(this, {key: Key.ENTER}, _ => this.handleOpenFile(), { + shouldSuppress: true, + }) ); }
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts index 19f12c5..a26fa08 100644 --- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts +++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -28,6 +28,7 @@ Key, Modifier, Binding, + shouldSuppress, } from '../../utils/dom-util'; import {ReportingService} from '../gr-reporting/gr-reporting'; @@ -154,34 +155,8 @@ shouldSuppress(e: KeyboardEvent) { if (this.shortcutsDisabled) return true; + if (shouldSuppress(e)) return true; - // Note that when you listen on document, then `e.currentTarget` will be the - // document and `e.target` will be `<gr-app>` due to shadow dom, but by - // using the composedPath() you can actually find the true origin of the - // event. - const rootTarget = e.composedPath()[0]; - if (!isElementTarget(rootTarget)) return false; - const tagName = rootTarget.tagName; - const type = rootTarget.getAttribute('type'); - - if ( - // Suppress shortcuts on <input> and <textarea>, but not on - // checkboxes, because we want to enable workflows like 'click - // mark-reviewed and then press ] to go to the next file'. - (tagName === 'INPUT' && type !== 'checkbox') || - tagName === 'TEXTAREA' || - // Suppress shortcuts if the key is 'enter' - // and target is an anchor or button or paper-tab. - (e.keyCode === 13 && - (tagName === 'A' || tagName === 'BUTTON' || tagName === 'PAPER-TAB')) - ) { - return true; - } - const path: EventTarget[] = e.composedPath() ?? []; - for (const el of path) { - if (!isElementTarget(el)) continue; - if (el.tagName === 'GR-OVERLAY') return true; - } // eg: {key: "k:keydown", ..., from: "gr-diff-view"} let key = `${e.key}:${e.type}`; if (this.isInSpecificComboKeyMode(ComboKey.G)) key = 'g+' + key;
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts index b6dece0..e2fa8fe 100644 --- a/polygerrit-ui/app/utils/dom-util.ts +++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -398,10 +398,16 @@ export function addShortcut( element: HTMLElement, shortcut: Binding, - listener: (e: KeyboardEvent) => void + listener: (e: KeyboardEvent) => void, + options: { + shouldSuppress: boolean; + } = { + shouldSuppress: false, + } ) { const wrappedListener = (e: KeyboardEvent) => { if (e.repeat) return; + if (options.shouldSuppress && shouldSuppress(e)) return; if (eventMatchesShortcut(e, shortcut)) { listener(e); } @@ -417,3 +423,43 @@ export function shiftPressed(e: KeyboardEvent) { return e.shiftKey; } + +/** + * When you listen on keyboard events, then within Gerrit's web app you may want + * to avoid firing in certain common scenarios such as key strokes from <input> + * elements. But this can also be undesirable, for example Ctrl-Enter from + * <input> should trigger a save event. + * + * The shortcuts-service has a stateful method `shouldSuppress()` with + * reporting functionality, which delegates to here. + */ +export function shouldSuppress(e: KeyboardEvent): boolean { + // Note that when you listen on document, then `e.currentTarget` will be the + // document and `e.target` will be `<gr-app>` due to shadow dom, but by + // using the composedPath() you can actually find the true origin of the + // event. + const rootTarget = e.composedPath()[0]; + if (!isElementTarget(rootTarget)) return false; + const tagName = rootTarget.tagName; + const type = rootTarget.getAttribute('type'); + + if ( + // Suppress shortcuts on <input> and <textarea>, but not on + // checkboxes, because we want to enable workflows like 'click + // mark-reviewed and then press ] to go to the next file'. + (tagName === 'INPUT' && type !== 'checkbox') || + tagName === 'TEXTAREA' || + // Suppress shortcuts if the key is 'enter' + // and target is an anchor or button or paper-tab. + (e.keyCode === 13 && + (tagName === 'A' || tagName === 'BUTTON' || tagName === 'PAPER-TAB')) + ) { + return true; + } + const path: EventTarget[] = e.composedPath() ?? []; + for (const el of path) { + if (!isElementTarget(el)) continue; + if (el.tagName === 'GR-OVERLAY') return true; + } + return false; +}
diff --git a/polygerrit-ui/app/utils/dom-util_test.ts b/polygerrit-ui/app/utils/dom-util_test.ts index 2993c0e..9dd5be2 100644 --- a/polygerrit-ui/app/utils/dom-util_test.ts +++ b/polygerrit-ui/app/utils/dom-util_test.ts
@@ -22,6 +22,7 @@ getEventPath, Modifier, querySelectorAll, + shouldSuppress, strToClassName, } from './dom-util'; import {PolymerElement} from '@polymer/polymer/polymer-element'; @@ -29,6 +30,22 @@ import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions'; import {queryAndAssert} from '../test/test-utils'; +async function keyEventOn( + el: HTMLElement, + callback: (e: KeyboardEvent) => void, + keyCode = 75, + key = 'k' +): Promise<KeyboardEvent> { + let resolve: (e: KeyboardEvent) => void; + const promise = new Promise<KeyboardEvent>(r => (resolve = r)); + el.addEventListener('keydown', (e: KeyboardEvent) => { + callback(e); + resolve(e); + }); + MockInteractions.keyDownOn(el, keyCode, null, key); + return await promise; +} + class TestEle extends PolymerElement { static get is() { return 'dom-util-test-element'; @@ -266,4 +283,53 @@ assert.isTrue(eventMatchesShortcut(e, sShift)); }); }); + + suite('shouldSuppress', () => { + test('do not suppress shortcut event from <div>', async () => { + await keyEventOn(document.createElement('div'), e => { + assert.isFalse(shouldSuppress(e)); + }); + }); + + test('suppress shortcut event from <input>', async () => { + await keyEventOn(document.createElement('input'), e => { + assert.isTrue(shouldSuppress(e)); + }); + }); + + test('suppress shortcut event from <textarea>', async () => { + await keyEventOn(document.createElement('textarea'), e => { + assert.isTrue(shouldSuppress(e)); + }); + }); + + test('do not suppress shortcut event from checkbox <input>', async () => { + const inputEl = document.createElement('input'); + inputEl.setAttribute('type', 'checkbox'); + await keyEventOn(inputEl, e => { + assert.isFalse(shouldSuppress(e)); + }); + }); + + test('suppress shortcut event from children of <gr-overlay>', async () => { + const overlay = document.createElement('gr-overlay'); + const div = document.createElement('div'); + overlay.appendChild(div); + await keyEventOn(div, e => { + assert.isTrue(shouldSuppress(e)); + }); + }); + + test('suppress "enter" shortcut event from <a>', async () => { + await keyEventOn(document.createElement('a'), e => { + assert.isFalse(shouldSuppress(e)); + }); + await keyEventOn( + document.createElement('a'), + e => assert.isTrue(shouldSuppress(e)), + 13, + 'enter' + ); + }); + }); });