Do not process keyboard shortcuts for invisible views
Since change 305575 the change- and diff-views are not destroyed
anymore, but stay around invisibly as long as the change number does
not change. This broke keyboard shortcut handling, because the handlers
of the invisible views were competing with the handlers from of the
visible ones.
This change enhances the keyboard shortcut mixin to not just run the
setup and destroy code on element connect and disconnect, but also when
the visibility of the view changes.
Bug: Issue 14522
Change-Id: I4e7c49ea26861f4e11faa6826a6e710366523075
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 837a795..62589f8 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
@@ -102,7 +102,7 @@
import {dedupingMixin} from '@polymer/polymer/lib/utils/mixin';
import {property} from '@polymer/decorators';
import {PolymerElement} from '@polymer/polymer';
-import {Constructor} from '../../utils/common-util';
+import {check, Constructor} from '../../utils/common-util';
import {getKeyboardEvent, isModifierPressed} from '../../utils/dom-util';
import {
CustomKeyboardEvent,
@@ -222,11 +222,6 @@
viewMap?: Map<ShortcutSection, SectionView>
) => void;
-interface ShortcutEnabledElement extends PolymerElement {
- // TODO: should replace with Map so we can have proper type here
- keyboardShortcuts(): {[shortcut: string]: string};
-}
-
interface ShortcutHelpItem {
shortcut: Shortcut;
text: string;
@@ -558,14 +553,9 @@
return this.bindings.get(shortcut);
}
- attachHost(host: PolymerElement | ShortcutEnabledElement) {
- if (!('keyboardShortcuts' in host)) {
- return;
- }
- const shortcuts = host.keyboardShortcuts();
- this.activeHosts.set(host, new Map(Object.entries(shortcuts)));
+ attachHost(host: PolymerElement, shortcuts: Map<string, string>) {
+ this.activeHosts.set(host, shortcuts);
this.notifyListeners();
- return shortcuts;
}
detachHost(host: PolymerElement) {
@@ -788,6 +778,12 @@
private readonly restApiService = appContext.restApiService;
+ /** Used to disable shortcuts when the element is not visible. */
+ private observer?: IntersectionObserver;
+
+ /** Are shortcuts currently enabled? True only when element is visible. */
+ private bindingsEnabled = false;
+
modifierPressed(event: CustomKeyboardEvent) {
/* We are checking for g/v as modifiers pressed. There are cases such as
* pressing v and then /, where we want the handler for / to be triggered.
@@ -902,21 +898,62 @@
/** @override */
connectedCallback() {
super.connectedCallback();
-
this.restApiService.getPreferences().then(prefs => {
if (prefs?.disable_keyboard_shortcuts) {
this._disableKeyboardShortcuts = true;
}
});
+ this.createVisibilityObserver();
+ this.enableBindings();
+ }
- const shortcuts = shortcutManager.attachHost(this);
- if (!shortcuts) {
- return;
- }
+ /** @override */
+ disconnectedCallback() {
+ this.destroyVisibilityObserver();
+ this.disableBindings();
+ super.disconnectedCallback();
+ }
- for (const key of Object.keys(shortcuts)) {
- // TODO(TS): not needed if convert shortcuts to Map
- this._addOwnKeyBindings(key as Shortcut, shortcuts[key]);
+ /**
+ * Creates an intersection observer that enables bindings when the
+ * element is visible and disables them when the element is hidden.
+ */
+ private createVisibilityObserver() {
+ if (!this.hasKeyboardShortcuts()) return;
+ if (this.observer) return;
+ this.observer = new IntersectionObserver(entries => {
+ check(entries.length === 1, 'Expected one observer entry.');
+ const isVisible = entries[0].isIntersecting;
+ if (isVisible) {
+ this.enableBindings();
+ } else {
+ this.disableBindings();
+ }
+ });
+ this.observer.observe(this);
+ }
+
+ private destroyVisibilityObserver() {
+ if (this.observer) this.observer.unobserve(this);
+ }
+
+ /**
+ * Enables all the shortcuts returned by keyboardShortcuts().
+ * This is a private method being called when the element becomes
+ * connected or visible.
+ */
+ private enableBindings() {
+ if (!this.hasKeyboardShortcuts()) return;
+ if (this.bindingsEnabled) return;
+ this.bindingsEnabled = true;
+
+ const shortcuts = new Map<string, string>(
+ Object.entries(this.keyboardShortcuts())
+ );
+ shortcutManager.attachHost(this, shortcuts);
+
+ for (const [key, value] of shortcuts.entries()) {
+ this._addOwnKeyBindings(key as Shortcut, value);
}
// each component that uses this behaviour must be aware if go key is
@@ -941,12 +978,21 @@
}
}
- /** @override */
- disconnectedCallback() {
+ /**
+ * Disables all the shortcuts returned by keyboardShortcuts().
+ * This is a private method being called when the element becomes
+ * disconnected or invisible.
+ */
+ private disableBindings() {
+ if (!this.bindingsEnabled) return;
+ this.bindingsEnabled = false;
if (shortcutManager.detachHost(this)) {
this.removeOwnKeyBindings();
}
- super.disconnectedCallback();
+ }
+
+ private hasKeyboardShortcuts() {
+ return Object.entries(this.keyboardShortcuts()).length > 0;
}
keyboardShortcuts() {
diff --git a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.js b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.js
index b22b8d8..d5e7fe7 100644
--- a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.js
+++ b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.js
@@ -181,13 +181,7 @@
mapToObject(mgr.activeShortcutsBySection()),
{});
- mgr.attachHost({
- keyboardShortcuts() {
- return {
- [Shortcut.NEXT_FILE]: null,
- };
- },
- });
+ mgr.attachHost({}, new Map([[Shortcut.NEXT_FILE, null]]));
assert.deepEqual(
mapToObject(mgr.activeShortcutsBySection()),
{
@@ -196,13 +190,7 @@
],
});
- mgr.attachHost({
- keyboardShortcuts() {
- return {
- [Shortcut.NEXT_LINE]: null,
- };
- },
- });
+ mgr.attachHost({}, new Map([[Shortcut.NEXT_LINE, null]]));
assert.deepEqual(
mapToObject(mgr.activeShortcutsBySection()),
{
@@ -214,14 +202,10 @@
],
});
- mgr.attachHost({
- keyboardShortcuts() {
- return {
- [Shortcut.SEARCH]: null,
- [Shortcut.GO_TO_OPENED_CHANGES]: null,
- };
- },
- });
+ mgr.attachHost({}, new Map([
+ [Shortcut.SEARCH, null],
+ [Shortcut.GO_TO_OPENED_CHANGES, null],
+ ]));
assert.deepEqual(
mapToObject(mgr.activeShortcutsBySection()),
{
@@ -254,17 +238,13 @@
assert.deepEqual(mapToObject(mgr.directoryView()), {});
- mgr.attachHost({
- keyboardShortcuts() {
- return {
- [Shortcut.GO_TO_OPENED_CHANGES]: null,
- [Shortcut.NEXT_FILE]: null,
- [Shortcut.NEXT_LINE]: null,
- [Shortcut.SAVE_COMMENT]: null,
- [Shortcut.SEARCH]: null,
- };
- },
- });
+ mgr.attachHost({}, new Map([
+ [Shortcut.GO_TO_OPENED_CHANGES, null],
+ [Shortcut.NEXT_FILE, null],
+ [Shortcut.NEXT_LINE, null],
+ [Shortcut.SAVE_COMMENT, null],
+ [Shortcut.SEARCH, null],
+ ]));
assert.deepEqual(
mapToObject(mgr.directoryView()),
{
