(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

@@ -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.
+  FocusLayer.create(this, {
-  // When it is, focus and blur events are unreliable, and we'll watch explicitly for events which
+    defaultFocusElem: this.copypasteField,
-  // may cause a change in focus. These wouldn't happen normally for a background window, but do
+    allowFocus: isCopyPasteTarget,
-  // happen in Selenium Webdriver testing.
+    onDefaultFocus: () => {
-  var grabber = this.grabFocus.bind(this);
+      this.copypasteField.value = ' ';
-  function setBackgroundCapture(onOff) {
+      this.copypasteField.select();
-    var addRemove = onOff ? window.addEventListener : window.removeEventListener;
+      this._app.trigger('clipboard_focus');
-    // Note the third argument useCapture=true, which lets us notice these events before other
+    },
-    // code that might call .stopPropagation on them.
+    onDefaultBlur: () => {
-    addRemove.call(window, 'click', grabber, true);
+      this._app.trigger('clipboard_blur');
-    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());
 
   // 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;

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));
 });
 
 //----------------------------------------------------------------------

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<null, FocusLayerManager>({
+  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<typeof setTimeout> | 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;
+  }
+}

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 we have a custom stopCallback, use it now.
-    if (cmdGroup) {
+    const custom = customStopCallbacks.get(element);
-      return !cmdGroup.knownKeys.hasOwnProperty(combo);
+    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;
 }

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),

app/common/RefCountMap.ts

@@ -49,6 +49,14 @@ export class RefCountMap<Key, Value> 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.