Add precise modifier matching to the new shortcut handler Also convert the dom-util tests to TypeScript. Google-Bug-Id: b/199305453 Change-Id: I83028effa04bce79192d181b94f5253220805d66
diff --git a/polygerrit-ui/app/test/common-test-setup-karma.ts b/polygerrit-ui/app/test/common-test-setup-karma.ts index 3463d3b..39c79d1 100644 --- a/polygerrit-ui/app/test/common-test-setup-karma.ts +++ b/polygerrit-ui/app/test/common-test-setup-karma.ts
@@ -118,7 +118,7 @@ } class TestFixture { - constructor(private readonly fixtureId: string) {} + constructor(readonly fixtureId: string) {} /** * Create an instance of a fixture's template.
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts index e3e342e..cdcadb0 100644 --- a/polygerrit-ui/app/utils/dom-util.ts +++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -327,26 +327,65 @@ } export interface Shortcut { - key: string; + key: string | Key; modifiers?: Modifier[]; } +const ALPHA_NUM = new RegExp(/^[A-Za-z0-9]$/); + +/** + * For "normal" keys we do not check that the SHIFT modifier is pressed or not, + * because that depends on the keyboard layout. Just checking the key string is + * sufficient. + * + * But for some special keys it is important whether SHIFT is pressed at the + * same time, for example we want to distinguish Enter from Shift+Enter. + */ +function shiftMustMatch(key: string | Key) { + return Object.values(Key).includes(key as Key); +} + +/** + * For a-zA-Z0-9 and for Enter, Tab, etc. we want to check the ALT modifier. + * + * But for special chars like []/? we don't care whether the user is pressing + * the ALT modifier to produce the special char. For example on a German + * keyboard layout you have to press ALT to produce a [. + */ +function altMustMatch(key: string | Key) { + return ALPHA_NUM.test(key) || Object.values(Key).includes(key as Key); +} + +export function eventMatchesShortcut( + e: KeyboardEvent, + shortcut: Shortcut +): boolean { + if (e.key !== shortcut.key) return false; + const modifiers = shortcut.modifiers ?? []; + if (e.ctrlKey !== modifiers.includes(Modifier.CTRL_KEY)) return false; + if (e.metaKey !== modifiers.includes(Modifier.META_KEY)) return false; + if ( + altMustMatch(e.key) && + e.altKey !== modifiers.includes(Modifier.ALT_KEY) + ) { + return false; + } + if ( + shiftMustMatch(e.key) && + e.shiftKey !== modifiers.includes(Modifier.SHIFT_KEY) + ) { + return false; + } + return true; +} + export function addShortcut( shortcut: Shortcut, listener: (e: KeyboardEvent) => void ) { const wrappedListener = (e: KeyboardEvent) => { - if (e.key !== shortcut.key) return; - const modifiers = shortcut.modifiers ?? []; - if (e.ctrlKey !== modifiers.includes(Modifier.CTRL_KEY)) return; - if (e.metaKey !== modifiers.includes(Modifier.META_KEY)) return; - // TODO(brohlfs): Refine the matching of modifiers. For "normal" keys we - // don't want to check ALT and SHIFT, because we don't know what the - // keyboard layout looks like. The user may have to use ALT and/or SHIFT for - // certain keys. Comparing the `key` string is sufficient in that case. - // if (e.altKey !== modifiers.includes(Modifier.ALT_KEY)) return; - // if (e.shiftKey !== modifiers.includes(Modifier.SHIFT_KEY)) return; - listener(e); + if (e.repeat) return; + if (eventMatchesShortcut(e, shortcut)) listener(e); }; document.addEventListener('keydown', wrappedListener); return () => document.removeEventListener('keydown', wrappedListener);
diff --git a/polygerrit-ui/app/utils/dom-util_test.js b/polygerrit-ui/app/utils/dom-util_test.js deleted file mode 100644 index bcb4505..0000000 --- a/polygerrit-ui/app/utils/dom-util_test.js +++ /dev/null
@@ -1,155 +0,0 @@ -/** - * @license - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -import '../test/common-test-setup-karma.js'; -import {strToClassName, getComputedStyleValue, querySelector, querySelectorAll, descendedFromClass, getEventPath} from './dom-util.js'; -import {PolymerElement} from '@polymer/polymer/polymer-element.js'; -import {html} from '@polymer/polymer/lib/utils/html-tag.js'; - -class TestEle extends PolymerElement { - static get is() { - return 'dom-util-test-element'; - } - - static get template() { - return html` - <div> - <div class="a"> - <div class="b"> - <div class="c"></div> - </div> - <span class="ss"></span> - </div> - <span class="ss"></span> - </div> - `; - } -} - -customElements.define(TestEle.is, TestEle); - -const basicFixture = fixtureFromTemplate(html` - <div id="test" class="a b c"> - <a class="testBtn" style="color:red;"></a> - <dom-util-test-element></dom-util-test-element> - <span class="ss"></span> - </div> -`); - -suite('dom-util tests', () => { - suite('getEventPath', () => { - test('empty event', () => { - assert.equal(getEventPath(), ''); - assert.equal(getEventPath(null), ''); - assert.equal(getEventPath(undefined), ''); - assert.equal(getEventPath({composedPath: () => []}), ''); - }); - - test('event with fake path', () => { - assert.equal(getEventPath({composedPath: () => []}), ''); - const dd = document.createElement('dd'); - assert.equal(getEventPath({composedPath: () => [dd]}), 'dd'); - }); - - test('event with fake complicated path', () => { - const dd = document.createElement('dd'); - dd.setAttribute('id', 'test'); - dd.className = 'a b'; - const divNode = document.createElement('DIV'); - divNode.id = 'test2'; - divNode.className = 'a b c'; - assert.equal(getEventPath( - {composedPath: () => [dd, divNode]}), - 'div#test2.a.b.c>dd#test.a.b' - ); - }); - - test('event with fake target', () => { - const fakeTargetParent1 = document.createElement('dd'); - fakeTargetParent1.setAttribute('id', 'test'); - fakeTargetParent1.className = 'a b'; - const fakeTargetParent2 = document.createElement('DIV'); - fakeTargetParent2.id = 'test2'; - fakeTargetParent2.className = 'a b c'; - fakeTargetParent2.appendChild(fakeTargetParent1); - const fakeTarget = document.createElement('SPAN'); - fakeTargetParent1.appendChild(fakeTarget); - assert.equal( - getEventPath({composedPath: () => {}, target: fakeTarget}), - 'div#test2.a.b.c>dd#test.a.b>span' - ); - }); - - test('event with real click', () => { - const element = basicFixture.instantiate(); - const aLink = element.querySelector('a'); - let path; - aLink.onclick = e => path = getEventPath(e); - MockInteractions.click(aLink); - assert.equal( - path, - `html>body>test-fixture#${basicFixture.fixtureId}>` + - 'div#test.a.b.c>a.testBtn' - ); - }); - }); - - suite('querySelector and querySelectorAll', () => { - test('query cross shadow dom', () => { - const element = basicFixture.instantiate(); - const theFirstEl = querySelector(element, '.ss'); - const allEls = querySelectorAll(element, '.ss'); - assert.equal(allEls.length, 3); - assert.equal(theFirstEl, allEls[0]); - }); - }); - - suite('getComputedStyleValue', () => { - test('color style', () => { - const element = basicFixture.instantiate(); - const testBtn = querySelector(element, '.testBtn'); - assert.equal( - getComputedStyleValue('color', testBtn), 'rgb(255, 0, 0)' - ); - }); - }); - - suite('descendedFromClass', () => { - test('basic tests', () => { - const element = basicFixture.instantiate(); - const testEl = querySelector(element, 'dom-util-test-element'); - // .c is a child of .a and not vice versa. - assert.isTrue(descendedFromClass(querySelector(testEl, '.c'), 'a')); - assert.isFalse(descendedFromClass(querySelector(testEl, '.a'), 'c')); - - // Stops at stop element. - assert.isFalse(descendedFromClass(querySelector(testEl, '.c'), 'a', - querySelector(testEl, '.b'))); - }); - }); - - suite('strToClassName', () => { - test('basic tests', () => { - assert.equal(strToClassName(''), 'generated_'); - assert.equal(strToClassName('11'), 'generated_11'); - assert.equal(strToClassName('0.123'), 'generated_0_123'); - assert.equal(strToClassName('0.123', 'prefix_'), 'prefix_0_123'); - assert.equal(strToClassName('0>123', 'prefix_'), 'prefix_0_123'); - assert.equal(strToClassName('0<123', 'prefix_'), 'prefix_0_123'); - assert.equal(strToClassName('0+1+23', 'prefix_'), 'prefix_0_1_23'); - }); - }); -}); \ No newline at end of file
diff --git a/polygerrit-ui/app/utils/dom-util_test.ts b/polygerrit-ui/app/utils/dom-util_test.ts new file mode 100644 index 0000000..2993c0e --- /dev/null +++ b/polygerrit-ui/app/utils/dom-util_test.ts
@@ -0,0 +1,269 @@ +/** + * @license + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import '../test/common-test-setup-karma'; +import { + descendedFromClass, + eventMatchesShortcut, + getComputedStyleValue, + getEventPath, + Modifier, + querySelectorAll, + strToClassName, +} from './dom-util'; +import {PolymerElement} from '@polymer/polymer/polymer-element'; +import {html} from '@polymer/polymer/lib/utils/html-tag'; +import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions'; +import {queryAndAssert} from '../test/test-utils'; + +class TestEle extends PolymerElement { + static get is() { + return 'dom-util-test-element'; + } + + static get template() { + return html` + <div> + <div class="a"> + <div class="b"> + <div class="c"></div> + </div> + <span class="ss"></span> + </div> + <span class="ss"></span> + </div> + `; + } +} + +customElements.define(TestEle.is, TestEle); + +const basicFixture = fixtureFromTemplate(html` + <div id="test" class="a b c"> + <a class="testBtn" style="color:red;"></a> + <dom-util-test-element></dom-util-test-element> + <span class="ss"></span> + </div> +`); + +suite('dom-util tests', () => { + suite('getEventPath', () => { + test('empty event', () => { + assert.equal(getEventPath(), ''); + assert.equal(getEventPath(undefined), ''); + assert.equal(getEventPath(new MouseEvent('click')), ''); + }); + + test('event with fake path', () => { + assert.equal(getEventPath(new MouseEvent('click')), ''); + const dd = document.createElement('dd'); + assert.equal( + getEventPath({...new MouseEvent('click'), composedPath: () => [dd]}), + 'dd' + ); + }); + + test('event with fake complicated path', () => { + const dd = document.createElement('dd'); + dd.setAttribute('id', 'test'); + dd.className = 'a b'; + const divNode = document.createElement('DIV'); + divNode.id = 'test2'; + divNode.className = 'a b c'; + assert.equal( + getEventPath({ + ...new MouseEvent('click'), + composedPath: () => [dd, divNode], + }), + 'div#test2.a.b.c>dd#test.a.b' + ); + }); + + test('event with fake target', () => { + const fakeTargetParent1 = document.createElement('dd'); + fakeTargetParent1.setAttribute('id', 'test'); + fakeTargetParent1.className = 'a b'; + const fakeTargetParent2 = document.createElement('DIV'); + fakeTargetParent2.id = 'test2'; + fakeTargetParent2.className = 'a b c'; + fakeTargetParent2.appendChild(fakeTargetParent1); + const fakeTarget = document.createElement('SPAN'); + fakeTargetParent1.appendChild(fakeTarget); + assert.equal( + getEventPath({ + ...new MouseEvent('click'), + composedPath: () => [], + target: fakeTarget, + }), + 'div#test2.a.b.c>dd#test.a.b>span' + ); + }); + + test('event with real click', () => { + const element = basicFixture.instantiate() as HTMLElement; + const aLink = queryAndAssert(element, 'a'); + let path; + aLink.addEventListener('click', (e: Event) => { + path = getEventPath(e as MouseEvent); + }); + MockInteractions.click(aLink); + assert.equal( + path, + `html>body>test-fixture#${basicFixture.fixtureId}>` + + 'div#test.a.b.c>a.testBtn' + ); + }); + }); + + suite('querySelector and querySelectorAll', () => { + test('query cross shadow dom', () => { + const element = basicFixture.instantiate() as HTMLElement; + const theFirstEl = queryAndAssert(element, '.ss'); + const allEls = querySelectorAll(element, '.ss'); + assert.equal(allEls.length, 3); + assert.equal(theFirstEl, allEls[0]); + }); + }); + + suite('getComputedStyleValue', () => { + test('color style', () => { + const element = basicFixture.instantiate() as HTMLElement; + const testBtn = queryAndAssert(element, '.testBtn'); + assert.equal(getComputedStyleValue('color', testBtn), 'rgb(255, 0, 0)'); + }); + }); + + suite('descendedFromClass', () => { + test('basic tests', () => { + const element = basicFixture.instantiate() as HTMLElement; + const testEl = queryAndAssert(element, 'dom-util-test-element'); + // .c is a child of .a and not vice versa. + assert.isTrue(descendedFromClass(queryAndAssert(testEl, '.c'), 'a')); + assert.isFalse(descendedFromClass(queryAndAssert(testEl, '.a'), 'c')); + + // Stops at stop element. + assert.isFalse( + descendedFromClass( + queryAndAssert(testEl, '.c'), + 'a', + queryAndAssert(testEl, '.b') + ) + ); + }); + }); + + suite('strToClassName', () => { + test('basic tests', () => { + assert.equal(strToClassName(''), 'generated_'); + assert.equal(strToClassName('11'), 'generated_11'); + assert.equal(strToClassName('0.123'), 'generated_0_123'); + assert.equal(strToClassName('0.123', 'prefix_'), 'prefix_0_123'); + assert.equal(strToClassName('0>123', 'prefix_'), 'prefix_0_123'); + assert.equal(strToClassName('0<123', 'prefix_'), 'prefix_0_123'); + assert.equal(strToClassName('0+1+23', 'prefix_'), 'prefix_0_1_23'); + }); + }); + + suite('eventMatchesShortcut', () => { + test('basic tests', () => { + const a = new KeyboardEvent('keydown', {key: 'a'}); + const b = new KeyboardEvent('keydown', {key: 'B'}); + assert.isTrue(eventMatchesShortcut(a, {key: 'a'})); + assert.isFalse(eventMatchesShortcut(a, {key: 'B'})); + assert.isFalse(eventMatchesShortcut(b, {key: 'a'})); + assert.isTrue(eventMatchesShortcut(b, {key: 'B'})); + }); + + test('check modifiers for a', () => { + const e = new KeyboardEvent('keydown', {key: 'a'}); + const s = {key: 'a'}; + assert.isTrue(eventMatchesShortcut(e, s)); + + const eAlt = new KeyboardEvent('keydown', {key: 'a', altKey: true}); + const sAlt = {key: 'a', modifiers: [Modifier.ALT_KEY]}; + assert.isFalse(eventMatchesShortcut(eAlt, s)); + assert.isFalse(eventMatchesShortcut(e, sAlt)); + const eCtrl = new KeyboardEvent('keydown', {key: 'a', ctrlKey: true}); + const sCtrl = {key: 'a', modifiers: [Modifier.CTRL_KEY]}; + assert.isFalse(eventMatchesShortcut(eCtrl, s)); + assert.isFalse(eventMatchesShortcut(e, sCtrl)); + const eMeta = new KeyboardEvent('keydown', {key: 'a', metaKey: true}); + const sMeta = {key: 'a', modifiers: [Modifier.META_KEY]}; + assert.isFalse(eventMatchesShortcut(eMeta, s)); + assert.isFalse(eventMatchesShortcut(e, sMeta)); + + // Do NOT check SHIFT for alphanum keys. + const eShift = new KeyboardEvent('keydown', {key: 'a', shiftKey: true}); + const sShift = {key: 'a', modifiers: [Modifier.SHIFT_KEY]}; + assert.isTrue(eventMatchesShortcut(eShift, s)); + assert.isTrue(eventMatchesShortcut(e, sShift)); + }); + + test('check modifiers for Enter', () => { + const e = new KeyboardEvent('keydown', {key: 'Enter'}); + const s = {key: 'Enter'}; + assert.isTrue(eventMatchesShortcut(e, s)); + + const eAlt = new KeyboardEvent('keydown', {key: 'Enter', altKey: true}); + const sAlt = {key: 'Enter', modifiers: [Modifier.ALT_KEY]}; + assert.isFalse(eventMatchesShortcut(eAlt, s)); + assert.isFalse(eventMatchesShortcut(e, sAlt)); + const eCtrl = new KeyboardEvent('keydown', {key: 'Enter', ctrlKey: true}); + const sCtrl = {key: 'Enter', modifiers: [Modifier.CTRL_KEY]}; + assert.isFalse(eventMatchesShortcut(eCtrl, s)); + assert.isFalse(eventMatchesShortcut(e, sCtrl)); + const eMeta = new KeyboardEvent('keydown', {key: 'Enter', metaKey: true}); + const sMeta = {key: 'Enter', modifiers: [Modifier.META_KEY]}; + assert.isFalse(eventMatchesShortcut(eMeta, s)); + assert.isFalse(eventMatchesShortcut(e, sMeta)); + const eShift = new KeyboardEvent('keydown', { + key: 'Enter', + shiftKey: true, + }); + const sShift = {key: 'Enter', modifiers: [Modifier.SHIFT_KEY]}; + assert.isFalse(eventMatchesShortcut(eShift, s)); + assert.isFalse(eventMatchesShortcut(e, sShift)); + }); + + test('check modifiers for [', () => { + const e = new KeyboardEvent('keydown', {key: '['}); + const s = {key: '['}; + assert.isTrue(eventMatchesShortcut(e, s)); + + const eCtrl = new KeyboardEvent('keydown', {key: '[', ctrlKey: true}); + const sCtrl = {key: '[', modifiers: [Modifier.CTRL_KEY]}; + assert.isFalse(eventMatchesShortcut(eCtrl, s)); + assert.isFalse(eventMatchesShortcut(e, sCtrl)); + const eMeta = new KeyboardEvent('keydown', {key: '[', metaKey: true}); + const sMeta = {key: '[', modifiers: [Modifier.META_KEY]}; + assert.isFalse(eventMatchesShortcut(eMeta, s)); + assert.isFalse(eventMatchesShortcut(e, sMeta)); + + // Do NOT check SHIFT and ALT for special chars like [. + const eAlt = new KeyboardEvent('keydown', {key: '[', altKey: true}); + const sAlt = {key: '[', modifiers: [Modifier.ALT_KEY]}; + assert.isTrue(eventMatchesShortcut(eAlt, s)); + assert.isTrue(eventMatchesShortcut(e, sAlt)); + const eShift = new KeyboardEvent('keydown', { + key: '[', + shiftKey: true, + }); + const sShift = {key: '[', modifiers: [Modifier.SHIFT_KEY]}; + assert.isTrue(eventMatchesShortcut(eShift, s)); + assert.isTrue(eventMatchesShortcut(e, sShift)); + }); + }); +});