From 06c933781e9704e167084e9dd6d642905ff7f4a3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 May 2017 06:36:03 -0700 Subject: [PATCH] Move keyboard focus reticle code to Differential Summary: Ref T12634. Using keyboard shortcuts in Differential currently relies on focus behavior in `KeyboardShortcutManager`. This possibly made sense long ago, but no longer does, and leads to a whole slew of bugs where the reticle doesn't interact properly with anything else. Move it to Differential so it can be made reasonably aware of edit operations, Quicksand navigation, etc. This just moves the code; future diffs will actually fix bugs. Test Plan: Used "n", "j", etc., saw the same behavior as before. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12634 Differential Revision: https://secure.phabricator.com/D17899 --- .../js/application/diff/DiffChangesetList.js | 62 +++++++++++++++---- .../rsrc/js/core/KeyboardShortcutManager.js | 43 ------------- 2 files changed, 49 insertions(+), 56 deletions(-) diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 27b539a311..b5fde20329 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -60,7 +60,10 @@ JX.install('DiffChangesetList', { _changesets: null, _cursorItem: null, - _lastKeyboardManager: null, + + _focusNode: null, + _focusStart: null, + _focusEnd: null, sleep: function() { this._asleep = true; @@ -198,7 +201,7 @@ JX.install('DiffChangesetList', { if (cursor.type == 'comment') { var inline = cursor.inline; if (inline.canReply()) { - manager.focusOn(null); + this.setFocus(null); inline.reply(); return; @@ -217,7 +220,7 @@ JX.install('DiffChangesetList', { if (cursor.type == 'comment') { var inline = cursor.inline; if (inline.canEdit()) { - manager.focusOn(null); + this.setFocus(null); inline.edit(); return; @@ -310,21 +313,13 @@ JX.install('DiffChangesetList', { }, _redrawSelection: function(manager, scroll) { - manager = manager || this._lastKeyboardManager; - this._lastKeyboardManager = manager; - - if (this.isAsleep()) { - manager.focusOn(null); - return; - } - var cursor = this._cursorItem; if (!cursor) { - manager.focusOn(null); + this.setFocus(null); return; } - manager.focusOn(cursor.nodes.begin, cursor.nodes.end); + this.setFocus(cursor.nodes.begin, cursor.nodes.end); if (scroll) { manager.scrollTo(cursor.nodes.begin); @@ -644,6 +639,47 @@ JX.install('DiffChangesetList', { } }, + setFocus: function(node, extended_node) { + this._focusStart = node; + this._focusEnd = extended_node; + this._redrawFocus(); + }, + + _redrawFocus: function() { + var node = this._focusStart; + var extended_node = this._focusEnd || node; + + var reticle = this._getFocusNode(); + if (!node) { + JX.DOM.remove(reticle); + return; + } + + // Outset the reticle some pixels away from the element, so there's some + // space between the focused element and the outline. + var p = JX.Vector.getPos(node); + var s = JX.Vector.getAggregateScrollForNode(node); + + p.add(s).add(-4, -4).setPos(reticle); + // Compute the size we need to extend to the full extent of the focused + // nodes. + JX.Vector.getPos(extended_node) + .add(-p.x, -p.y) + .add(JX.Vector.getDim(extended_node)) + .add(8, 8) + .setDim(reticle); + + JX.DOM.getContentFrame().appendChild(reticle); + }, + + _getFocusNode: function() { + if (!this._focusNode) { + var node = JX.$N('div', {className : 'keyboard-focus-focus-reticle'}); + this._focusNode = node; + } + return this._focusNode; + }, + _deleteInlineByID: function(id) { var uri = this.getInlineURI(); var data = { diff --git a/webroot/rsrc/js/core/KeyboardShortcutManager.js b/webroot/rsrc/js/core/KeyboardShortcutManager.js index c9a1fbe64e..281c12a8f3 100644 --- a/webroot/rsrc/js/core/KeyboardShortcutManager.js +++ b/webroot/rsrc/js/core/KeyboardShortcutManager.js @@ -54,7 +54,6 @@ JX.install('KeyboardShortcutManager', { members : { _shortcuts : null, - _focusReticle : null, /** * Instead of calling this directly, you should call @@ -83,48 +82,6 @@ JX.install('KeyboardShortcutManager', { JX.DOM.scrollToPosition(0, node_position.y + scroll_distance.y - 60); }, - /** - * Move the keyboard shortcut focus to an element. - * - * @param Node Node to focus, or pass null to clear the focus. - * @param Node To focus multiple nodes (like rows in a table), specify the - * top-left node as the first parameter and the bottom-right - * node as the focus extension. - * @return void - */ - focusOn : function(node, extended_node) { - this._clearReticle(); - - if (!node) { - return; - } - - var r = JX.$N('div', {className : 'keyboard-focus-focus-reticle'}); - - extended_node = extended_node || node; - - // Outset the reticle some pixels away from the element, so there's some - // space between the focused element and the outline. - var p = JX.Vector.getPos(node); - var s = JX.Vector.getAggregateScrollForNode(node); - - p.add(s).add(-4, -4).setPos(r); - // Compute the size we need to extend to the full extent of the focused - // nodes. - JX.Vector.getPos(extended_node) - .add(-p.x, -p.y) - .add(JX.Vector.getDim(extended_node)) - .add(8, 8) - .setDim(r); - JX.DOM.getContentFrame().appendChild(r); - - this._focusReticle = r; - }, - - _clearReticle : function() { - this._focusReticle && JX.DOM.remove(this._focusReticle); - this._focusReticle = null; - }, _onkeypress : function(e) { if (!(this._getKey(e) in JX.KeyboardShortcutManager._downkeys)) { this._onkeyhit(e);