Merge changes I70842e26,I74131118
* changes:
Do not show hovercards when the target pops up under the mouse
Fix a regression with bolded buttons
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 975ad52..8f0b535 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -943,8 +943,7 @@
if (lowerNames.containsKey(lower)) {
error(
String.format(
- "Submit requirement \"%s\" conflicts with \"%s\". Skipping the former.",
- name, lowerNames.get(lower)));
+ "Submit requirement '%s' conflicts with '%s'.", name, lowerNames.get(lower)));
continue;
}
lowerNames.put(lower, name);
@@ -958,9 +957,7 @@
if (blockExpr == null) {
error(
String.format(
- "Submit requirement \"%s\" does not define a submittability expression."
- + " Skipping this requirement.",
- name));
+ "Submit requirement '%s' does not define a submittability expression.", name));
continue;
}
diff --git a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsValidationIT.java b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsValidationIT.java
new file mode 100644
index 0000000..4675bc0
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsValidationIT.java
@@ -0,0 +1,252 @@
+// Copyright (C) 2021 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.
+
+package com.google.gerrit.acceptance.server.project;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.gerrit.acceptance.GitUtil.fetch;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.project.ProjectConfig;
+import java.util.Locale;
+import java.util.function.Consumer;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
+import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.util.RawParseUtils;
+import org.junit.Test;
+
+/**
+ * Tests validating submit requirements on upload of {@code project.config} to {@code
+ * refs/meta/config}.
+ */
+public class SubmitRequirementsValidationIT extends AbstractDaemonTest {
+ @Test
+ public void validSubmitRequirementIsAccepted_optionalParametersNotSet() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\""));
+
+ PushResult r = pushRefsMetaConfig();
+ assertOkStatus(r);
+ }
+
+ @Test
+ public void validSubmitRequirementIsAccepted_allParametersSet() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig -> {
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_DESCRIPTION,
+ /* value= */ "foo bar description");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_APPLICABILITY_EXPRESSION,
+ /* value= */ "branch:refs/heads/master");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\"");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_OVERRIDE_EXPRESSION,
+ /* value= */ "label:\"override=+1\"");
+ projectConfig.setBoolean(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_OVERRIDE_IN_CHILD_PROJECTS,
+ /* value= */ false);
+ });
+
+ PushResult r = pushRefsMetaConfig();
+ assertOkStatus(r);
+ }
+
+ @Test
+ public void conflictingSubmitRequirementsAreRejected() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig -> {
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\"");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName.toLowerCase(Locale.US),
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\"");
+ });
+
+ PushResult r = pushRefsMetaConfig();
+ assertErrorStatus(
+ r,
+ "Invalid project configuration",
+ String.format(
+ "project.config: Submit requirement '%s' conflicts with '%s'.",
+ submitRequirementName.toLowerCase(Locale.US), submitRequirementName));
+ }
+
+ @Test
+ public void conflictingSubmitRequirementIsRejected() throws Exception {
+ fetchRefsMetaConfig();
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\""));
+ PushResult r = pushRefsMetaConfig();
+ assertOkStatus(r);
+
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName.toLowerCase(Locale.US),
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\""));
+ r = pushRefsMetaConfig();
+ assertErrorStatus(
+ r,
+ "Invalid project configuration",
+ String.format(
+ "project.config: Submit requirement '%s' conflicts with '%s'.",
+ submitRequirementName.toLowerCase(Locale.US), submitRequirementName));
+ }
+
+ @Test
+ public void submitRequirementWithoutSubmittabilityExpressionIsRejected() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_DESCRIPTION,
+ /* value= */ "foo bar description"));
+
+ PushResult r = pushRefsMetaConfig();
+ assertErrorStatus(
+ r,
+ "Invalid project configuration",
+ String.format(
+ "project.config: Submit requirement '%s' does not define a submittability expression.",
+ submitRequirementName));
+ }
+
+ private void fetchRefsMetaConfig() throws Exception {
+ fetch(testRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
+ testRepo.reset(RefNames.REFS_CONFIG);
+ }
+
+ private PushResult pushRefsMetaConfig() throws Exception {
+ return pushHead(testRepo, RefNames.REFS_CONFIG);
+ }
+
+ private void updateProjectConfig(Consumer<Config> configUpdater) throws Exception {
+ RevCommit head = getHead(testRepo.getRepository(), RefNames.REFS_CONFIG);
+ Config projectConfig = readProjectConfig(head);
+ configUpdater.accept(projectConfig);
+ RevCommit commit =
+ testRepo.update(
+ RefNames.REFS_CONFIG,
+ testRepo
+ .commit()
+ .parent(head)
+ .message("Update project config")
+ .author(admin.newIdent())
+ .committer(admin.newIdent())
+ .add(ProjectConfig.PROJECT_CONFIG, projectConfig.toText()));
+
+ testRepo.reset(commit);
+ }
+
+ private Config readProjectConfig(RevCommit commit) throws Exception {
+ try (TreeWalk tw =
+ TreeWalk.forPath(
+ testRepo.getRevWalk().getObjectReader(),
+ ProjectConfig.PROJECT_CONFIG,
+ commit.getTree())) {
+ if (tw == null) {
+ throw new IllegalStateException(
+ String.format("%s does not exist", ProjectConfig.PROJECT_CONFIG));
+ }
+ }
+ RevObject blob = testRepo.get(commit.getTree(), ProjectConfig.PROJECT_CONFIG);
+ byte[] data = testRepo.getRepository().open(blob).getCachedBytes(Integer.MAX_VALUE);
+ String content = RawParseUtils.decode(data);
+
+ Config projectConfig = new Config();
+ projectConfig.fromText(content);
+ return projectConfig;
+ }
+
+ public void assertOkStatus(PushResult result) {
+ RemoteRefUpdate refUpdate = result.getRemoteUpdate(RefNames.REFS_CONFIG);
+ assertThat(refUpdate).isNotNull();
+ assertWithMessage(getMessage(result, refUpdate))
+ .that(refUpdate.getStatus())
+ .isEqualTo(Status.OK);
+ }
+
+ public void assertErrorStatus(PushResult result, String... expectedMessages) {
+ RemoteRefUpdate refUpdate = result.getRemoteUpdate(RefNames.REFS_CONFIG);
+ assertThat(refUpdate).isNotNull();
+ assertWithMessage(getMessage(result, refUpdate))
+ .that(refUpdate.getStatus())
+ .isEqualTo(Status.REJECTED_OTHER_REASON);
+ for (String expectedMessage : expectedMessages) {
+ assertThat(result.getMessages()).contains(expectedMessage);
+ }
+ }
+
+ private String getMessage(PushResult result, RemoteRefUpdate refUpdate) {
+ StringBuilder b = new StringBuilder();
+ if (refUpdate.getMessage() != null) {
+ b.append(refUpdate.getMessage());
+ b.append("\n");
+ }
+ b.append(result.getMessages());
+ return b.toString();
+ }
+}
diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
index 7f0b685..9df59c2 100644
--- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
+++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
@@ -310,9 +310,7 @@
assertThat(cfg.getValidationErrors()).hasSize(1);
assertThat(Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage())
.isEqualTo(
- "project.config: "
- + "Submit requirement \"Code-Review\" conflicts with \"code-review\". "
- + "Skipping the former.");
+ "project.config: Submit requirement 'Code-Review' conflicts with 'code-review'.");
}
@Test
@@ -332,8 +330,8 @@
assertThat(cfg.getValidationErrors()).hasSize(1);
assertThat(Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage())
.isEqualTo(
- "project.config: Submit requirement \"code-review\" does not define a submittability"
- + " expression. Skipping this requirement.");
+ "project.config: Submit requirement 'code-review' does not define a submittability"
+ + " expression.");
}
@Test
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
index a3db699..a79dd8e 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
@@ -27,6 +27,7 @@
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {
GerritNav,
@@ -46,10 +47,9 @@
PreferencesInput,
} from '../../../types/common';
import {hasAttention} from '../../../utils/attention-set-util';
-import {IronKeyboardEvent} from '../../../types/events';
import {fireEvent, fireReload} from '../../../utils/event-util';
-import {isShiftPressed, modifierPressed} from '../../../utils/dom-util';
import {ScrollMode} from '../../../constants/constants';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
const NUMBER_FIXED_COLUMNS = 3;
const CLOSED_STATUS = ['MERGED', 'ABANDONED'];
@@ -135,9 +135,6 @@
@property({type: Boolean})
showReviewedState = false;
- @property({type: Object})
- keyEventTarget: HTMLElement = document.body;
-
@property({type: Array})
changeTableColumns?: string[];
@@ -157,19 +154,19 @@
private readonly restApiService = appContext.restApiService;
- private readonly shortcuts = appContext.shortcutsService;
-
- override keyboardShortcuts() {
- return {
- [Shortcut.CURSOR_NEXT_CHANGE]: '_nextChange',
- [Shortcut.CURSOR_PREV_CHANGE]: '_prevChange',
- [Shortcut.NEXT_PAGE]: '_nextPage',
- [Shortcut.PREV_PAGE]: '_prevPage',
- [Shortcut.OPEN_CHANGE]: '_openChange',
- [Shortcut.TOGGLE_CHANGE_REVIEWED]: '_toggleChangeReviewed',
- [Shortcut.TOGGLE_CHANGE_STAR]: '_toggleChangeStar',
- [Shortcut.REFRESH_CHANGE_LIST]: '_refreshChangeList',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.CURSOR_NEXT_CHANGE, _ => this._nextChange()),
+ listen(Shortcut.CURSOR_PREV_CHANGE, _ => this._prevChange()),
+ listen(Shortcut.NEXT_PAGE, _ => this._nextPage()),
+ listen(Shortcut.PREV_PAGE, _ => this._prevPage()),
+ listen(Shortcut.OPEN_CHANGE, _ => this.openChange()),
+ listen(Shortcut.TOGGLE_CHANGE_REVIEWED, _ =>
+ this._toggleChangeReviewed()
+ ),
+ listen(Shortcut.TOGGLE_CHANGE_STAR, _ => this._toggleChangeStar()),
+ listen(Shortcut.REFRESH_CHANGE_LIST, _ => this._refreshChangeList()),
+ ];
}
private cursor = new GrCursorManager();
@@ -204,7 +201,7 @@
}
/**
- * Iron-a11y-keys-behavior catches keyboard events globally. Some keyboard
+ * shortcut-service catches keyboard events globally. Some keyboard
* events must be scoped to a component level (e.g. `enter`) in order to not
* override native browser functionality.
*
@@ -213,7 +210,7 @@
_scopedKeydownHandler(e: KeyboardEvent) {
if (e.keyCode === 13) {
// Enter.
- this.openChange(e);
+ this.openChange();
}
}
@@ -406,63 +403,30 @@
);
}
- _nextChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _nextChange() {
this.isCursorMoving = true;
this.cursor.next();
this.isCursorMoving = false;
this.selectedIndex = this.cursor.index;
}
- _prevChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _prevChange() {
this.isCursorMoving = true;
this.cursor.previous();
this.isCursorMoving = false;
this.selectedIndex = this.cursor.index;
}
- _openChange(e: IronKeyboardEvent) {
- if (this.shortcuts.modifierPressed(e)) return;
- this.openChange(e.detail.keyboardEvent);
- }
-
- openChange(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || modifierPressed(e)) return;
- e.preventDefault();
+ openChange() {
const change = this._changeForIndex(this.selectedIndex);
if (change) GerritNav.navigateToChange(change);
}
- _nextPage(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e))
- ) {
- return;
- }
-
- e.preventDefault();
+ _nextPage() {
fireEvent(this, 'next-page');
}
- _prevPage(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e))
- ) {
- return;
- }
-
- e.preventDefault();
+ _prevPage() {
this.dispatchEvent(
new CustomEvent('previous-page', {
composed: true,
@@ -471,12 +435,7 @@
);
}
- _toggleChangeReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _toggleChangeReviewed() {
this._toggleReviewedForIndex(this.selectedIndex);
}
@@ -490,21 +449,11 @@
changeEl.toggleReviewed();
}
- _refreshChangeList(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
-
- e.preventDefault();
+ _refreshChangeList() {
fireReload(this);
}
- _toggleChangeStar(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _toggleChangeStar() {
this._toggleStarForIndex(this.selectedIndex);
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js
index 4956380..de1a0a2 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js
@@ -147,38 +147,37 @@
await flush();
const promise = mockPromise();
afterNextRender(element, () => {
- const elementItems = element.root.querySelectorAll(
- 'gr-change-list-item');
- assert.equal(elementItems.length, 3);
-
- assert.isTrue(elementItems[0].hasAttribute('selected'));
- MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
- assert.equal(element.selectedIndex, 1);
- assert.isTrue(elementItems[1].hasAttribute('selected'));
- MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
- assert.equal(element.selectedIndex, 2);
- assert.isTrue(elementItems[2].hasAttribute('selected'));
-
- const navStub = sinon.stub(GerritNav, 'navigateToChange');
- assert.equal(element.selectedIndex, 2);
- MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
- assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
- 'Should navigate to /c/2/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- assert.equal(element.selectedIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
- assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
- 'Should navigate to /c/1/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- assert.equal(element.selectedIndex, 0);
-
promise.resolve();
});
await promise;
+ const elementItems = element.root.querySelectorAll(
+ 'gr-change-list-item');
+ assert.equal(elementItems.length, 3);
+
+ assert.isTrue(elementItems[0].hasAttribute('selected'));
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 1);
+ assert.isTrue(elementItems[1].hasAttribute('selected'));
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 2);
+ assert.isTrue(elementItems[2].hasAttribute('selected'));
+
+ const navStub = sinon.stub(GerritNav, 'navigateToChange');
+ assert.equal(element.selectedIndex, 2);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
+ 'Should navigate to /c/2/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ assert.equal(element.selectedIndex, 1);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
+ 'Should navigate to /c/1/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ assert.equal(element.selectedIndex, 0);
});
test('no changes', () => {
@@ -449,45 +448,45 @@
await flush();
const promise = mockPromise();
afterNextRender(element, () => {
- const elementItems = element.root.querySelectorAll(
- 'gr-change-list-item');
- assert.equal(elementItems.length, 9);
-
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- assert.equal(element.selectedIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
-
- const navStub = sinon.stub(GerritNav, 'navigateToChange');
- assert.equal(element.selectedIndex, 2);
-
- MockInteractions.pressAndReleaseKeyOn(element, 13); // 'enter'
- assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
- 'Should navigate to /c/2/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 75); // 'k'
- assert.equal(element.selectedIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 13); // 'enter'
- assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
- 'Should navigate to /c/1/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- assert.equal(element.selectedIndex, 4);
- MockInteractions.pressAndReleaseKeyOn(element, 13); // 'enter'
- assert.deepEqual(navStub.lastCall.args[0], {_number: 4},
- 'Should navigate to /c/4/');
-
- MockInteractions.keyUpOn(element, 82); // 'r'
- const change = element._changeForIndex(element.selectedIndex);
- assert.equal(change.reviewed, true,
- 'Should mark change as reviewed');
- MockInteractions.keyUpOn(element, 82); // 'r'
- assert.equal(change.reviewed, false,
- 'Should mark change as unreviewed');
promise.resolve();
});
await promise;
+ const elementItems = element.root.querySelectorAll(
+ 'gr-change-list-item');
+ assert.equal(elementItems.length, 9);
+
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 1);
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+
+ const navStub = sinon.stub(GerritNav, 'navigateToChange');
+ assert.equal(element.selectedIndex, 2);
+
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'Enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
+ 'Should navigate to /c/2/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ assert.equal(element.selectedIndex, 1);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'Enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
+ 'Should navigate to /c/1/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 4);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'Enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 4},
+ 'Should navigate to /c/4/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
+ const change = element._changeForIndex(element.selectedIndex);
+ assert.equal(change.reviewed, true,
+ 'Should mark change as reviewed');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
+ assert.equal(change.reviewed, false,
+ 'Should mark change as unreviewed');
});
test('_computeItemHighlight gives false for null account', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index c19be58e..b635d3f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -48,11 +48,12 @@
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
ShortcutSection,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {GrEditConstants} from '../../edit/gr-edit-constants';
import {pluralize} from '../../../utils/string-util';
-import {windowLocationReload, querySelectorAll} from '../../../utils/dom-util';
+import {querySelectorAll, windowLocationReload} from '../../../utils/dom-util';
import {
GeneratedWebLink,
GerritNav,
@@ -62,8 +63,8 @@
import {RevisionInfo as RevisionInfoClass} from '../../shared/revision-info/revision-info';
import {DiffViewMode} from '../../../api/diff';
import {
- DefaultBase,
ChangeStatus,
+ DefaultBase,
PrimaryTab,
SecondaryTab,
} from '../../../constants/constants';
@@ -83,9 +84,9 @@
changeIsOpen,
changeStatuses,
isCc,
+ isInvolved,
isOwner,
isReviewer,
- isInvolved,
} from '../../../utils/change-util';
import {EventType as PluginEventType} from '../../../api/plugin';
import {customElement, observe, property} from '@polymer/decorators';
@@ -158,9 +159,7 @@
ParsedChangeInfo,
} from '../../../types/types';
import {
- IronKeyboardEventListener,
CloseFixPreviewEvent,
- IronKeyboardEvent,
EditableContentSaveEvent,
EventType,
OpenFixPreviewEvent,
@@ -192,10 +191,11 @@
drafts$,
} from '../../../services/comments/comments-model';
import {
- hasAttention,
getAddedByReason,
getRemovedByReason,
+ hasAttention,
} from '../../../utils/attention-set-util';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
const MIN_LINES_FOR_COMMIT_COLLAPSE = 18;
@@ -295,9 +295,6 @@
@property({type: Boolean})
hasParent?: boolean;
- @property({type: Object})
- keyEventTarget = document.body;
-
@property({type: Boolean})
disableEdit = false;
@@ -529,7 +526,7 @@
@property({type: Boolean})
_showRobotCommentsButton = false;
- _throttledToggleChangeStar?: IronKeyboardEventListener;
+ _throttledToggleChangeStar?: (e: KeyboardEvent) => void;
@property({type: Boolean})
_showChecksTab = false;
@@ -560,28 +557,50 @@
private replyDialogResizeObserver?: ResizeObserver;
- override keyboardShortcuts() {
- return {
- [Shortcut.SEND_REPLY]: null, // DOC_ONLY binding
- [Shortcut.EMOJI_DROPDOWN]: null, // DOC_ONLY binding
- [Shortcut.REFRESH_CHANGE]: '_handleRefreshChange',
- [Shortcut.OPEN_REPLY_DIALOG]: '_handleOpenReplyDialog',
- [Shortcut.OPEN_DOWNLOAD_DIALOG]: '_handleOpenDownloadDialogShortcut',
- [Shortcut.TOGGLE_DIFF_MODE]: '_handleToggleDiffMode',
- [Shortcut.TOGGLE_CHANGE_STAR]: '_throttledToggleChangeStar',
- [Shortcut.UP_TO_DASHBOARD]: '_handleUpToDashboard',
- [Shortcut.EXPAND_ALL_MESSAGES]: '_handleExpandAllMessages',
- [Shortcut.COLLAPSE_ALL_MESSAGES]: '_handleCollapseAllMessages',
- [Shortcut.OPEN_DIFF_PREFS]: '_handleOpenDiffPrefsShortcut',
- [Shortcut.EDIT_TOPIC]: '_handleEditTopic',
- [Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase',
- [Shortcut.DIFF_AGAINST_LATEST]: '_handleDiffAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LEFT]: '_handleDiffBaseAgainstLeft',
- [Shortcut.DIFF_RIGHT_AGAINST_LATEST]: '_handleDiffRightAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LATEST]: '_handleDiffBaseAgainstLatest',
- [Shortcut.OPEN_SUBMIT_DIALOG]: '_handleOpenSubmitDialog',
- [Shortcut.TOGGLE_ATTENTION_SET]: '_handleToggleAttentionSet',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.SEND_REPLY, _ => {}), // docOnly
+ listen(Shortcut.EMOJI_DROPDOWN, _ => {}), // docOnly
+ listen(Shortcut.REFRESH_CHANGE, _ => fireReload(this, true)),
+ listen(Shortcut.OPEN_REPLY_DIALOG, _ => this._handleOpenReplyDialog()),
+ listen(Shortcut.OPEN_DOWNLOAD_DIALOG, _ =>
+ this._handleOpenDownloadDialog()
+ ),
+ listen(Shortcut.TOGGLE_DIFF_MODE, _ => this._handleToggleDiffMode()),
+ listen(Shortcut.TOGGLE_CHANGE_STAR, e => {
+ if (this._throttledToggleChangeStar) {
+ this._throttledToggleChangeStar(e);
+ }
+ }),
+ listen(Shortcut.UP_TO_DASHBOARD, _ => this._determinePageBack()),
+ listen(Shortcut.EXPAND_ALL_MESSAGES, _ =>
+ this._handleExpandAllMessages()
+ ),
+ listen(Shortcut.COLLAPSE_ALL_MESSAGES, _ =>
+ this._handleCollapseAllMessages()
+ ),
+ listen(Shortcut.OPEN_DIFF_PREFS, _ =>
+ this._handleOpenDiffPrefsShortcut()
+ ),
+ listen(Shortcut.EDIT_TOPIC, _ => this.$.metadata.editTopic()),
+ listen(Shortcut.DIFF_AGAINST_BASE, _ => this._handleDiffAgainstBase()),
+ listen(Shortcut.DIFF_AGAINST_LATEST, _ =>
+ this._handleDiffAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LEFT, _ =>
+ this._handleDiffBaseAgainstLeft()
+ ),
+ listen(Shortcut.DIFF_RIGHT_AGAINST_LATEST, _ =>
+ this._handleDiffRightAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LATEST, _ =>
+ this._handleDiffBaseAgainstLatest()
+ ),
+ listen(Shortcut.OPEN_SUBMIT_DIALOG, _ => this._handleOpenSubmitDialog()),
+ listen(Shortcut.TOGGLE_ATTENTION_SET, _ =>
+ this._handleToggleAttentionSet()
+ ),
+ ];
}
disconnected$ = new Subject();
@@ -632,8 +651,8 @@
override connectedCallback() {
super.connectedCallback();
- this._throttledToggleChangeStar = throttleWrap<IronKeyboardEvent>(e =>
- this._handleToggleChangeStar(e)
+ this._throttledToggleChangeStar = throttleWrap<KeyboardEvent>(_ =>
+ this._handleToggleChangeStar()
);
this._getServerConfig().then(config => {
this._serverConfig = config;
@@ -742,12 +761,7 @@
if (e.detail.fixApplied) fireReload(this);
}
- _handleToggleDiffMode(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleToggleDiffMode() {
if (this.viewState.diffMode === DiffViewMode.SIDE_BY_SIDE) {
this.$.fileListHeader.setDiffViewMode(DiffViewMode.UNIFIED);
} else {
@@ -1486,52 +1500,22 @@
return label;
}
- _handleOpenReplyDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
+ _handleOpenReplyDialog() {
this._getLoggedIn().then(isLoggedIn => {
if (!isLoggedIn) {
fireEvent(this, 'show-auth-required');
return;
}
-
- e.preventDefault();
this._openReplyDialog(this.$.replyDialog.FocusTarget.ANY);
});
}
- _handleOpenDownloadDialogShortcut(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- this._handleOpenDownloadDialog();
- }
-
- _handleEditTopic(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- this.$.metadata.editTopic();
- }
-
- _handleOpenSubmitDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || !this._submitEnabled) {
- return;
- }
-
- e.preventDefault();
+ _handleOpenSubmitDialog() {
+ if (!this._submitEnabled) return;
this.$.actions.showSubmitDialog();
}
- _handleToggleAttentionSet(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleToggleAttentionSet() {
if (!this._change || !this._account?._account_id) return;
if (!this._loggedIn || !isInvolved(this._change, this._account)) return;
if (!this._change.attention_set) this._change.attention_set = {};
@@ -1570,10 +1554,7 @@
this._change = {...this._change};
}
- _handleDiffAgainstBase(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffAgainstBase() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1584,10 +1565,7 @@
GerritNav.navigateToChange(this._change, this._patchRange.patchNum);
}
- _handleDiffBaseAgainstLeft(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffBaseAgainstLeft() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1598,10 +1576,7 @@
GerritNav.navigateToChange(this._change, this._patchRange.basePatchNum);
}
- _handleDiffAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffAgainstLatest() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1617,10 +1592,7 @@
);
}
- _handleDiffRightAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffRightAgainstLatest() {
assertIsDefined(this._change, '_change');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (!this._patchRange)
@@ -1636,10 +1608,7 @@
);
}
- _handleDiffBaseAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffBaseAgainstLatest() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1654,59 +1623,24 @@
GerritNav.navigateToChange(this._change, latestPatchNum);
}
- _handleRefreshChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
- e.preventDefault();
- fireReload(this, true);
- }
-
- _handleToggleChangeStar(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
- e.preventDefault();
+ _handleToggleChangeStar() {
this.$.changeStar.toggleStar();
}
- _handleUpToDashboard(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- this._determinePageBack();
- }
-
- _handleExpandAllMessages(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleExpandAllMessages() {
if (this.messagesList) {
this.messagesList.handleExpandCollapse(true);
}
}
- _handleCollapseAllMessages(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleCollapseAllMessages() {
if (this.messagesList) {
this.messagesList.handleExpandCollapse(false);
}
}
- _handleOpenDiffPrefsShortcut(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
+ _handleOpenDiffPrefsShortcut() {
if (!this._loggedIn) return;
- e.preventDefault();
this.$.fileList.openDiffPrefs();
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index e508c63..6cbe59c 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -82,17 +82,12 @@
} from '../../../types/common';
import {
pressAndReleaseKeyOn,
- keyUpOn,
tap,
} from '@polymer/iron-test-helpers/mock-interactions';
import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls';
import {AppElementChangeViewParams} from '../../gr-app-types';
import {SinonFakeTimers, SinonStubbedMember} from 'sinon';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
-import {
- IronKeyboardEvent,
- IronKeyboardEventDetail,
-} from '../../../types/events';
import {CommentThread, UIRobot} from '../../../utils/comment-util';
import {GerritView} from '../../../services/router/router-model';
import {ParsedChangeInfo} from '../../../types/types';
@@ -403,7 +398,7 @@
patchNum: 3 as RevisionPatchSetNum,
basePatchNum: 1 as BasePatchSetNum,
};
- element._handleDiffAgainstBase(new CustomEvent('') as IronKeyboardEvent);
+ element._handleDiffAgainstBase();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[0], element._change);
@@ -419,7 +414,7 @@
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
- element._handleDiffAgainstLatest(new CustomEvent('') as IronKeyboardEvent);
+ element._handleDiffAgainstLatest();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[0], element._change);
@@ -436,9 +431,7 @@
patchNum: 3 as RevisionPatchSetNum,
basePatchNum: 1 as BasePatchSetNum,
};
- element._handleDiffBaseAgainstLeft(
- new CustomEvent('') as IronKeyboardEvent
- );
+ element._handleDiffBaseAgainstLeft();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[0], element._change);
@@ -454,9 +447,7 @@
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
- element._handleDiffRightAgainstLatest(
- new CustomEvent('') as IronKeyboardEvent
- );
+ element._handleDiffRightAgainstLatest();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[1], 10 as PatchSetNum);
@@ -472,9 +463,7 @@
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
- element._handleDiffBaseAgainstLatest(
- new CustomEvent('') as IronKeyboardEvent
- );
+ element._handleDiffBaseAgainstLatest();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[1], 10 as PatchSetNum);
@@ -501,11 +490,11 @@
assert.isNotOk(element._change.attention_set);
await element._getLoggedIn();
await element.restApiService.getAccount();
- element._handleToggleAttentionSet(new CustomEvent('') as IronKeyboardEvent);
+ element._handleToggleAttentionSet();
assert.isTrue(addToAttentionSetStub.called);
assert.isFalse(removeFromAttentionSetStub.called);
- element._handleToggleAttentionSet(new CustomEvent('') as IronKeyboardEvent);
+ element._handleToggleAttentionSet();
assert.isTrue(removeFromAttentionSetStub.called);
});
@@ -650,7 +639,7 @@
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(false));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- keyUpOn(element, 65, null, 'a');
+ pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isFalse(element.$.replyOverlay.opened);
assert.isTrue(loggedInErrorSpy.called);
@@ -683,7 +672,7 @@
const openSpy = sinon.spy(element, '_openReplyDialog');
- keyUpOn(element, 65, null, 'a');
+ pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(element.$.replyOverlay.opened);
element.$.replyOverlay.close();
@@ -796,7 +785,7 @@
const stub = sinon
.stub(element.$.downloadOverlay, 'open')
.returns(Promise.resolve());
- keyUpOn(element, 68, null, 'd');
+ pressAndReleaseKeyOn(element, 68, null, 'd');
assert.isTrue(stub.called);
});
@@ -819,17 +808,14 @@
element.$.fileListHeader,
'setDiffViewMode'
);
- const e = new CustomEvent<IronKeyboardEventDetail>('keydown', {
- detail: {keyboardEvent: new KeyboardEvent('keydown'), key: 'x'},
- });
flush();
element.viewState.diffMode = DiffViewMode.SIDE_BY_SIDE;
- element._handleToggleDiffMode(e);
+ element._handleToggleDiffMode();
assert.isTrue(setModeStub.calledWith(DiffViewMode.UNIFIED));
element.viewState.diffMode = DiffViewMode.UNIFIED;
- element._handleToggleDiffMode(e);
+ element._handleToggleDiffMode();
assert.isTrue(setModeStub.calledWith(DiffViewMode.SIDE_BY_SIDE));
});
});
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 4fd5ff3..cc76145 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
@@ -36,6 +36,7 @@
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {FilesExpandedState} from '../gr-file-list-constants';
import {pluralize} from '../../../utils/string-util';
@@ -50,10 +51,9 @@
} from '../../../constants/constants';
import {
addGlobalShortcut,
+ addShortcut,
descendedFromClass,
- isShiftPressed,
Key,
- modifierPressed,
toggleClass,
} from '../../../utils/dom-util';
import {
@@ -80,7 +80,6 @@
import {GrCursorManager} from '../../shared/gr-cursor-manager/gr-cursor-manager';
import {PolymerSpliceChange} from '@polymer/polymer/interfaces';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
-import {IronKeyboardEvent} from '../../../types/events';
import {ParsedChangeInfo, PatchSetFile} from '../../../types/types';
import {Timing} from '../../../constants/reporting';
import {RevisionInfo} from '../../shared/revision-info/revision-info';
@@ -88,6 +87,7 @@
import {changeComments$} from '../../../services/comments/comments-model';
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
export const DEFAULT_NUM_FILES_SHOWN = 200;
@@ -200,9 +200,6 @@
selectedIndex = -1;
@property({type: Object})
- keyEventTarget = document.body;
-
- @property({type: Object})
change?: ParsedChangeInfo;
@property({type: String, notify: true, observer: '_updateDiffPreferences'})
@@ -324,45 +321,53 @@
/** Called in disconnectedCallback. */
private cleanups: (() => void)[] = [];
- override keyboardShortcuts() {
- return {
- [Shortcut.LEFT_PANE]: '_handleLeftPane',
- [Shortcut.RIGHT_PANE]: '_handleRightPane',
- [Shortcut.TOGGLE_INLINE_DIFF]: '_handleToggleInlineDiff',
- [Shortcut.TOGGLE_ALL_INLINE_DIFFS]: '_handleToggleAllInlineDiffs',
- [Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS]:
- '_handleToggleHideAllCommentThreads',
- [Shortcut.CURSOR_NEXT_FILE]: '_handleCursorNext',
- [Shortcut.CURSOR_PREV_FILE]: '_handleCursorPrev',
- [Shortcut.NEXT_LINE]: '_handleCursorNext',
- [Shortcut.PREV_LINE]: '_handleCursorPrev',
- [Shortcut.NEW_COMMENT]: '_handleNewComment',
- [Shortcut.OPEN_LAST_FILE]: '_handleOpenLastFile',
- [Shortcut.OPEN_FIRST_FILE]: '_handleOpenFirstFile',
- [Shortcut.OPEN_FILE]: '_handleOpenFile',
- [Shortcut.NEXT_CHUNK]: '_handleNextChunk',
- [Shortcut.PREV_CHUNK]: '_handlePrevChunk',
- [Shortcut.TOGGLE_FILE_REVIEWED]: '_handleToggleFileReviewed',
- [Shortcut.TOGGLE_LEFT_PANE]: '_handleToggleLeftPane',
-
- // Final two are actually handled by gr-comment-thread.
- [Shortcut.EXPAND_ALL_COMMENT_THREADS]: null,
- [Shortcut.COLLAPSE_ALL_COMMENT_THREADS]: null,
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.LEFT_PANE, _ => this._handleLeftPane()),
+ listen(Shortcut.RIGHT_PANE, _ => this._handleRightPane()),
+ listen(Shortcut.TOGGLE_INLINE_DIFF, _ => this._handleToggleInlineDiff()),
+ listen(Shortcut.TOGGLE_ALL_INLINE_DIFFS, _ => this._toggleInlineDiffs()),
+ listen(Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS, _ =>
+ toggleClass(this, 'hideComments')
+ ),
+ listen(Shortcut.CURSOR_NEXT_FILE, e => this._handleCursorNext(e)),
+ listen(Shortcut.CURSOR_PREV_FILE, e => this._handleCursorPrev(e)),
+ // This is already been taken care of by CURSOR_NEXT_FILE above. The two
+ // shortcuts share the same bindings. It depends on whether all files
+ // are expanded whether the cursor moves to the next file or line.
+ listen(Shortcut.NEXT_LINE, _ => {}), // docOnly
+ // This is already been taken care of by CURSOR_PREV_FILE above. The two
+ // shortcuts share the same bindings. It depends on whether all files
+ // are expanded whether the cursor moves to the previous file or line.
+ listen(Shortcut.PREV_LINE, _ => {}), // docOnly
+ listen(Shortcut.NEW_COMMENT, _ => this._handleNewComment()),
+ listen(Shortcut.OPEN_LAST_FILE, _ =>
+ this._openSelectedFile(this._files.length - 1)
+ ),
+ listen(Shortcut.OPEN_FIRST_FILE, _ => this._openSelectedFile(0)),
+ listen(Shortcut.OPEN_FILE, _ => this.handleOpenFile()),
+ listen(Shortcut.NEXT_CHUNK, _ => this._handleNextChunk()),
+ listen(Shortcut.PREV_CHUNK, _ => this._handlePrevChunk()),
+ listen(Shortcut.NEXT_COMMENT_THREAD, _ => this._handleNextComment()),
+ listen(Shortcut.PREV_COMMENT_THREAD, _ => this._handlePrevComment()),
+ listen(Shortcut.TOGGLE_FILE_REVIEWED, _ =>
+ this._handleToggleFileReviewed()
+ ),
+ listen(Shortcut.TOGGLE_LEFT_PANE, _ => this._handleToggleLeftPane()),
+ listen(Shortcut.EXPAND_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ listen(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ ];
}
private fileCursor = new GrCursorManager();
private diffCursor = new GrDiffCursor();
- private readonly shortcuts = appContext.shortcutsService;
-
constructor() {
super();
this.fileCursor.scrollMode = ScrollMode.KEEP_VISIBLE;
this.fileCursor.cursorTargetClass = 'selected';
this.fileCursor.focusOnMove = true;
- this.addEventListener('keydown', e => this._scopedKeydownHandler(e));
}
override connectedCallback() {
@@ -415,7 +420,8 @@
}
});
this.cleanups.push(
- addGlobalShortcut({key: Key.ESC}, e => this._handleEscKey(e))
+ addGlobalShortcut({key: Key.ESC}, _ => this._handleEscKey()),
+ addShortcut(this, {key: Key.ENTER}, _ => this.handleOpenFile())
);
}
@@ -430,17 +436,6 @@
super.disconnectedCallback();
}
- /**
- * Iron-a11y-keys-behavior catches keyboard events globally. Some keyboard
- * events must be scoped to a component level (e.g. `enter`) in order to not
- * override native browser functionality.
- *
- * Context: Issue 7277
- */
- _scopedKeydownHandler(e: KeyboardEvent) {
- if (e.keyCode === 13) this.handleOpenFile(e);
- }
-
reload() {
if (!this.changeNum || !this.patchRange?.patchNum) {
return Promise.resolve();
@@ -885,196 +880,84 @@
return fileData;
}
- _handleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this._noDiffsExpanded()) {
- return;
- }
-
- e.preventDefault();
+ _handleLeftPane() {
+ if (this._noDiffsExpanded()) return;
this.diffCursor.moveLeft();
}
- _handleRightPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this._noDiffsExpanded()) {
- return;
- }
-
- e.preventDefault();
+ _handleRightPane() {
+ if (this._noDiffsExpanded()) return;
this.diffCursor.moveRight();
}
- _handleToggleInlineDiff(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- this.shortcuts.modifierPressed(e) ||
- e.detail?.keyboardEvent?.repeat ||
- this.fileCursor.index === -1
- ) {
- return;
- }
-
- e.preventDefault();
+ _handleToggleInlineDiff() {
+ if (this.fileCursor.index === -1) return;
this._toggleFileExpandedByIndex(this.fileCursor.index);
}
- _handleToggleAllInlineDiffs(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || e.detail?.keyboardEvent?.repeat) {
- return;
- }
-
- e.preventDefault();
- this._toggleInlineDiffs();
- }
-
- _handleToggleHideAllCommentThreads(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- toggleClass(this, 'hideComments');
- }
-
- _handleCursorNext(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
+ _handleCursorNext(e: KeyboardEvent) {
if (this.filesExpanded === FilesExpandedState.ALL) {
- e.preventDefault();
this.diffCursor.moveDown();
this._displayLine = true;
} else {
- // Down key
- if (e.detail.keyboardEvent.keyCode === 40) {
- return;
- }
- e.preventDefault();
+ if (e.key === Key.DOWN) return;
this.fileCursor.next({circular: true});
this.selectedIndex = this.fileCursor.index;
}
}
- _handleCursorPrev(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
+ _handleCursorPrev(e: KeyboardEvent) {
if (this.filesExpanded === FilesExpandedState.ALL) {
- e.preventDefault();
this.diffCursor.moveUp();
this._displayLine = true;
} else {
- // Up key
- if (e.detail.keyboardEvent.keyCode === 38) {
- return;
- }
- e.preventDefault();
+ if (e.key === Key.UP) return;
this.fileCursor.previous({circular: true});
this.selectedIndex = this.fileCursor.index;
}
}
- _handleNewComment(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
- e.preventDefault();
+ _handleNewComment() {
this.classList.remove('hideComments');
this.diffCursor.createCommentInPlace();
}
- _handleOpenLastFile(e: IronKeyboardEvent) {
- // Check for meta key to avoid overriding native chrome shortcut.
- if (this.shortcuts.shouldSuppress(e) || e.detail.keyboardEvent.metaKey) {
- return;
- }
-
- e.preventDefault();
- this._openSelectedFile(this._files.length - 1);
- }
-
- _handleOpenFirstFile(e: IronKeyboardEvent) {
- // Check for meta key to avoid overriding native chrome shortcut.
- if (this.shortcuts.shouldSuppress(e) || e.detail.keyboardEvent.metaKey) {
- return;
- }
-
- e.preventDefault();
- this._openSelectedFile(0);
- }
-
- _handleOpenFile(e: IronKeyboardEvent) {
- if (this.shortcuts.modifierPressed(e)) return;
- this.handleOpenFile(e.detail.keyboardEvent);
- }
-
- handleOpenFile(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || modifierPressed(e)) {
- return;
- }
- e.preventDefault();
-
+ handleOpenFile() {
if (this.filesExpanded === FilesExpandedState.ALL) {
this._openCursorFile();
return;
}
-
this._openSelectedFile();
}
- _handleNextChunk(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e)) ||
- this._noDiffsExpanded()
- ) {
- return;
- }
-
- e.preventDefault();
- if (isShiftPressed(e)) {
- this.diffCursor.moveToNextCommentThread();
- } else {
- this.diffCursor.moveToNextChunk();
- }
+ _handleNextChunk() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToNextChunk();
}
- _handlePrevChunk(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e)) ||
- this._noDiffsExpanded()
- ) {
- return;
- }
-
- e.preventDefault();
- if (isShiftPressed(e)) {
- this.diffCursor.moveToPreviousCommentThread();
- } else {
- this.diffCursor.moveToPreviousChunk();
- }
+ _handleNextComment() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToNextCommentThread();
}
- _handleToggleFileReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
+ _handlePrevChunk() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToPreviousChunk();
+ }
- e.preventDefault();
+ _handlePrevComment() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToPreviousCommentThread();
+ }
+
+ _handleToggleFileReviewed() {
if (!this._files[this.fileCursor.index]) {
return;
}
this._reviewFile(this._files[this.fileCursor.index].__path);
}
- _handleToggleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleToggleLeftPane() {
this._forEachDiff(diff => {
diff.toggleLeftDiff();
});
@@ -1546,9 +1429,7 @@
return undefined;
}
- _handleEscKey(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- e.preventDefault();
+ _handleEscKey() {
this._displayLine = false;
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
index 062d6a2..7409be7 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -27,11 +27,11 @@
import {runA11yAudit} from '../../../test/a11y-test-utils.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
import {
- stubRestApi,
- spyRestApi,
listenOnce,
mockPromise,
query,
+ spyRestApi,
+ stubRestApi,
} from '../../../test/test-utils.js';
import {EditPatchSetNum} from '../../../types/common.js';
import {createCommentThreads} from '../../../utils/comment-util.js';
@@ -470,7 +470,7 @@
// https://github.com/sinonjs/sinon/issues/781
const diffsStub = sinon.stub(element, 'diffs')
.get(() => [{toggleLeftDiff: toggleLeftDiffStub}]);
- MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift', 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'A');
assert.isTrue(toggleLeftDiffStub.calledOnce);
diffsStub.restore();
});
@@ -486,7 +486,7 @@
assert.isFalse(items[1].classList.contains('selected'));
assert.isFalse(items[2].classList.contains('selected'));
// j with a modifier should not move the cursor.
- MockInteractions.pressAndReleaseKeyOn(element, 74, 'shift', 'j');
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'J');
assert.equal(element.fileCursor.index, 0);
// down should not move the cursor.
MockInteractions.pressAndReleaseKeyOn(element, 40, null, 'down');
@@ -502,7 +502,7 @@
assert.equal(element.selectedIndex, 2);
// k with a modifier should not move the cursor.
- MockInteractions.pressAndReleaseKeyOn(element, 75, 'shift', 'k');
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'K');
assert.equal(element.fileCursor.index, 2);
// up should not move the cursor.
@@ -560,7 +560,7 @@
assert.equal(element._expandedFiles.length, 1);
assert.equal(element._expandedFiles[0].path, paths[1]);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
+ MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'I');
flush();
assert.equal(element.diffs.length, paths.length);
assert.equal(element._expandedFiles.length, paths.length);
@@ -569,7 +569,7 @@
}
// since _expandedFilesChanged is stubbed
element.filesExpanded = FilesExpandedState.ALL;
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
+ MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'I');
flush();
assert.equal(element.diffs.length, 0);
assert.equal(element._expandedFiles.length, 0);
@@ -584,16 +584,16 @@
assert.equal(getNumReviewed(), 0);
// Press the review key to toggle it (set the flag).
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
flush();
assert.equal(getNumReviewed(), 1);
// Press the review key to toggle it (clear the flag).
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.equal(getNumReviewed(), 0);
});
- suite('_handleOpenFile', () => {
+ suite('handleOpenFile', () => {
let interact;
setup(() => {
@@ -605,14 +605,7 @@
openCursorStub.reset();
openSelectedStub.reset();
expandStub.reset();
-
- const keyboardEvent = new KeyboardEvent('keydown');
- const e = new CustomEvent('keydown', {
- detail: {keyboardEvent, key: 'x'},
- });
- sinon.stub(keyboardEvent, 'preventDefault');
- element._handleOpenFile(e);
- assert.isTrue(keyboardEvent.preventDefault.called);
+ element.handleOpenFile();
const result = {};
if (openCursorStub.called) {
result.opened_cursor = true;
@@ -653,16 +646,20 @@
sinon.stub(element, '_noDiffsExpanded')
.callsFake(() => noDiffsExpanded);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'left');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowLeft');
assert.isFalse(moveLeftStub.called);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'right');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowRight');
assert.isFalse(moveRightStub.called);
noDiffsExpanded = false;
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'left');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowLeft');
assert.isTrue(moveLeftStub.called);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'right');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowRight');
assert.isTrue(moveRightStub.called);
});
});
@@ -1590,7 +1587,7 @@
});
test('cursor with toggle all files', async () => {
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
+ MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'I');
await flush();
const diffs = await renderAndGetNewDiffs(0);
@@ -1620,14 +1617,12 @@
});
suite('n key presses', () => {
- let nKeySpy;
let nextCommentStub;
let nextChunkStub;
let fileRows;
setup(() => {
sinon.stub(element, '_renderInOrder').returns(Promise.resolve());
- nKeySpy = sinon.spy(element, '_handleNextChunk');
nextCommentStub = sinon.stub(element.diffCursor,
'moveToNextCommentThread');
nextChunkStub = sinon.stub(element.diffCursor,
@@ -1636,58 +1631,40 @@
element.root.querySelectorAll('.row:not(.header-row)');
});
- test('n key with some files expanded and no shift key', async () => {
+ test('n key with some files expanded', async () => {
MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'i');
await flush();
+ assert.equal(element.filesExpanded, FilesExpandedState.SOME);
- // Handle N key should return before calling diff cursor functions.
MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'n');
- assert.isTrue(nKeySpy.called);
- assert.isFalse(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 1);
- assert.equal(element.filesExpanded, 'some');
+ assert.isTrue(nextChunkStub.calledOnce);
});
- test('n key with some files expanded and shift key', async () => {
+ test('N key with some files expanded', async () => {
MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'i');
await flush();
- assert.equal(nextChunkStub.callCount, 0);
+ assert.equal(element.filesExpanded, FilesExpandedState.SOME);
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
- assert.isTrue(nKeySpy.called);
- assert.isTrue(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 0);
- assert.equal(element.filesExpanded, 'some');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
+ assert.isTrue(nextCommentStub.calledOnce);
});
- test('n key without all files expanded and shift key', async () => {
- MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, 'shift', 'i');
+ test('n key with all files expanded', async () => {
+ MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'I');
await flush();
+ assert.equal(element.filesExpanded, FilesExpandedState.ALL);
MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'n');
- assert.isTrue(nKeySpy.called);
- assert.isFalse(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 1);
- assert.equal(element.filesExpanded, FilesExpandedState.ALL);
+ assert.isTrue(nextChunkStub.calledOnce);
});
- test('n key without all files expanded and no shift key', async () => {
- MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, 'shift', 'i');
+ test('N key with all files expanded', async () => {
+ MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'I');
await flush();
-
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
- assert.isTrue(nKeySpy.called);
- assert.isTrue(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 0);
assert.equal(element.filesExpanded, FilesExpandedState.ALL);
+
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
+ assert.isTrue(nextCommentStub.called);
});
});
@@ -1708,26 +1685,17 @@
test('_displayLine', () => {
element.filesExpanded = FilesExpandedState.ALL;
- const mockEvent = {
- preventDefault() {},
- composedPath() { return []; },
- detail: {
- keyboardEvent: {
- composedPath() { return []; },
- },
- },
- };
element._displayLine = false;
- element._handleCursorNext(mockEvent);
+ element._handleCursorNext(new KeyboardEvent('keydown'));
assert.isTrue(element._displayLine);
element._displayLine = false;
- element._handleCursorPrev(mockEvent);
+ element._handleCursorPrev(new KeyboardEvent('keydown'));
assert.isTrue(element._displayLine);
element._displayLine = true;
- element._handleEscKey(mockEvent);
+ element._handleEscKey();
assert.isFalse(element._displayLine);
});
@@ -1737,13 +1705,13 @@
const saveReviewStub = sinon.stub(element, '_saveReviewedState');
element.editMode = false;
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(saveReviewStub.calledOnce);
element.editMode = true;
await flush();
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(saveReviewStub.calledOnce);
});
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts
index c91ae5a..8610999 100644
--- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts
+++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts
@@ -22,11 +22,11 @@
import {htmlTemplate} from './gr-keyboard-shortcuts-dialog_html';
import {
ShortcutSection,
- ShortcutListener,
SectionView,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {property, customElement} from '@polymer/decorators';
import {appContext} from '../../../services/app-context';
+import {ShortcutViewListener} from '../../../services/shortcuts/shortcuts-service';
declare global {
interface HTMLElementTagNameMap {
@@ -57,7 +57,7 @@
@property({type: Array})
_right?: SectionShortcut[];
- private readonly shortcutListener: ShortcutListener;
+ private readonly shortcutListener: ShortcutViewListener;
private readonly shortcuts = appContext.shortcutsService;
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
index dd1ebd1..2901b8a 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
@@ -22,6 +22,7 @@
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {customElement, property} from '@polymer/decorators';
import {ServerInfo} from '../../../types/common';
@@ -31,9 +32,9 @@
GrAutocomplete,
} from '../../shared/gr-autocomplete/gr-autocomplete';
import {getDocsBaseUrl} from '../../../utils/url-util';
-import {IronKeyboardEvent} from '../../../types/events';
import {MergeabilityComputationBehavior} from '../../../constants/constants';
import {appContext} from '../../../services/app-context';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
// Possible static search options for auto complete, without negations.
const SEARCH_OPERATORS: ReadonlyArray<string> = [
@@ -169,9 +170,6 @@
value = '';
@property({type: Object})
- keyEventTarget: unknown = document.body;
-
- @property({type: Object})
query: AutocompleteQuery;
@property({type: Object})
@@ -197,8 +195,6 @@
private readonly restApiService = appContext.restApiService;
- private readonly shortcuts = appContext.shortcutsService;
-
constructor() {
super();
this.query = (input: string) => this._getSearchSuggestions(input);
@@ -245,10 +241,8 @@
}
}
- override keyboardShortcuts() {
- return {
- [Shortcut.SEARCH]: '_handleSearch',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [listen(Shortcut.SEARCH, _ => this._handleSearch())];
}
_valueChanged(value: string) {
@@ -396,16 +390,7 @@
});
}
- _handleSearch(e: IronKeyboardEvent) {
- const keyboardEvent = e.detail.keyboardEvent;
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !keyboardEvent.shiftKey)
- ) {
- return;
- }
-
- e.preventDefault();
+ _handleSearch() {
this.$.searchInput.focus();
this.$.searchInput.selectAll();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 236f00f..0813900 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -37,6 +37,7 @@
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
ShortcutSection,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {
@@ -94,16 +95,11 @@
import {RevisionInfo as RevisionInfoObj} from '../../shared/revision-info/revision-info';
import {
CommentMap,
- isInBaseOfPatchRange,
getPatchRangeForCommentUrl,
+ isInBaseOfPatchRange,
} from '../../../utils/comment-util';
import {AppElementParams} from '../../gr-app-types';
-import {
- IronKeyboardEventListener,
- IronKeyboardEvent,
- EventType,
- OpenFixPreviewEvent,
-} from '../../../types/events';
+import {EventType, OpenFixPreviewEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {GerritView} from '../../../services/router/router-model';
import {assertIsDefined} from '../../../utils/common-util';
@@ -114,6 +110,7 @@
import {takeUntil} from 'rxjs/operators';
import {Subject} from 'rxjs';
import {preferences$} from '../../../services/user/user-model';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
const LOADING_BLAME = 'Loading blame...';
@@ -168,9 +165,6 @@
@property({type: Object, observer: '_paramsChanged'})
params?: AppElementParams;
- @property({type: Object})
- keyEventTarget: HTMLElement = document.body;
-
@property({type: Object, notify: true, observer: '_changeViewStateChanged'})
changeViewState: Partial<ChangeViewState> = {};
@@ -284,46 +278,71 @@
/** Called in disconnectedCallback. */
private cleanups: (() => void)[] = [];
- override keyboardShortcuts() {
- return {
- [Shortcut.LEFT_PANE]: '_handleLeftPane',
- [Shortcut.RIGHT_PANE]: '_handleRightPane',
- [Shortcut.NEXT_LINE]: '_handleNextLineOrFileWithComments',
- [Shortcut.PREV_LINE]: '_handlePrevLineOrFileWithComments',
- [Shortcut.VISIBLE_LINE]: '_handleVisibleLine',
- [Shortcut.NEXT_FILE_WITH_COMMENTS]: '_handleNextLineOrFileWithComments',
- [Shortcut.PREV_FILE_WITH_COMMENTS]: '_handlePrevLineOrFileWithComments',
- [Shortcut.NEW_COMMENT]: '_handleNewComment',
- [Shortcut.SAVE_COMMENT]: null, // DOC_ONLY binding
- [Shortcut.NEXT_FILE]: '_handleNextFile',
- [Shortcut.PREV_FILE]: '_handlePrevFile',
- [Shortcut.NEXT_CHUNK]: '_handleNextChunkOrCommentThread',
- [Shortcut.NEXT_COMMENT_THREAD]: '_handleNextChunkOrCommentThread',
- [Shortcut.PREV_CHUNK]: '_handlePrevChunkOrCommentThread',
- [Shortcut.PREV_COMMENT_THREAD]: '_handlePrevChunkOrCommentThread',
- [Shortcut.OPEN_REPLY_DIALOG]: '_handleOpenReplyDialog',
- [Shortcut.TOGGLE_LEFT_PANE]: '_handleToggleLeftPane',
- [Shortcut.OPEN_DOWNLOAD_DIALOG]: '_handleOpenDownloadDialog',
- [Shortcut.UP_TO_CHANGE]: '_handleUpToChange',
- [Shortcut.OPEN_DIFF_PREFS]: '_handleCommaKey',
- [Shortcut.TOGGLE_DIFF_MODE]: '_handleToggleDiffMode',
- [Shortcut.TOGGLE_FILE_REVIEWED]: '_throttledToggleFileReviewed',
- [Shortcut.TOGGLE_ALL_DIFF_CONTEXT]: '_handleToggleAllDiffContext',
- [Shortcut.NEXT_UNREVIEWED_FILE]: '_handleNextUnreviewedFile',
- [Shortcut.TOGGLE_BLAME]: '_handleToggleBlame',
- [Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS]:
- '_handleToggleHideAllCommentThreads',
- [Shortcut.OPEN_FILE_LIST]: '_handleOpenFileList',
- [Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase',
- [Shortcut.DIFF_AGAINST_LATEST]: '_handleDiffAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LEFT]: '_handleDiffBaseAgainstLeft',
- [Shortcut.DIFF_RIGHT_AGAINST_LATEST]: '_handleDiffRightAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LATEST]: '_handleDiffBaseAgainstLatest',
-
- // Final two are actually handled by gr-comment-thread.
- [Shortcut.EXPAND_ALL_COMMENT_THREADS]: null,
- [Shortcut.COLLAPSE_ALL_COMMENT_THREADS]: null,
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.LEFT_PANE, _ => this.cursor.moveLeft()),
+ listen(Shortcut.RIGHT_PANE, _ => this.cursor.moveRight()),
+ listen(Shortcut.NEXT_LINE, _ => this._handleNextLine()),
+ listen(Shortcut.PREV_LINE, _ => this._handlePrevLine()),
+ listen(Shortcut.VISIBLE_LINE, _ => this.cursor.moveToVisibleArea()),
+ listen(Shortcut.NEXT_FILE_WITH_COMMENTS, _ =>
+ this._moveToNextFileWithComment()
+ ),
+ listen(Shortcut.PREV_FILE_WITH_COMMENTS, _ =>
+ this._moveToPreviousFileWithComment()
+ ),
+ listen(Shortcut.NEW_COMMENT, _ => this._handleNewComment()),
+ listen(Shortcut.SAVE_COMMENT, _ => {}),
+ listen(Shortcut.NEXT_FILE, _ => this._handleNextFile()),
+ listen(Shortcut.PREV_FILE, _ => this._handlePrevFile()),
+ listen(Shortcut.NEXT_CHUNK, _ => this._handleNextChunk()),
+ listen(Shortcut.PREV_CHUNK, _ => this._handlePrevChunk()),
+ listen(Shortcut.NEXT_COMMENT_THREAD, _ =>
+ this._handleNextCommentThread()
+ ),
+ listen(Shortcut.PREV_COMMENT_THREAD, _ =>
+ this._handlePrevCommentThread()
+ ),
+ listen(Shortcut.OPEN_REPLY_DIALOG, _ => this._handleOpenReplyDialog()),
+ listen(Shortcut.TOGGLE_LEFT_PANE, _ => this._handleToggleLeftPane()),
+ listen(Shortcut.OPEN_DOWNLOAD_DIALOG, _ =>
+ this._handleOpenDownloadDialog()
+ ),
+ listen(Shortcut.UP_TO_CHANGE, _ => this._handleUpToChange()),
+ listen(Shortcut.OPEN_DIFF_PREFS, _ => this._handleCommaKey()),
+ listen(Shortcut.TOGGLE_DIFF_MODE, _ => this._handleToggleDiffMode()),
+ listen(Shortcut.TOGGLE_FILE_REVIEWED, e => {
+ if (this._throttledToggleFileReviewed) {
+ this._throttledToggleFileReviewed(e);
+ }
+ }),
+ listen(Shortcut.TOGGLE_ALL_DIFF_CONTEXT, _ =>
+ this._handleToggleAllDiffContext()
+ ),
+ listen(Shortcut.NEXT_UNREVIEWED_FILE, _ =>
+ this._handleNextUnreviewedFile()
+ ),
+ listen(Shortcut.TOGGLE_BLAME, _ => this._handleToggleBlame()),
+ listen(Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS, _ =>
+ this._handleToggleHideAllCommentThreads()
+ ),
+ listen(Shortcut.OPEN_FILE_LIST, _ => this._handleOpenFileList()),
+ listen(Shortcut.DIFF_AGAINST_BASE, _ => this._handleDiffAgainstBase()),
+ listen(Shortcut.DIFF_AGAINST_LATEST, _ =>
+ this._handleDiffAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LEFT, _ =>
+ this._handleDiffBaseAgainstLeft()
+ ),
+ listen(Shortcut.DIFF_RIGHT_AGAINST_LATEST, _ =>
+ this._handleDiffRightAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LATEST, _ =>
+ this._handleDiffBaseAgainstLatest()
+ ),
+ listen(Shortcut.EXPAND_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ listen(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ ];
}
private readonly reporting = appContext.reportingService;
@@ -334,7 +353,7 @@
private readonly shortcuts = appContext.shortcutsService;
- _throttledToggleFileReviewed?: IronKeyboardEventListener;
+ _throttledToggleFileReviewed?: (e: KeyboardEvent) => void;
_onRenderHandler?: EventListener;
@@ -344,8 +363,8 @@
override connectedCallback() {
super.connectedCallback();
- this._throttledToggleFileReviewed = throttleWrap(e =>
- this._handleToggleFileReviewed(e)
+ this._throttledToggleFileReviewed = throttleWrap(_ =>
+ this._handleToggleFileReviewed()
);
this._getLoggedIn().then(loggedIn => {
this._loggedIn = loggedIn;
@@ -371,7 +390,10 @@
};
this.$.diffHost.addEventListener('render', this._onRenderHandler);
this.cleanups.push(
- addGlobalShortcut({key: Key.ESC}, e => this._handleEscKey(e))
+ addGlobalShortcut(
+ {key: Key.ESC},
+ _ => (this.$.diffHost.displayLine = false)
+ )
);
}
@@ -525,81 +547,20 @@
);
}
- _handleToggleFileReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
- e.preventDefault();
+ _handleToggleFileReviewed() {
this._setReviewed(!this.$.reviewed.checked);
}
- _handleEscKey(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- e.preventDefault();
- this.$.diffHost.displayLine = false;
- }
-
- _handleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- this.cursor.moveLeft();
- }
-
- _handleRightPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- this.cursor.moveRight();
- }
-
- _handlePrevLineOrFileWithComments(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- if (
- e.detail.keyboardEvent?.shiftKey &&
- e.detail.keyboardEvent?.keyCode === 75
- ) {
- // 'K'
- this._moveToPreviousFileWithComment();
- return;
- }
- if (this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handlePrevLine() {
this.$.diffHost.displayLine = true;
this.cursor.moveUp();
}
- _handleVisibleLine(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- this.cursor.moveToVisibleArea();
- }
-
_onOpenFixPreview(e: OpenFixPreviewEvent) {
this.$.applyFixDialog.open(e);
}
- _handleNextLineOrFileWithComments(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- if (
- e.detail.keyboardEvent?.shiftKey &&
- e.detail.keyboardEvent?.keyCode === 74
- ) {
- // 'J'
- this._moveToNextFileWithComment();
- return;
- }
- if (this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleNextLine() {
this.$.diffHost.displayLine = true;
this.cursor.moveDown();
}
@@ -643,59 +604,34 @@
);
}
- _handleNewComment(ike: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(ike)) return;
- if (this.shortcuts.modifierPressed(ike)) return;
-
- ike.preventDefault();
+ _handleNewComment() {
this.classList.remove('hideComments');
this.cursor.createCommentInPlace();
}
- _handlePrevFile(ike: IronKeyboardEvent) {
- const ke = ike.detail.keyboardEvent;
- if (this.shortcuts.shouldSuppress(ike)) return;
- // Check for meta key to avoid overriding native chrome shortcut.
- if (ke.metaKey) return;
+ _handlePrevFile() {
if (!this._path) return;
if (!this._fileList) return;
-
- ike.preventDefault();
this._navToFile(this._path, this._fileList, -1);
}
- _handleNextFile(ike: IronKeyboardEvent) {
- const ke = ike.detail.keyboardEvent;
- if (this.shortcuts.shouldSuppress(ike)) return;
- // Check for meta key to avoid overriding native chrome shortcut.
- if (ke.metaKey) return;
+ _handleNextFile() {
if (!this._path) return;
if (!this._fileList) return;
-
- ike.preventDefault();
this._navToFile(this._path, this._fileList, 1);
}
- _handleNextChunkOrCommentThread(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleNextChunk() {
+ const result = this.cursor.moveToNextChunk();
+ if (result === CursorMoveResult.CLIPPED && this.cursor.isAtEnd()) {
+ this.showToastAndNavigateFile('next', 'n');
+ }
+ }
- e.preventDefault();
- if (e.detail.keyboardEvent?.shiftKey) {
- const result = this.cursor.moveToNextCommentThread();
- if (result === CursorMoveResult.CLIPPED) {
- this._navigateToNextFileWithCommentThread();
- }
- } else {
- if (this.shortcuts.modifierPressed(e)) return;
- const result = this.cursor.moveToNextChunk();
- // navigate to next file if key is not being held down
- if (
- !e.detail.keyboardEvent?.repeat &&
- result === CursorMoveResult.CLIPPED &&
- this.cursor.isAtEnd()
- ) {
- this.showToastAndNavigateFile('next', 'n');
- }
+ _handleNextCommentThread() {
+ const result = this.cursor.moveToNextCommentThread();
+ if (result === CursorMoveResult.CLIPPED) {
+ this._navigateToNextFileWithCommentThread();
}
}
@@ -737,25 +673,19 @@
this._navToFile(this._path, unreviewedFiles, direction === 'next' ? 1 : -1);
}
- _handlePrevChunkOrCommentThread(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- if (e.detail.keyboardEvent?.shiftKey) {
- this.cursor.moveToPreviousCommentThread();
- } else {
- if (this.shortcuts.modifierPressed(e)) return;
- this.cursor.moveToPreviousChunk();
- if (!e.detail.keyboardEvent?.repeat && this.cursor.isAtStart()) {
- this.showToastAndNavigateFile('previous', 'p');
- }
+ _handlePrevChunk() {
+ this.cursor.moveToPreviousChunk();
+ if (this.cursor.isAtStart()) {
+ this.showToastAndNavigateFile('previous', 'p');
}
}
+ _handlePrevCommentThread() {
+ this.cursor.moveToPreviousCommentThread();
+ }
+
// Similar to gr-change-view._handleOpenReplyDialog
- _handleOpenReplyDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
+ _handleOpenReplyDialog() {
this._getLoggedIn().then(isLoggedIn => {
if (!isLoggedIn) {
fireEvent(this, 'show-auth-required');
@@ -763,50 +693,29 @@
}
this.set('changeViewState.showReplyDialog', true);
- e.preventDefault();
this._navToChangeView();
});
}
- _handleToggleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (!e.detail.keyboardEvent?.shiftKey) return;
-
- e.preventDefault();
+ _handleToggleLeftPane() {
this.$.diffHost.toggleLeftDiff();
}
- _handleOpenDownloadDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
+ _handleOpenDownloadDialog() {
this.set('changeViewState.showDownloadDialog', true);
- e.preventDefault();
this._navToChangeView();
}
- _handleUpToChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
- e.preventDefault();
+ _handleUpToChange() {
this._navToChangeView();
}
- _handleCommaKey(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
+ _handleCommaKey() {
if (!this._loggedIn) return;
-
- e.preventDefault();
this.$.diffPreferencesDialog.open();
}
- _handleToggleDiffMode(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
- e.preventDefault();
+ _handleToggleDiffMode() {
if (this._getDiffViewMode() === DiffViewMode.SIDE_BY_SIDE) {
this.$.modeSelect.setMode(DiffViewMode.UNIFIED);
} else {
@@ -1689,28 +1598,19 @@
this._loadBlame();
}
- _handleToggleBlame(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
+ _handleToggleBlame() {
this._toggleBlame();
}
- _handleToggleHideAllCommentThreads(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
+ _handleToggleHideAllCommentThreads() {
toggleClass(this, 'hideComments');
}
- _handleOpenFileList(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
+ _handleOpenFileList() {
this.$.dropdown.open();
}
- _handleDiffAgainstBase(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffAgainstBase() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1726,8 +1626,7 @@
);
}
- _handleDiffBaseAgainstLeft(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffBaseAgainstLeft() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1747,8 +1646,7 @@
);
}
- _handleDiffAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffAgainstLatest() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1767,8 +1665,7 @@
);
}
- _handleDiffRightAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffRightAgainstLatest() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1786,8 +1683,7 @@
);
}
- _handleDiffBaseAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffBaseAgainstLatest() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1824,14 +1720,11 @@
return '';
}
- _handleToggleAllDiffContext(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
+ _handleToggleAllDiffContext() {
this.$.diffHost.toggleAllContext();
}
- _handleNextUnreviewedFile(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleNextUnreviewedFile() {
this._setReviewed(true);
this.navigateToUnreviewedFile('next');
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index d45ca0e..e4a8aa4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -426,7 +426,7 @@
test('toggle left diff with a hotkey', () => {
const toggleLeftDiffStub = sinon.stub(
element.$.diffHost, 'toggleLeftDiff');
- MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift', 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'A');
assert.isTrue(toggleLeftDiffStub.calledOnce);
});
@@ -498,12 +498,12 @@
assert(scrollStub.calledOnce);
scrollStub = sinon.stub(element.cursor, 'moveToNextCommentThread');
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
assert(scrollStub.calledOnce);
scrollStub = sinon.stub(element.cursor,
'moveToPreviousCommentThread');
- MockInteractions.pressAndReleaseKeyOn(element, 80, 'shift', 'p');
+ MockInteractions.pressAndReleaseKeyOn(element, 80, null, 'P');
assert(scrollStub.calledOnce);
const computeContainerClassStub = sinon.stub(element.$.diffHost.$.diff,
@@ -516,22 +516,29 @@
assert(computeContainerClassStub.lastCall.calledWithExactly(
false, 'SIDE_BY_SIDE', false));
+ // Note that stubbing _setReviewed means that the value of the
+ // `element.$.reviewed` checkbox is not flipped.
sinon.stub(element, '_setReviewed');
sinon.spy(element, '_handleToggleFileReviewed');
element.$.reviewed.checked = false;
- MockInteractions.keyUpOn(element, 82, 'shift', 'r');
+ assert.isFalse(element._handleToggleFileReviewed.called);
assert.isFalse(element._setReviewed.called);
- assert.isTrue(element._handleToggleFileReviewed.calledOnce);
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(element._handleToggleFileReviewed.calledOnce);
+ assert.isTrue(element._setReviewed.calledOnce);
+ assert.equal(element._setReviewed.lastCall.args[0], true);
+
+ // Handler is throttled, so another key press within 500 ms is ignored.
+ clock.tick(100);
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
+ assert.isTrue(element._handleToggleFileReviewed.calledOnce);
+ assert.isTrue(element._setReviewed.calledOnce);
clock.tick(1000);
-
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(element._handleToggleFileReviewed.calledTwice);
- assert.isTrue(element._setReviewed.called);
- assert.equal(element._setReviewed.lastCall.args[0], true);
+ assert.isTrue(element._setReviewed.calledTwice);
});
test('moveToNextCommentThread navigates to next file', () => {
@@ -563,14 +570,14 @@
element.changeViewState.selectedFileIndex = 1;
element._loggedIn = true;
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
flush();
assert.isTrue(diffNavStub.calledWithExactly(
element._change, 'wheatley.md', 10, PARENT, 21));
element._path = 'wheatley.md'; // navigated to next file
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
flush();
assert.isTrue(diffChangeStub.called);
@@ -578,7 +585,7 @@
test('shift+x shortcut toggles all diff context', () => {
const toggleStub = sinon.stub(element.$.diffHost, 'toggleAllContext');
- MockInteractions.pressAndReleaseKeyOn(element, 88, 'shift', 'x');
+ MockInteractions.pressAndReleaseKeyOn(element, 88, null, 'X');
flush();
assert.isTrue(toggleStub.called);
});
@@ -691,7 +698,7 @@
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(false));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- MockInteractions.keyUpOn(element, 65, null, 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(changeNavStub.notCalled, 'The `a` keyboard shortcut ' +
'should only work when the user is logged in.');
@@ -717,7 +724,7 @@
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(true));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- MockInteractions.keyUpOn(element, 65, null, 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(element.changeViewState.showReplyDialog);
assert(changeNavStub.lastCall.calledWithExactly(element._change, 10,
@@ -743,7 +750,7 @@
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(true));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- MockInteractions.keyUpOn(element, 65, null, 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(element.changeViewState.showReplyDialog);
assert(changeNavStub.lastCall.calledWithExactly(element._change, 1,
@@ -807,7 +814,7 @@
'Should navigate to /c/42/5..10');
assert.isUndefined(element.changeViewState.showDownloadDialog);
- MockInteractions.keyUpOn(element, 68, null, 'd');
+ MockInteractions.pressAndReleaseKeyOn(element, 68, null, 'd');
assert.isTrue(element.changeViewState.showDownloadDialog);
});
@@ -1730,7 +1737,7 @@
test('toggle blame with shortcut', () => {
const toggleBlame = sinon.stub(
element.$.diffHost, 'loadBlame').callsFake(() => Promise.resolve());
- MockInteractions.keyUpOn(element, 66, null, 'b');
+ MockInteractions.pressAndReleaseKeyOn(element, 66, null, 'b');
assert.isTrue(toggleBlame.calledOnce);
});
});
@@ -1890,7 +1897,7 @@
element._path = 'file1';
const reviewedStub = sinon.stub(element, '_setReviewed');
const navStub = sinon.stub(element, '_navToFile');
- MockInteractions.pressAndReleaseKeyOn(element, 77, 'shift', 'm');
+ MockInteractions.pressAndReleaseKeyOn(element, 77, null, 'M');
flush();
assert.isTrue(reviewedStub.lastCall.args[0]);
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
index 8821725..551889f 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
@@ -47,9 +47,6 @@
* @event create-comment-requested
*/
- @property({type: Object})
- keyEventTarget = document.body;
-
@property({type: Boolean})
positionBelow = false;
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index 38f55bd..d88eeaf 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -43,6 +43,7 @@
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {GerritNav} from './core/gr-navigation/gr-navigation';
import {appContext} from '../services/app-context';
@@ -68,7 +69,6 @@
import {GrMainHeader} from './core/gr-main-header/gr-main-header';
import {GrSettingsView} from './settings/gr-settings-view/gr-settings-view';
import {
- IronKeyboardEvent,
DialogChangeEventDetail,
EventType,
LocationChangeEvent,
@@ -81,6 +81,7 @@
import {LifeCycle} from '../constants/reporting';
import {fireIronAnnounce} from '../utils/event-util';
import {assertIsDefined} from '../utils/common-util';
+import {listen} from '../services/shortcuts/shortcuts-service';
interface ErrorInfo {
text: string;
@@ -120,9 +121,6 @@
@property({type: Object})
params?: AppElementParams;
- @property({type: Object})
- keyEventTarget = document.body;
-
@property({type: Object, observer: '_accountChanged'})
_account?: AccountDetailInfo;
@@ -214,17 +212,19 @@
private readonly restApiService = appContext.restApiService;
- private readonly shortcuts = appContext.shortcutsService;
-
- override keyboardShortcuts() {
- return {
- [Shortcut.OPEN_SHORTCUT_HELP_DIALOG]: '_showKeyboardShortcuts',
- [Shortcut.GO_TO_USER_DASHBOARD]: '_goToUserDashboard',
- [Shortcut.GO_TO_OPENED_CHANGES]: '_goToOpenedChanges',
- [Shortcut.GO_TO_MERGED_CHANGES]: '_goToMergedChanges',
- [Shortcut.GO_TO_ABANDONED_CHANGES]: '_goToAbandonedChanges',
- [Shortcut.GO_TO_WATCHED_CHANGES]: '_goToWatchedChanges',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, _ =>
+ this._showKeyboardShortcuts()
+ ),
+ listen(Shortcut.GO_TO_USER_DASHBOARD, _ => this._goToUserDashboard()),
+ listen(Shortcut.GO_TO_OPENED_CHANGES, _ => this._goToOpenedChanges()),
+ listen(Shortcut.GO_TO_MERGED_CHANGES, _ => this._goToMergedChanges()),
+ listen(Shortcut.GO_TO_ABANDONED_CHANGES, _ =>
+ this._goToAbandonedChanges()
+ ),
+ listen(Shortcut.GO_TO_WATCHED_CHANGES, _ => this._goToWatchedChanges()),
+ ];
}
constructor() {
@@ -502,8 +502,7 @@
(this.shadowRoot!.querySelector('#keyboardShortcuts') as GrOverlay).open();
}
- _showKeyboardShortcuts(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _showKeyboardShortcuts() {
// same shortcut should close the dialog if pressed again
// when dialog is open
this.loadKeyboardShortcutsDialog = true;
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
index afb695f..e0d1d15 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
@@ -23,13 +23,13 @@
import {customElement, property} from '@polymer/decorators';
import {htmlTemplate} from './gr-editable-label_html';
import {IronDropdownElement} from '@polymer/iron-dropdown/iron-dropdown';
-import {dom, EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
import {PaperInputElementExt} from '../../../types/types';
import {
AutocompleteQuery,
GrAutocomplete,
} from '../gr-autocomplete/gr-autocomplete';
import {addShortcut, Key} from '../../../utils/dom-util';
+import {queryAndAssert} from '../../../utils/common-util';
const AWAIT_MAX_ITERS = 10;
const AWAIT_STEP = 5;
@@ -212,18 +212,24 @@
this.getGrAutocomplete()) as HTMLInputElement;
}
- _handleEnter(e: KeyboardEvent) {
- const target = (dom(e) as EventApi).rootTarget;
- if (target === this._nativeInput) {
- e.preventDefault();
+ _handleEnter(event: KeyboardEvent) {
+ const inputContainer = queryAndAssert(this, '.inputContainer');
+ const isEventFromInput = event
+ .composedPath()
+ .some(element => element === inputContainer);
+ if (isEventFromInput) {
+ event.preventDefault();
this._save();
}
}
- _handleEsc(e: KeyboardEvent) {
- const target = (dom(e) as EventApi).rootTarget;
- if (target === this._nativeInput) {
- e.preventDefault();
+ _handleEsc(event: KeyboardEvent) {
+ const inputContainer = queryAndAssert(this, '.inputContainer');
+ const isEventFromInput = event
+ .composedPath()
+ .some(element => element === inputContainer);
+ if (isEventFromInput) {
+ event.preventDefault();
this._cancel();
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index 337d595..9e6b42a 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -17,7 +17,6 @@
import '../gr-autocomplete-dropdown/gr-autocomplete-dropdown';
import '../gr-cursor-manager/gr-cursor-manager';
import '../gr-overlay/gr-overlay';
-import '@polymer/iron-a11y-keys-behavior/iron-a11y-keys-behavior';
import '@polymer/iron-autogrow-textarea/iron-autogrow-textarea';
import '../../../styles/shared-styles';
import {flush} from '@polymer/polymer/lib/legacy/polymer.dom';
diff --git a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
index 02cfa9f..f133c116 100644
--- a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
+++ b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
@@ -14,12 +14,9 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import {IronA11yKeysBehavior} from '@polymer/iron-a11y-keys-behavior/iron-a11y-keys-behavior';
-import {mixinBehaviors} from '@polymer/polymer/lib/legacy/class';
import {property} from '@polymer/decorators';
import {PolymerElement} from '@polymer/polymer';
import {check, Constructor} from '../../utils/common-util';
-import {IronKeyboardEvent} from '../../types/events';
import {appContext} from '../../services/app-context';
import {
Shortcut,
@@ -27,7 +24,6 @@
SPECIAL_SHORTCUT,
} from '../../services/shortcuts/shortcuts-config';
import {
- ComboKey,
SectionView,
ShortcutListener,
} from '../../services/shortcuts/shortcuts-service';
@@ -40,18 +36,7 @@
SectionView,
};
-interface IronA11yKeysMixinConstructor {
- // Note: this is needed to have same interface as other mixins
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- new (...args: any[]): IronA11yKeysBehavior;
-}
-/**
- * @polymer
- * @mixinFunction
- */
-const InternalKeyboardShortcutMixin = <
- T extends Constructor<PolymerElement> & IronA11yKeysMixinConstructor
->(
+export const KeyboardShortcutMixin = <T extends Constructor<PolymerElement>>(
superClass: T
) => {
/**
@@ -59,14 +44,10 @@
* @mixinClass
*/
class Mixin extends superClass {
- @property({type: Object})
- _shortcut_go_table: Map<string, string> = new Map<string, string>();
-
- @property({type: Object})
- _shortcut_v_table: Map<string, string> = new Map<string, string>();
-
+ // This enables `Shortcut` to be used in the html template.
Shortcut = Shortcut;
+ // This enables `ShortcutSection` to be used in the html template.
ShortcutSection = ShortcutSection;
private readonly shortcuts = appContext.shortcutsService;
@@ -88,30 +69,6 @@
/** Are shortcuts currently enabled? True only when element is visible. */
private bindingsEnabled = false;
- _addOwnKeyBindings(shortcut: Shortcut, handler: string) {
- const bindings = this.shortcuts.getBindingsForShortcut(shortcut);
- if (!bindings) {
- return;
- }
- if (bindings[0] === SPECIAL_SHORTCUT.DOC_ONLY) {
- return;
- }
- if (bindings[0] === SPECIAL_SHORTCUT.GO_KEY) {
- bindings
- .slice(1)
- .forEach(binding => this._shortcut_go_table.set(binding, handler));
- } else if (bindings[0] === SPECIAL_SHORTCUT.V_KEY) {
- // for each binding added with the go/v key, we set the handler to be
- // handleVKeyAction. handleVKeyAction then looks up in th
- // shortcut_table to see what the relevant handler should be
- bindings
- .slice(1)
- .forEach(binding => this._shortcut_v_table.set(binding, handler));
- } else {
- this.addOwnKeyBinding(bindings.join(' '), handler);
- }
- }
-
override connectedCallback() {
super.connectedCallback();
this.createVisibilityObserver();
@@ -157,28 +114,7 @@
if (this.bindingsEnabled) return;
this.bindingsEnabled = true;
- const shortcuts = new Map<string, string>(
- Object.entries(this.keyboardShortcuts())
- );
- this.shortcuts.attachHost(this, shortcuts);
-
- for (const [key, value] of shortcuts.entries()) {
- this._addOwnKeyBindings(key as Shortcut, value);
- }
-
- // If any of the shortcuts utilized GO_KEY, then they are handled
- // directly by this behavior.
- if (this._shortcut_go_table.size > 0) {
- this._shortcut_go_table.forEach((_, key) => {
- this.addOwnKeyBinding(key, '_handleGoAction');
- });
- }
-
- if (this._shortcut_v_table.size > 0) {
- this._shortcut_v_table.forEach((_, key) => {
- this.addOwnKeyBinding(key, '_handleVAction');
- });
- }
+ this.shortcuts.attachHost(this, this.keyboardShortcuts());
}
/**
@@ -189,76 +125,22 @@
private disableBindings() {
if (!this.bindingsEnabled) return;
this.bindingsEnabled = false;
- if (this.shortcuts.detachHost(this)) {
- this.removeOwnKeyBindings();
- }
+ this.shortcuts.detachHost(this);
}
private hasKeyboardShortcuts() {
- return Object.entries(this.keyboardShortcuts()).length > 0;
+ return this.keyboardShortcuts().length > 0;
}
- keyboardShortcuts() {
- return {};
- }
-
- _handleVAction(e: IronKeyboardEvent) {
- if (
- !this.shortcuts.isInSpecificComboKeyMode(ComboKey.V) ||
- !this._shortcut_v_table.has(e.detail.key) ||
- this.shortcuts.shouldSuppress(e)
- ) {
- return;
- }
- e.preventDefault();
- const handler = this._shortcut_v_table.get(e.detail.key);
- if (handler) {
- // TODO(TS): should fix this
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (this as any)[handler](e);
- }
- }
-
- _handleGoAction(e: IronKeyboardEvent) {
- if (
- !this.shortcuts.isInSpecificComboKeyMode(ComboKey.G) ||
- !this._shortcut_go_table.has(e.detail.key) ||
- this.shortcuts.shouldSuppress(e)
- ) {
- return;
- }
- e.preventDefault();
- const handler = this._shortcut_go_table.get(e.detail.key);
- if (handler) {
- // TODO(TS): should fix this
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (this as any)[handler](e);
- }
+ keyboardShortcuts(): ShortcutListener[] {
+ return [];
}
}
return Mixin as T & Constructor<KeyboardShortcutMixinInterface>;
};
-// The following doesn't work (IronA11yKeysBehavior crashes):
-// const KeyboardShortcutMixin = superClass => {
-// class Mixin extends mixinBehaviors([IronA11yKeysBehavior], superClass) {
-// ...
-// }
-// return Mixin;
-// }
-// This is a workaround
-export const KeyboardShortcutMixin = <T extends Constructor<PolymerElement>>(
- superClass: T
-): T & Constructor<KeyboardShortcutMixinInterface> =>
- InternalKeyboardShortcutMixin(
- // TODO(TS): mixinBehaviors in some lib is returning: `new () => T` instead
- // which will fail the type check due to missing IronA11yKeysBehavior interface
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- mixinBehaviors([IronA11yKeysBehavior], superClass) as any
- );
-
/** The interface corresponding to KeyboardShortcutMixin */
export interface KeyboardShortcutMixinInterface {
- keyboardShortcuts(): {[key: string]: string | null};
+ keyboardShortcuts(): ShortcutListener[];
}
diff --git a/polygerrit-ui/app/package.json b/polygerrit-ui/app/package.json
index 2ad4e79..281a1eb 100644
--- a/polygerrit-ui/app/package.json
+++ b/polygerrit-ui/app/package.json
@@ -6,7 +6,6 @@
"@polymer/decorators": "^3.0.0",
"@polymer/font-roboto-local": "^3.0.2",
"@polymer/iron-a11y-announcer": "^3.1.0",
- "@polymer/iron-a11y-keys-behavior": "^3.0.1",
"@polymer/iron-autogrow-textarea": "^3.0.3",
"@polymer/iron-dropdown": "^3.0.1",
"@polymer/iron-fit-behavior": "^3.1.0",
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
index bd004d7..3c9e058 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
@@ -16,6 +16,8 @@
*/
/** Enum for all special shortcuts */
+import {ComboKey, Key, Modifier, Binding} from '../../utils/dom-util';
+
export enum SPECIAL_SHORTCUT {
DOC_ONLY = 'DOC_ONLY',
GO_KEY = 'GO_KEY',
@@ -115,7 +117,7 @@
export interface ShortcutHelpItem {
shortcut: Shortcut;
text: string;
- bindings: string[];
+ bindings: Binding[];
}
export const config = new Map<ShortcutSection, ShortcutHelpItem[]>();
@@ -124,8 +126,8 @@
shortcut: Shortcut,
section: ShortcutSection,
text: string,
- binding: string,
- ...moreBindings: string[]
+ binding: Binding,
+ ...moreBindings: Binding[]
) {
if (!config.has(section)) {
config.set(section, []);
@@ -136,417 +138,388 @@
}
}
-describe(Shortcut.SEARCH, ShortcutSection.EVERYWHERE, 'Search', '/');
+describe(Shortcut.SEARCH, ShortcutSection.EVERYWHERE, 'Search', {key: '/'});
describe(
Shortcut.OPEN_SHORTCUT_HELP_DIALOG,
ShortcutSection.EVERYWHERE,
'Show this dialog',
- '?'
+ {key: '?'}
);
describe(
Shortcut.GO_TO_USER_DASHBOARD,
ShortcutSection.EVERYWHERE,
'Go to User Dashboard',
- SPECIAL_SHORTCUT.GO_KEY,
- 'i'
+ {key: 'i', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_OPENED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Opened Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'o'
+ {key: 'o', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_MERGED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Merged Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'm'
+ {key: 'm', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_ABANDONED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Abandoned Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'a'
+ {key: 'a', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_WATCHED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Watched Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'w'
+ {key: 'w', combo: ComboKey.G}
);
describe(
Shortcut.CURSOR_NEXT_CHANGE,
ShortcutSection.ACTIONS,
'Select next change',
- 'j'
+ {key: 'j'}
);
describe(
Shortcut.CURSOR_PREV_CHANGE,
ShortcutSection.ACTIONS,
'Select previous change',
- 'k'
+ {key: 'k'}
);
describe(
Shortcut.OPEN_CHANGE,
ShortcutSection.ACTIONS,
'Show selected change',
- 'o'
+ {key: 'o'}
);
describe(
Shortcut.NEXT_PAGE,
ShortcutSection.ACTIONS,
'Go to next page',
- 'n',
- ']'
+ {key: 'n'},
+ {key: ']'}
);
describe(
Shortcut.PREV_PAGE,
ShortcutSection.ACTIONS,
'Go to previous page',
- 'p',
- '['
+ {key: 'p'},
+ {key: '['}
);
describe(
Shortcut.OPEN_REPLY_DIALOG,
ShortcutSection.ACTIONS,
'Open reply dialog to publish comments and add reviewers',
- 'a:keyup'
+ {key: 'a'}
);
describe(
Shortcut.OPEN_DOWNLOAD_DIALOG,
ShortcutSection.ACTIONS,
'Open download overlay',
- 'd:keyup'
+ {key: 'd'}
);
describe(
Shortcut.EXPAND_ALL_MESSAGES,
ShortcutSection.ACTIONS,
'Expand all messages',
- 'x'
+ {key: 'x'}
);
describe(
Shortcut.COLLAPSE_ALL_MESSAGES,
ShortcutSection.ACTIONS,
'Collapse all messages',
- 'z'
+ {key: 'z'}
);
describe(
Shortcut.REFRESH_CHANGE,
ShortcutSection.ACTIONS,
'Reload the change at the latest patch',
- 'shift+r:keyup'
+ {key: 'R'}
);
describe(
Shortcut.TOGGLE_CHANGE_REVIEWED,
ShortcutSection.ACTIONS,
'Mark/unmark change as reviewed',
- 'r:keyup'
+ {key: 'r'}
);
describe(
Shortcut.TOGGLE_FILE_REVIEWED,
ShortcutSection.ACTIONS,
'Toggle review flag on selected file',
- 'r:keyup'
+ {key: 'r'}
);
describe(
Shortcut.REFRESH_CHANGE_LIST,
ShortcutSection.ACTIONS,
'Refresh list of changes',
- 'shift+r:keyup'
+ {key: 'R'}
);
describe(
Shortcut.TOGGLE_CHANGE_STAR,
ShortcutSection.ACTIONS,
'Star/unstar change',
- 's:keydown'
+ {key: 's'}
);
describe(
Shortcut.OPEN_SUBMIT_DIALOG,
ShortcutSection.ACTIONS,
'Open submit dialog',
- 'shift+s'
+ {key: 'S'}
);
describe(
Shortcut.TOGGLE_ATTENTION_SET,
ShortcutSection.ACTIONS,
'Toggle attention set status',
- 'shift+t'
+ {key: 'T'}
);
-describe(
- Shortcut.EDIT_TOPIC,
- ShortcutSection.ACTIONS,
- 'Add a change topic',
- 't'
-);
+describe(Shortcut.EDIT_TOPIC, ShortcutSection.ACTIONS, 'Add a change topic', {
+ key: 't',
+});
describe(
Shortcut.DIFF_AGAINST_BASE,
ShortcutSection.DIFFS,
'Diff against base',
- SPECIAL_SHORTCUT.V_KEY,
- 'down',
- 's'
+ {key: Key.DOWN, combo: ComboKey.V},
+ {key: 's', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_AGAINST_LATEST,
ShortcutSection.DIFFS,
'Diff against latest patchset',
- SPECIAL_SHORTCUT.V_KEY,
- 'up',
- 'w'
+ {key: Key.UP, combo: ComboKey.V},
+ {key: 'w', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_BASE_AGAINST_LEFT,
ShortcutSection.DIFFS,
'Diff base against left',
- SPECIAL_SHORTCUT.V_KEY,
- 'left',
- 'a'
+ {key: Key.LEFT, combo: ComboKey.V},
+ {key: 'a', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_RIGHT_AGAINST_LATEST,
ShortcutSection.DIFFS,
'Diff right against latest',
- SPECIAL_SHORTCUT.V_KEY,
- 'right',
- 'd'
+ {key: Key.RIGHT, combo: ComboKey.V},
+ {key: 'd', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_BASE_AGAINST_LATEST,
ShortcutSection.DIFFS,
'Diff base against latest',
- SPECIAL_SHORTCUT.V_KEY,
- 'b'
+ {key: 'b', combo: ComboKey.V}
);
describe(
Shortcut.NEXT_LINE,
ShortcutSection.DIFFS,
'Go to next line',
- 'j',
- 'down'
+ {key: 'j'},
+ {key: Key.DOWN}
);
describe(
Shortcut.PREV_LINE,
ShortcutSection.DIFFS,
'Go to previous line',
- 'k',
- 'up'
+ {key: 'k'},
+ {key: Key.UP}
);
describe(
Shortcut.VISIBLE_LINE,
ShortcutSection.DIFFS,
'Move cursor to currently visible code',
- '.'
+ {key: '.'}
);
-describe(
- Shortcut.NEXT_CHUNK,
- ShortcutSection.DIFFS,
- 'Go to next diff chunk',
- 'n'
-);
+describe(Shortcut.NEXT_CHUNK, ShortcutSection.DIFFS, 'Go to next diff chunk', {
+ key: 'n',
+});
describe(
Shortcut.PREV_CHUNK,
ShortcutSection.DIFFS,
'Go to previous diff chunk',
- 'p'
+ {key: 'p'}
);
describe(
Shortcut.TOGGLE_ALL_DIFF_CONTEXT,
ShortcutSection.DIFFS,
'Toggle all diff context',
- 'shift+x'
+ {key: 'X'}
);
describe(
Shortcut.NEXT_COMMENT_THREAD,
ShortcutSection.DIFFS,
'Go to next comment thread',
- 'shift+n'
+ {key: 'N'}
);
describe(
Shortcut.PREV_COMMENT_THREAD,
ShortcutSection.DIFFS,
'Go to previous comment thread',
- 'shift+p'
+ {key: 'P'}
);
describe(
Shortcut.EXPAND_ALL_COMMENT_THREADS,
ShortcutSection.DIFFS,
'Expand all comment threads',
- SPECIAL_SHORTCUT.DOC_ONLY,
- 'e'
+ {key: 'e', docOnly: true}
);
describe(
Shortcut.COLLAPSE_ALL_COMMENT_THREADS,
ShortcutSection.DIFFS,
'Collapse all comment threads',
- SPECIAL_SHORTCUT.DOC_ONLY,
- 'shift+e'
+ {key: 'E', docOnly: true}
);
describe(
Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS,
ShortcutSection.DIFFS,
'Hide/Display all comment threads',
- 'h'
+ {key: 'h'}
);
-describe(
- Shortcut.LEFT_PANE,
- ShortcutSection.DIFFS,
- 'Select left pane',
- 'shift+left'
-);
-describe(
- Shortcut.RIGHT_PANE,
- ShortcutSection.DIFFS,
- 'Select right pane',
- 'shift+right'
-);
+describe(Shortcut.LEFT_PANE, ShortcutSection.DIFFS, 'Select left pane', {
+ key: Key.LEFT,
+ modifiers: [Modifier.SHIFT_KEY],
+});
+describe(Shortcut.RIGHT_PANE, ShortcutSection.DIFFS, 'Select right pane', {
+ key: Key.RIGHT,
+ modifiers: [Modifier.SHIFT_KEY],
+});
describe(
Shortcut.TOGGLE_LEFT_PANE,
ShortcutSection.DIFFS,
'Hide/show left diff',
- 'shift+a'
+ {key: 'A'}
);
-describe(Shortcut.NEW_COMMENT, ShortcutSection.DIFFS, 'Draft new comment', 'c');
+describe(Shortcut.NEW_COMMENT, ShortcutSection.DIFFS, 'Draft new comment', {
+ key: 'c',
+});
describe(
Shortcut.SAVE_COMMENT,
ShortcutSection.DIFFS,
'Save comment',
- 'ctrl+enter',
- 'meta+enter',
- 'ctrl+s',
- 'meta+s'
+ {key: Key.ENTER, modifiers: [Modifier.CTRL_KEY]},
+ {key: Key.ENTER, modifiers: [Modifier.META_KEY]},
+ {key: 's', modifiers: [Modifier.CTRL_KEY]},
+ {key: 's', modifiers: [Modifier.META_KEY]}
);
describe(
Shortcut.OPEN_DIFF_PREFS,
ShortcutSection.DIFFS,
'Show diff preferences',
- ','
+ {key: ','}
);
describe(
Shortcut.TOGGLE_DIFF_REVIEWED,
ShortcutSection.DIFFS,
'Mark/unmark file as reviewed',
- 'r:keyup'
+ {key: 'r'}
);
describe(
Shortcut.TOGGLE_DIFF_MODE,
ShortcutSection.DIFFS,
'Toggle unified/side-by-side diff',
- 'm:keyup'
+ {key: 'm'}
);
describe(
Shortcut.NEXT_UNREVIEWED_FILE,
ShortcutSection.DIFFS,
'Mark file as reviewed and go to next unreviewed file',
- 'shift+m'
+ {key: 'M'}
);
-describe(
- Shortcut.TOGGLE_BLAME,
- ShortcutSection.DIFFS,
- 'Toggle blame',
- 'b:keyup'
-);
-describe(Shortcut.OPEN_FILE_LIST, ShortcutSection.DIFFS, 'Open file list', 'f');
-describe(
- Shortcut.NEXT_FILE,
- ShortcutSection.NAVIGATION,
- 'Go to next file',
- ']'
-);
+describe(Shortcut.TOGGLE_BLAME, ShortcutSection.DIFFS, 'Toggle blame', {
+ key: 'b',
+});
+describe(Shortcut.OPEN_FILE_LIST, ShortcutSection.DIFFS, 'Open file list', {
+ key: 'f',
+});
+describe(Shortcut.NEXT_FILE, ShortcutSection.NAVIGATION, 'Go to next file', {
+ key: ']',
+});
describe(
Shortcut.PREV_FILE,
ShortcutSection.NAVIGATION,
'Go to previous file',
- '['
+ {key: '['}
);
describe(
Shortcut.NEXT_FILE_WITH_COMMENTS,
ShortcutSection.NAVIGATION,
'Go to next file that has comments',
- 'shift+j'
+ {key: 'J'}
);
describe(
Shortcut.PREV_FILE_WITH_COMMENTS,
ShortcutSection.NAVIGATION,
'Go to previous file that has comments',
- 'shift+k'
+ {key: 'K'}
);
describe(
Shortcut.OPEN_FIRST_FILE,
ShortcutSection.NAVIGATION,
'Go to first file',
- ']'
+ {key: ']'}
);
describe(
Shortcut.OPEN_LAST_FILE,
ShortcutSection.NAVIGATION,
'Go to last file',
- '['
+ {key: '['}
);
describe(
Shortcut.UP_TO_DASHBOARD,
ShortcutSection.NAVIGATION,
'Up to dashboard',
- 'u'
+ {key: 'u'}
);
-describe(
- Shortcut.UP_TO_CHANGE,
- ShortcutSection.NAVIGATION,
- 'Up to change',
- 'u'
-);
+describe(Shortcut.UP_TO_CHANGE, ShortcutSection.NAVIGATION, 'Up to change', {
+ key: 'u',
+});
describe(
Shortcut.CURSOR_NEXT_FILE,
ShortcutSection.FILE_LIST,
'Select next file',
- 'j',
- 'down'
+ {key: 'j'},
+ {key: Key.DOWN}
);
describe(
Shortcut.CURSOR_PREV_FILE,
ShortcutSection.FILE_LIST,
'Select previous file',
- 'k',
- 'up'
+ {key: 'k'},
+ {key: Key.UP}
);
describe(
Shortcut.OPEN_FILE,
ShortcutSection.FILE_LIST,
'Go to selected file',
- 'o',
- 'enter'
+ {key: 'o'},
+ {key: Key.ENTER}
);
describe(
Shortcut.TOGGLE_ALL_INLINE_DIFFS,
ShortcutSection.FILE_LIST,
'Show/hide all inline diffs',
- 'shift+i'
+ {key: 'I'}
);
describe(
Shortcut.TOGGLE_INLINE_DIFF,
ShortcutSection.FILE_LIST,
'Show/hide selected inline diff',
- 'i'
+ {key: 'i'}
);
describe(
Shortcut.SEND_REPLY,
ShortcutSection.REPLY_DIALOG,
'Send reply',
- SPECIAL_SHORTCUT.DOC_ONLY,
- 'ctrl+enter',
- 'meta+enter'
+ {key: Key.ENTER, modifiers: [Modifier.CTRL_KEY], docOnly: true},
+ {key: Key.ENTER, modifiers: [Modifier.META_KEY], docOnly: true}
);
describe(
Shortcut.EMOJI_DROPDOWN,
ShortcutSection.REPLY_DIALOG,
'Emoji dropdown',
- SPECIAL_SHORTCUT.DOC_ONLY,
- ':'
+ {key: ':', docOnly: true}
);
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index edc31a4..19f12c5 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -19,27 +19,39 @@
Shortcut,
ShortcutHelpItem,
ShortcutSection,
- SPECIAL_SHORTCUT,
} from './shortcuts-config';
import {disableShortcuts$} from '../user/user-model';
-import {IronKeyboardEvent, isIronKeyboardEvent} from '../../types/events';
-import {isElementTarget, isModifierPressed} from '../../utils/dom-util';
+import {
+ ComboKey,
+ eventMatchesShortcut,
+ isElementTarget,
+ Key,
+ Modifier,
+ Binding,
+} from '../../utils/dom-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
export type SectionView = Array<{binding: string[][]; text: string}>;
+export interface ShortcutListener {
+ shortcut: Shortcut;
+ listener: (e: KeyboardEvent) => void;
+}
+
+export function listen(
+ shortcut: Shortcut,
+ listener: (e: KeyboardEvent) => void
+): ShortcutListener {
+ return {shortcut, listener};
+}
+
/**
* The interface for listener for shortcut events.
*/
-export type ShortcutListener = (
+export type ShortcutViewListener = (
viewMap?: Map<ShortcutSection, SectionView>
) => void;
-export enum ComboKey {
- G = 'g',
- V = 'v',
-}
-
function isComboKey(key: string): key is ComboKey {
return Object.values(ComboKey).includes(key as ComboKey);
}
@@ -55,12 +67,18 @@
* show a shortcut help dialog that only shows the shortcuts that are
* currently relevant.
*/
- private readonly activeHosts = new Map<unknown, Map<string, string>>();
+ private readonly activeShortcuts = new Map<HTMLElement, Shortcut[]>();
+
+ /**
+ * Keeps track of cleanup callbacks (which remove keyboard listeners) that
+ * have to be invoked when a component unregisters itself.
+ */
+ private readonly cleanupsPerHost = new Map<HTMLElement, (() => void)[]>();
/** Static map built in the constructor by iterating over the config. */
- private readonly bindings = new Map<Shortcut, string[]>();
+ private readonly bindings = new Map<Shortcut, Binding[]>();
- private readonly listeners = new Set<ShortcutListener>();
+ private readonly listeners = new Set<ShortcutViewListener>();
/**
* Stores the timestamp of the last combo key being pressed.
@@ -89,7 +107,7 @@
}
public _testOnly_isEmpty() {
- return this.activeHosts.size === 0 && this.listeners.size === 0;
+ return this.activeShortcuts.size === 0 && this.listeners.size === 0;
}
isInComboKeyMode() {
@@ -107,13 +125,35 @@
);
}
- modifierPressed(e: IronKeyboardEvent) {
- return isModifierPressed(e) || this.isInComboKeyMode();
+ /**
+ * TODO(brohlfs): Reconcile with the addShortcut() function in dom-util.
+ * Most likely we will just keep this one here, but that is something for a
+ * follow-up change.
+ */
+ addShortcut(
+ element: HTMLElement,
+ shortcut: Binding,
+ listener: (e: KeyboardEvent) => void
+ ) {
+ const wrappedListener = (e: KeyboardEvent) => {
+ if (e.repeat) return;
+ if (!eventMatchesShortcut(e, shortcut)) return;
+ if (shortcut.combo) {
+ if (!this.isInSpecificComboKeyMode(shortcut.combo)) return;
+ } else {
+ if (this.isInComboKeyMode()) return;
+ }
+ if (this.shouldSuppress(e)) return;
+ e.preventDefault();
+ e.stopPropagation();
+ listener(e);
+ };
+ element.addEventListener('keydown', wrappedListener);
+ return () => element.removeEventListener('keydown', wrappedListener);
}
- shouldSuppress(event: IronKeyboardEvent | KeyboardEvent) {
+ shouldSuppress(e: KeyboardEvent) {
if (this.shortcutsDisabled) return true;
- const e = isIronKeyboardEvent(event) ? event.detail.keyboardEvent : event;
// 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
@@ -168,23 +208,37 @@
return this.bindings.get(shortcut);
}
- attachHost(host: unknown, shortcuts: Map<string, string>) {
- this.activeHosts.set(host, shortcuts);
- this.notifyListeners();
+ attachHost(host: HTMLElement, shortcuts: ShortcutListener[]) {
+ this.activeShortcuts.set(
+ host,
+ shortcuts.map(s => s.shortcut)
+ );
+ const cleanups: (() => void)[] = [];
+ for (const s of shortcuts) {
+ const bindings = this.getBindingsForShortcut(s.shortcut);
+ for (const binding of bindings ?? []) {
+ if (binding.docOnly) continue;
+ cleanups.push(this.addShortcut(document.body, binding, s.listener));
+ }
+ }
+ this.cleanupsPerHost.set(host, cleanups);
+ this.notifyViewListeners();
}
- detachHost(host: unknown) {
- if (!this.activeHosts.delete(host)) return false;
- this.notifyListeners();
+ detachHost(host: HTMLElement) {
+ this.activeShortcuts.delete(host);
+ const cleanups = this.cleanupsPerHost.get(host);
+ for (const cleanup of cleanups ?? []) cleanup();
+ this.notifyViewListeners();
return true;
}
- addListener(listener: ShortcutListener) {
+ addListener(listener: ShortcutViewListener) {
this.listeners.add(listener);
listener(this.directoryView());
}
- removeListener(listener: ShortcutListener) {
+ removeListener(listener: ShortcutViewListener) {
return this.listeners.delete(listener);
}
@@ -199,15 +253,17 @@
const bindings = this.bindings.get(shortcutName);
if (!bindings) return '';
return bindings
- .map(binding => this.describeBinding(binding).join('+'))
+ .map(binding => describeBinding(binding).join('+'))
.join(',');
}
activeShortcutsBySection() {
- const activeShortcuts = new Set<string>();
- this.activeHosts.forEach(shortcuts => {
- shortcuts.forEach((_, shortcut) => activeShortcuts.add(shortcut));
- });
+ const activeShortcuts = new Set<Shortcut>();
+ for (const shortcuts of this.activeShortcuts.values()) {
+ for (const shortcut of shortcuts) {
+ activeShortcuts.add(shortcut);
+ }
+ }
const activeShortcutsBySection = new Map<
ShortcutSection,
@@ -219,8 +275,6 @@
if (!activeShortcutsBySection.has(section)) {
activeShortcutsBySection.set(section, []);
}
- // From previous condition, the `get(section)`
- // should always return a valid result
activeShortcutsBySection.get(section)!.push(shortcutHelp);
}
});
@@ -282,63 +336,53 @@
describeBindings(shortcut: Shortcut): string[][] | null {
const bindings = this.bindings.get(shortcut);
- if (!bindings) {
- return null;
- }
- if (bindings[0] === SPECIAL_SHORTCUT.GO_KEY) {
- return bindings
- .slice(1)
- .map(binding => this._describeKey(binding))
- .map(binding => ['g'].concat(binding));
- }
- if (bindings[0] === SPECIAL_SHORTCUT.V_KEY) {
- return bindings
- .slice(1)
- .map(binding => this._describeKey(binding))
- .map(binding => ['v'].concat(binding));
- }
-
+ if (!bindings) return null;
return bindings
- .filter(binding => binding !== SPECIAL_SHORTCUT.DOC_ONLY)
- .map(binding => this.describeBinding(binding));
+ .filter(binding => !binding.docOnly)
+ .map(binding => describeBinding(binding));
}
- _describeKey(key: string) {
- switch (key) {
- case 'shift':
- return 'Shift';
- case 'meta':
- return 'Meta';
- case 'ctrl':
- return 'Ctrl';
- case 'enter':
- return 'Enter';
- case 'up':
- return '\u2191'; // ↑
- case 'down':
- return '\u2193'; // ↓
- case 'left':
- return '\u2190'; // ←
- case 'right':
- return '\u2192'; // →
- default:
- return key;
- }
- }
-
- describeBinding(binding: string) {
- // single key bindings
- if (binding.length === 1) {
- return [binding];
- }
- return binding
- .split(':')[0]
- .split('+')
- .map(part => this._describeKey(part));
- }
-
- notifyListeners() {
+ notifyViewListeners() {
const view = this.directoryView();
this.listeners.forEach(listener => listener(view));
}
}
+
+function describeKey(key: string | Key) {
+ switch (key) {
+ case Key.UP:
+ return '\u2191'; // ↑
+ case Key.DOWN:
+ return '\u2193'; // ↓
+ case Key.LEFT:
+ return '\u2190'; // ←
+ case Key.RIGHT:
+ return '\u2192'; // →
+ default:
+ return key;
+ }
+}
+
+export function describeBinding(binding: Binding): string[] {
+ const description: string[] = [];
+ if (binding.combo === ComboKey.G) {
+ description.push('g');
+ }
+ if (binding.combo === ComboKey.V) {
+ description.push('v');
+ }
+ if (binding.modifiers?.includes(Modifier.SHIFT_KEY)) {
+ description.push('Shift');
+ }
+ if (binding.modifiers?.includes(Modifier.ALT_KEY)) {
+ description.push('Alt');
+ }
+ if (binding.modifiers?.includes(Modifier.CTRL_KEY)) {
+ description.push('Ctrl');
+ }
+ if (binding.modifiers?.includes(Modifier.META_KEY)) {
+ description.push('Meta/Cmd');
+ }
+ description.push(describeKey(binding.key));
+ return description;
+}
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
index d8aa11e..05c4f53 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
@@ -16,12 +16,14 @@
*/
import '../../test/common-test-setup-karma';
import {
- ShortcutsService,
COMBO_TIMEOUT_MS,
+ describeBinding,
+ ShortcutsService,
} from '../../services/shortcuts/shortcuts-service';
import {Shortcut, ShortcutSection} from './shortcuts-config';
import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
import {SinonFakeTimers} from 'sinon';
+import {Key, Modifier} from '../../utils/dom-util';
async function keyEventOn(
el: HTMLElement,
@@ -96,13 +98,12 @@
});
test('getShortcut', () => {
- const NEXT_FILE = Shortcut.NEXT_FILE;
- assert.equal(service.getShortcut(NEXT_FILE), ']');
- });
-
- test('getShortcut with modifiers', () => {
- const NEXT_FILE = Shortcut.TOGGLE_LEFT_PANE;
- assert.equal(service.getShortcut(NEXT_FILE), 'Shift+a');
+ assert.equal(service.getShortcut(Shortcut.NEXT_FILE), ']');
+ assert.equal(service.getShortcut(Shortcut.TOGGLE_LEFT_PANE), 'A');
+ assert.equal(
+ service.getShortcut(Shortcut.SEND_REPLY),
+ 'Ctrl+Enter,Meta/Cmd+Enter'
+ );
});
suite('binding descriptions', () => {
@@ -113,14 +114,18 @@
}
test('single combo description', () => {
- assert.deepEqual(service.describeBinding('a'), ['a']);
- assert.deepEqual(service.describeBinding('a:keyup'), ['a']);
- assert.deepEqual(service.describeBinding('ctrl+a'), ['Ctrl', 'a']);
- assert.deepEqual(service.describeBinding('ctrl+shift+up:keyup'), [
- 'Ctrl',
- 'Shift',
- '↑',
- ]);
+ assert.deepEqual(describeBinding({key: 'a'}), ['a']);
+ assert.deepEqual(
+ describeBinding({key: 'a', modifiers: [Modifier.CTRL_KEY]}),
+ ['Ctrl', 'a']
+ );
+ assert.deepEqual(
+ describeBinding({
+ key: Key.UP,
+ modifiers: [Modifier.CTRL_KEY, Modifier.SHIFT_KEY],
+ }),
+ ['Shift', 'Ctrl', '↑']
+ );
});
test('combo set description', () => {
@@ -130,9 +135,9 @@
);
assert.deepEqual(service.describeBindings(Shortcut.SAVE_COMMENT), [
['Ctrl', 'Enter'],
- ['Meta', 'Enter'],
+ ['Meta/Cmd', 'Enter'],
['Ctrl', 's'],
- ['Meta', 's'],
+ ['Meta/Cmd', 's'],
]);
assert.deepEqual(service.describeBindings(Shortcut.PREV_FILE), [['[']]);
});
@@ -187,67 +192,68 @@
test('active shortcuts by section', () => {
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {});
- service.attachHost({}, new Map([[Shortcut.NEXT_FILE, 'null']]));
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.NEXT_FILE, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
- service.attachHost({}, new Map([[Shortcut.NEXT_LINE, 'null']]));
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.NEXT_LINE, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.DIFFS]: [
{
shortcut: Shortcut.NEXT_LINE,
text: 'Go to next line',
- bindings: ['j', 'down'],
+ bindings: [{key: 'j'}, {key: 'ArrowDown'}],
},
],
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
- service.attachHost(
- {},
- new Map([
- [Shortcut.SEARCH, 'null'],
- [Shortcut.GO_TO_OPENED_CHANGES, 'null'],
- ])
- );
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.SEARCH, listener: _ => {}},
+ {shortcut: Shortcut.GO_TO_OPENED_CHANGES, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.DIFFS]: [
{
shortcut: Shortcut.NEXT_LINE,
text: 'Go to next line',
- bindings: ['j', 'down'],
+ bindings: [{key: 'j'}, {key: 'ArrowDown'}],
},
],
[ShortcutSection.EVERYWHERE]: [
{
shortcut: Shortcut.SEARCH,
text: 'Search',
- bindings: ['/'],
+ bindings: [{key: '/'}],
},
{
shortcut: Shortcut.GO_TO_OPENED_CHANGES,
text: 'Go to Opened Changes',
- bindings: ['GO_KEY', 'o'],
+ bindings: [{key: 'o', combo: 'g'}],
},
],
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
@@ -256,33 +262,31 @@
test('directory view', () => {
assert.deepEqual(mapToObject(service.directoryView()), {});
- service.attachHost(
- {},
- new Map([
- [Shortcut.GO_TO_OPENED_CHANGES, 'null'],
- [Shortcut.NEXT_FILE, 'null'],
- [Shortcut.NEXT_LINE, 'null'],
- [Shortcut.SAVE_COMMENT, 'null'],
- [Shortcut.SEARCH, 'null'],
- ])
- );
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.GO_TO_OPENED_CHANGES, listener: _ => {}},
+ {shortcut: Shortcut.NEXT_FILE, listener: _ => {}},
+ {shortcut: Shortcut.NEXT_LINE, listener: _ => {}},
+ {shortcut: Shortcut.SAVE_COMMENT, listener: _ => {}},
+ {shortcut: Shortcut.SEARCH, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.directoryView()), {
[ShortcutSection.DIFFS]: [
{binding: [['j'], ['↓']], text: 'Go to next line'},
{
- binding: [
- ['Ctrl', 'Enter'],
- ['Meta', 'Enter'],
- ],
+ binding: [['Ctrl', 'Enter']],
text: 'Save comment',
},
{
binding: [
+ ['Meta/Cmd', 'Enter'],
['Ctrl', 's'],
- ['Meta', 's'],
],
text: 'Save comment',
},
+ {
+ binding: [['Meta/Cmd', 's']],
+ text: 'Save comment',
+ },
],
[ShortcutSection.EVERYWHERE]: [
{binding: [['/']], text: 'Search'},
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index c78f61a..b6376c4 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -241,30 +241,3 @@
title: string;
}
export type TitleChangeEvent = CustomEvent<TitleChangeEventDetail>;
-
-/**
- * Keyboard events emitted from elements using IronA11yKeysBehavior: That means
- * that the element returns a list of handlers from either `keyBindings()` or
- * from `keyboardShortcuts()`. This event should not be used in Lit elements
- * and will be obsolete once the Lit migration is completed.
- */
-export interface IronKeyboardEvent extends CustomEvent {
- detail: IronKeyboardEventDetail;
-}
-
-export interface IronKeyboardEventDetail {
- keyboardEvent: KeyboardEvent;
- key: string;
- combo?: string;
-}
-
-export function isIronKeyboardEvent(
- e: IronKeyboardEvent | Event | CustomEvent
-): e is IronKeyboardEvent {
- const ike = e as IronKeyboardEvent;
- return !!ike?.detail?.keyboardEvent;
-}
-
-export interface IronKeyboardEventListener {
- (evt: IronKeyboardEvent): void;
-}
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index a7ad6e1..b6dece0 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -16,7 +16,6 @@
*/
import {EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
import {check} from './common-util';
-import {IronKeyboardEvent} from '../types/events';
/**
* Event emitted from polymer elements.
@@ -326,8 +325,18 @@
SHIFT_KEY,
}
-export interface Shortcut {
+export enum ComboKey {
+ G = 'g',
+ V = 'v',
+}
+
+export interface Binding {
key: string | Key;
+ /** Defaults to false. */
+ docOnly?: boolean;
+ /** Defaults to not being a combo shortcut. */
+ combo?: ComboKey;
+ /** Defaults to no modifiers. */
modifiers?: Modifier[];
}
@@ -358,7 +367,7 @@
export function eventMatchesShortcut(
e: KeyboardEvent,
- shortcut: Shortcut
+ shortcut: Binding
): boolean {
if (e.key !== shortcut.key) return false;
const modifiers = shortcut.modifiers ?? [];
@@ -380,7 +389,7 @@
}
export function addGlobalShortcut(
- shortcut: Shortcut,
+ shortcut: Binding,
listener: (e: KeyboardEvent) => void
) {
return addShortcut(document.body, shortcut, listener);
@@ -388,12 +397,14 @@
export function addShortcut(
element: HTMLElement,
- shortcut: Shortcut,
+ shortcut: Binding,
listener: (e: KeyboardEvent) => void
) {
const wrappedListener = (e: KeyboardEvent) => {
if (e.repeat) return;
- if (eventMatchesShortcut(e, shortcut)) listener(e);
+ if (eventMatchesShortcut(e, shortcut)) {
+ listener(e);
+ }
};
element.addEventListener('keydown', wrappedListener);
return () => element.removeEventListener('keydown', wrappedListener);
@@ -406,11 +417,3 @@
export function shiftPressed(e: KeyboardEvent) {
return e.shiftKey;
}
-
-export function isModifierPressed(e: IronKeyboardEvent) {
- return modifierPressed(e.detail.keyboardEvent);
-}
-
-export function isShiftPressed(e: IronKeyboardEvent) {
- return shiftPressed(e.detail.keyboardEvent);
-}
diff --git a/polygerrit-ui/app/yarn.lock b/polygerrit-ui/app/yarn.lock
index bfca566..d3b22eb 100644
--- a/polygerrit-ui/app/yarn.lock
+++ b/polygerrit-ui/app/yarn.lock
@@ -46,7 +46,7 @@
dependencies:
"@polymer/polymer" "^3.0.0"
-"@polymer/iron-a11y-keys-behavior@^3.0.0-pre.26", "@polymer/iron-a11y-keys-behavior@^3.0.1":
+"@polymer/iron-a11y-keys-behavior@^3.0.0-pre.26":
version "3.0.1"
resolved "https://registry.yarnpkg.com/@polymer/iron-a11y-keys-behavior/-/iron-a11y-keys-behavior-3.0.1.tgz#2868ea72912d2007ffab4734684a91f5aac49b84"
integrity sha512-lnrjKq3ysbBPT/74l0Fj0U9H9C35Tpw2C/tpJ8a+5g8Y3YJs1WSZYnEl1yOkw6sEyaxOq/1DkzH0+60gGu5/PQ==
