From 90db5020c967a08f83d1bf36cfafd0ba3c1fa9f3 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Fri, 2 Oct 2020 15:26:14 -0400 Subject: [PATCH] (core) Improve focus and keyboard shortcuts in modals. Summary: - Factor out focusing logic from Clipboard to FocusLayer. - Generalize FocusLayer to support adding a temporary layer while a modal is open. - Stop Mousetrap shortcuts while a modal is open. - Refactor how Mousetrap's custom stopCallback is implemented to avoid needing to bundle knockout for mousetrap. Test Plan: Added a test that Enter in a UserManager doesn't open a cell editor from underneath the modal. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2626 --- app/client/components/Clipboard.js | 87 +++------------ app/client/components/commands.js | 2 +- app/client/lib/FocusLayer.ts | 174 +++++++++++++++++++++++++++++ app/client/lib/Mousetrap.js | 18 ++- app/client/ui2018/modals.ts | 16 ++- app/common/RefCountMap.ts | 8 ++ 6 files changed, 224 insertions(+), 81 deletions(-) create mode 100644 app/client/lib/FocusLayer.ts diff --git a/app/client/components/Clipboard.js b/app/client/components/Clipboard.js index 5802122d..868cfdbc 100644 --- a/app/client/components/Clipboard.js +++ b/app/client/components/Clipboard.js @@ -32,10 +32,10 @@ -/* global window, document, $ */ +/* global window, document */ -var ko = require('knockout'); var {tsvDecode} = require('app/common/tsvFormat'); +var {FocusLayer} = require('app/client/lib/FocusLayer'); var commands = require('./commands'); var dom = require('../lib/dom'); @@ -48,9 +48,6 @@ function Clipboard(app) { this.copypasteField = this.autoDispose(dom('textarea.copypaste.mousetrap', '')); this.timeoutId = null; - this.onEvent(window, 'focus', this.grabFocus); - this.onEvent(this.copypasteField, 'blur', this.grabFocus); - this.onEvent(this.copypasteField, 'input', function(elem, event) { var value = elem.value; elem.value = ''; @@ -62,34 +59,29 @@ function Clipboard(app) { this.onEvent(this.copypasteField, 'paste', this._onPaste); document.body.appendChild(this.copypasteField); - this.grabFocus(); - // The following block of code deals with what happens when the window is in the background. - // When it is, focus and blur events are unreliable, and we'll watch explicitly for events which - // may cause a change in focus. These wouldn't happen normally for a background window, but do - // happen in Selenium Webdriver testing. - var grabber = this.grabFocus.bind(this); - function setBackgroundCapture(onOff) { - var addRemove = onOff ? window.addEventListener : window.removeEventListener; - // Note the third argument useCapture=true, which lets us notice these events before other - // code that might call .stopPropagation on them. - addRemove.call(window, 'click', grabber, true); - addRemove.call(window, 'mousedown', grabber, true); - addRemove.call(window, 'keydown', grabber, true); - } - this.onEvent(window, 'blur', setBackgroundCapture.bind(null, true)); - this.onEvent(window, 'focus', setBackgroundCapture.bind(null, false)); - setBackgroundCapture(!document.hasFocus()); + FocusLayer.create(this, { + defaultFocusElem: this.copypasteField, + allowFocus: isCopyPasteTarget, + onDefaultFocus: () => { + this.copypasteField.value = ' '; + this.copypasteField.select(); + this._app.trigger('clipboard_focus'); + }, + onDefaultBlur: () => { + this._app.trigger('clipboard_blur'); + }, + }); // Expose the grabber as a global to allow upload from tests to explicitly restore focus - window.gristClipboardGrabFocus = grabber; + window.gristClipboardGrabFocus = () => FocusLayer.grabFocus(); // Some bugs may prevent Clipboard from re-grabbing focus. To limit the impact of such bugs on // the user, recover from a bad state in mousedown events. (At the moment of this comment, all // such known bugs are fixed.) this.onEvent(window, 'mousedown', (ev) => { if (!document.activeElement || document.activeElement === document.body) { - this.grabFocus(); + FocusLayer.grabFocus(); } }); @@ -180,24 +172,6 @@ Clipboard.prototype._onPaste = function(elem, event) { this._cutCallback = null; }; -/** - * Helper to watch a focused element to lose focus, in which point the Clipboard will grab it. - * Because elements getting removed from the DOM don't always trigger 'blur' event, this also - * watches for the element getting disposed (using ko.removeNode or ko.cleanNode). - */ -Clipboard.prototype._watchElementForBlur = function(elem) { - var self = this; - function done() { - $(elem).off('blur.clipboard'); - ko.utils.domNodeDisposal.removeDisposeCallback(elem, done); - self.grabFocus(); - } - $(elem).one('blur.clipboard', done); - // TODO We need to add proper integration of grainjs and knockout dom-disposal. Otherwise a - // focused node that's disposed by grainjs will not trigger this knockout disposal callback. - ko.utils.domNodeDisposal.addDisposeCallback(elem, done); -}; - var FOCUS_TARGET_TAGS = { 'INPUT': true, 'TEXTAREA': true, @@ -216,33 +190,4 @@ function isCopyPasteTarget(elem) { elem.classList.contains('clipboard_focus')); } -/** - * Select the special copypaste field to capture clipboard events. - */ -Clipboard.prototype.grabFocus = function() { - if (!this.timeoutId) { - var self = this; - this.timeoutId = setTimeout(() => { - if (self.isDisposed()) { return; } - self.timeoutId = null; - if (document.activeElement === self.copypasteField) { - return; - } - // If the window doesn't have focus, don't rush to grab it, or we can interfere with focus - // outside the frame when embedded. We'll grab focus when setBackgroundCapture tells us to. - if (!document.hasFocus()) { - return; - } - if (isCopyPasteTarget(document.activeElement)) { - self._watchElementForBlur(document.activeElement); - self._app.trigger('clipboard_blur'); - } else { - self.copypasteField.value = ' '; - self.copypasteField.select(); - self._app.trigger('clipboard_focus'); - } - }, 0); - } -}; - module.exports = Clipboard; diff --git a/app/client/components/commands.js b/app/client/components/commands.js index 5745427c..3be23819 100644 --- a/app/client/components/commands.js +++ b/app/client/components/commands.js @@ -310,7 +310,7 @@ CommandGroup.prototype._removeGroup = function() { * See also stopCallback in app/client/lib/Mousetrap.js. */ CommandGroup.prototype.attach = dom.inlinable(function(elem) { - ko.utils.domData.set(elem, 'mousetrapCommandGroup', this); + Mousetrap.setCustomStopCallback(elem, (combo) => !this.knownKeys.hasOwnProperty(combo)); }); //---------------------------------------------------------------------- diff --git a/app/client/lib/FocusLayer.ts b/app/client/lib/FocusLayer.ts new file mode 100644 index 00000000..c7e02cf4 --- /dev/null +++ b/app/client/lib/FocusLayer.ts @@ -0,0 +1,174 @@ +/** + * FocusLayer addresses the issue of where focus goes "by default". In most of Grist operation, + * the focus is on the special Clipboard element to support typing into cells, and copy-pasting. + * When a modal is open, the focus is on the modal. + * + * When the focus moves to some specific element such as a textbox or a dropdown menu, the + * FocusLayerManager will watch for this element to lose focus or to get disposed, and will + * restore focus to the default element. + */ +import {arrayRemove} from 'app/common/gutil'; +import {RefCountMap} from 'app/common/RefCountMap'; +import {Disposable, dom} from 'grainjs'; + +/** + * The default focus is organized into layers. A layer determines when focus should move to the + * default element, and what that element should be. Only the top (most recently created) layer is + * active at any given time. + */ +export interface FocusLayerOptions { + // The default element that should have focus while this layer is active. + defaultFocusElem: HTMLElement; + + // When true for an element, that element may hold focus even while this layer is active. + allowFocus: (elem: Element) => boolean; + + // Called when the defaultFocusElem gets focused. + onDefaultFocus?: () => void; + + // Called when the defaultFocusElem gets blurred. + onDefaultBlur?: () => void; +} + +// Use RefCountMap to have a reference-counted instance of the global FocusLayerManager. It will +// be active as long as at least one FocusLayer is active (i.e. not disposed). +const _focusLayerManager = new RefCountMap({ + create: (key) => FocusLayerManager.create(null), + dispose: (key, value) => value.dispose(), + gracePeriodMs: 10, +}); + +/** + * The FocusLayerManager implements the functionality, using the top (most recently created) layer + * to determine when and to what to move focus. + */ +class FocusLayerManager extends Disposable { + private _timeoutId: ReturnType | null = null; + private _focusLayers: FocusLayer[] = []; + + constructor() { + super(); + + const grabFocus = this.grabFocus.bind(this); + + this.autoDispose(dom.onElem(window, 'focus', grabFocus)); + this.grabFocus(); + + // The following block of code deals with what happens when the window is in the background. + // When it is, focus and blur events are unreliable, and we'll watch explicitly for events which + // may cause a change in focus. These wouldn't happen normally for a background window, but do + // happen in Selenium Webdriver testing. + function setBackgroundCapture(onOff: boolean) { + const addRemove = onOff ? window.addEventListener : window.removeEventListener; + // Note the third argument useCapture=true, which lets us notice these events before other + // code that might call .stopPropagation on them. + addRemove.call(window, 'click', grabFocus, true); + addRemove.call(window, 'mousedown', grabFocus, true); + addRemove.call(window, 'keydown', grabFocus, true); + } + this.autoDispose(dom.onElem(window, 'blur', setBackgroundCapture.bind(null, true))); + this.autoDispose(dom.onElem(window, 'focus', setBackgroundCapture.bind(null, false))); + setBackgroundCapture(!document.hasFocus()); + } + + public addLayer(layer: FocusLayer) { + this._focusLayers.push(layer); + // Move the focus to the new layer. Not just grabFocus, because if the focus is on the previous + // layer's defaultFocusElem, the new layer might consider it "allowed" and never get the focus. + setTimeout(() => layer.defaultFocusElem.focus(), 0); + } + + public removeLayer(layer: FocusLayer) { + arrayRemove(this._focusLayers, layer); + // Give the remaining layers a chance to check focus. + this.grabFocus(); + } + + public getCurrentLayer(): FocusLayer|undefined { + return this._focusLayers[this._focusLayers.length - 1]; + } + + /** + * Select the default focus element, or wait until the current element loses focus. + */ + public grabFocus() { + if (!this._timeoutId) { + this._timeoutId = setTimeout(() => this._doGrabFocus(), 0); + } + } + + private _doGrabFocus() { + if (this.isDisposed()) { return; } + this._timeoutId = null; + const layer = this.getCurrentLayer(); + if (!layer || document.activeElement === layer.defaultFocusElem) { + return; + } + // If the window doesn't have focus, don't rush to grab it, or we can interfere with focus + // outside the frame when embedded. We'll grab focus when setBackgroundCapture tells us to. + if (!document.hasFocus()) { + return; + } + if (document.activeElement && layer.allowFocus(document.activeElement)) { + watchElementForBlur(document.activeElement, () => this.grabFocus()); + layer.onDefaultBlur?.(); + } else { + layer.defaultFocusElem.focus(); + layer.onDefaultFocus?.(); + } + } +} + +/** + * An individual FocusLayer determines where focus should default to while this layer is active. + */ +export class FocusLayer extends Disposable implements FocusLayerOptions { + // FocusLayer.grabFocus() allows triggering the focus check manually. + public static grabFocus() { + _focusLayerManager.get(null)?.grabFocus(); + } + + public defaultFocusElem: HTMLElement; + public allowFocus: (elem: Element) => boolean; + public onDefaultFocus?: () => void; + public onDefaultBlur?: () => void; + + constructor(options: FocusLayerOptions) { + super(); + this.defaultFocusElem = options.defaultFocusElem; + this.allowFocus = options.allowFocus; + this.onDefaultFocus = options.onDefaultFocus; + this.onDefaultBlur = options.onDefaultBlur; + + const managerRefCount = this.autoDispose(_focusLayerManager.use(null)); + const manager = managerRefCount.get(); + manager.addLayer(this); + this.onDispose(() => manager.removeLayer(this)); + this.autoDispose(dom.onElem(this.defaultFocusElem, 'blur', () => manager.grabFocus())); + } +} + +/** + * Helper to watch a focused element to lose focus, at which point callback() will get called. + * Because elements getting removed from the DOM don't always trigger 'blur' event, this also + * uses MutationObserver to watch for the element to get removed from DOM. + */ +function watchElementForBlur(elem: Element, callback: () => void) { + const maybeDone = () => { + if (document.activeElement !== elem) { + lis.dispose(); + observer.disconnect(); + callback(); + } + }; + const lis = dom.onElem(elem, 'blur', maybeDone); + + // Watch for the removal of elem by observing the childList of all its ancestors. + // (Just guessing that it is more efficient than watching document.body with {subtree: true}). + const observer = new MutationObserver(maybeDone); + let parent = elem.parentNode; + while (parent) { + observer.observe(parent, {childList: true}); + parent = parent.parentNode; + } +} diff --git a/app/client/lib/Mousetrap.js b/app/client/lib/Mousetrap.js index 9d79a522..74fb4714 100644 --- a/app/client/lib/Mousetrap.js +++ b/app/client/lib/Mousetrap.js @@ -17,13 +17,14 @@ if (typeof window === 'undefined') { } else { var Mousetrap = require('mousetrap'); - var ko = require('knockout'); // Minus is different on Gecko: // see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode // and https://github.com/ccampbell/mousetrap/pull/215 Mousetrap.addKeycodes({173: '-'}); + var customStopCallbacks = new WeakMap(); + var MousetrapProtype = Mousetrap.prototype; var origStopCallback = MousetrapProtype.stopCallback; @@ -36,9 +37,10 @@ if (typeof window === 'undefined') { if (mousetrapBindingsPaused) { return true; } - var cmdGroup = ko.utils.domData.get(element, 'mousetrapCommandGroup'); - if (cmdGroup) { - return !cmdGroup.knownKeys.hasOwnProperty(combo); + // If we have a custom stopCallback, use it now. + const custom = customStopCallbacks.get(element); + if (custom) { + return custom(combo); } try { return origStopCallback.call(this, e, element, combo, sequence); @@ -63,5 +65,13 @@ if (typeof window === 'undefined') { mousetrapBindingsPaused = yesNo; }; + /** + * Set a custom stopCallback for an element. When a key combo is pressed for this element, + * callback(combo) is called. If it returns true, Mousetrap should NOT process the combo. + */ + Mousetrap.setCustomStopCallback = function(element, callback) { + customStopCallbacks.set(element, callback); + }; + module.exports = Mousetrap; } diff --git a/app/client/ui2018/modals.ts b/app/client/ui2018/modals.ts index 44ba792a..ee4e580e 100644 --- a/app/client/ui2018/modals.ts +++ b/app/client/ui2018/modals.ts @@ -1,3 +1,5 @@ +import {FocusLayer} from 'app/client/lib/FocusLayer'; +import * as Mousetrap from 'app/client/lib/Mousetrap'; import {reportError} from 'app/client/models/errors'; import {bigBasicButton, bigPrimaryButton, cssButton} from 'app/client/ui2018/buttons'; import {colors, testId, vars} from 'app/client/ui2018/cssVars'; @@ -52,18 +54,22 @@ export function modal(createFn: (ctl: IModalControl, owner: MultiHolder) => DomE const modalDom = cssModalBacker( dom.create((owner) => { + // Pause mousetrap keyboard shortcuts while the modal is shown. Without this, arrow keys + // will navigate in a grid underneath the modal, and Enter may open a cell there. + Mousetrap.setPaused(true); + owner.onDispose(() => Mousetrap.setPaused(false)); + const focus = () => dialog.focus(); const dialog = cssModalDialog( createFn({ close, focus }, owner), dom.on('click', (ev) => ev.stopPropagation()), options.noEscapeKey ? null : dom.onKeyDown({ Escape: close }), - // Focus the dialog to allow it to receive keyboard events. - // When triggered by a weasel menu, the menu grabs restores focus after getting closed to the - // element focused before it was opened. This interferes with focusing the modal, so we need to - // wait a bit and focus later. TODO: Weasel menus should stop creating problems with focus. - (elem) => { setTimeout(() => elem.focus(), 10); }, testId('modal-dialog') ); + FocusLayer.create(owner, { + defaultFocusElem: dialog, + allowFocus: (elem) => (elem !== document.body), + }); return dialog; }), options.noClickAway ? null : dom.on('click', close), diff --git a/app/common/RefCountMap.ts b/app/common/RefCountMap.ts index 614722f1..4526076e 100644 --- a/app/common/RefCountMap.ts +++ b/app/common/RefCountMap.ts @@ -49,6 +49,14 @@ export class RefCountMap implements IDisposable { }; } + /** + * Return the value for the key, if one is set, or undefined otherwise, without touching + * reference counts. + */ + public get(key: Key): Value|undefined { + return this._map.get(key)?.value; + } + /** * Purge a key by immediately removing it from the map. Disposing the remaining IRefCountSub * values will be no-ops.