Move most Differetial keyboard shortcuts into DiffChangesetList

Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.

Test Plan:
  - Used j, k, n, p, J, K shortcuts to navigate a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17859

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '2ff7879f',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '58712637',
-    'differential.pkg.js' => '6375358e',
+    'differential.pkg.js' => 'e6129b80',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -390,15 +390,14 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
-    'rsrc/js/application/diff/DiffChangeset.js' => '4c9c47ad',
+    'rsrc/js/application/diff/DiffChangeset.js' => '3d4b3c5e',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '589a30aa',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'e2c315d9',
     'rsrc/js/application/diff/DiffInline.js' => '98c12b2f',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
     'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
     'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
     'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '89d11432',
-    'rsrc/js/application/differential/behavior-keyboard-nav.js' => '92904457',
     'rsrc/js/application/differential/behavior-populate.js' => '5e41c819',
     'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
     'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d',
@@ -624,7 +623,6 @@ return array(
     'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
     'javelin-behavior-differential-edit-inline-comments' => '89d11432',
     'javelin-behavior-differential-feedback-preview' => 'b064af76',
-    'javelin-behavior-differential-keyboard-navigation' => '92904457',
     'javelin-behavior-differential-populate' => '5e41c819',
     'javelin-behavior-differential-toggle-files' => 'ca3f91eb',
     'javelin-behavior-differential-user-select' => 'a8d8459d',
@@ -783,8 +781,8 @@ return array(
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => '4c9c47ad',
+    'phabricator-diff-changeset' => '3d4b3c5e',
-    'phabricator-diff-changeset-list' => '589a30aa',
+    'phabricator-diff-changeset-list' => 'e2c315d9',
     'phabricator-diff-inline' => '98c12b2f',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
@@ -1160,6 +1158,17 @@ return array(
       'javelin-util',
       'javelin-uri',
     ),
+    '3d4b3c5e' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     '3dbf94d5' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1270,17 +1279,6 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
-    '4c9c47ad' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     '4d863052' => array(
       'javelin-dom',
       'javelin-util',
@@ -1348,9 +1346,6 @@ return array(
       'javelin-vector',
       'javelin-dom',
     ),
-    '589a30aa' => array(
-      'javelin-install',
-    ),
     '58dea2fa' => array(
       'javelin-install',
       'javelin-util',
@@ -1629,12 +1624,6 @@ return array(
       'javelin-dom',
       'javelin-request',
     ),
-    92904457 => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-stratcom',
-      'phabricator-keyboard-shortcut',
-    ),
     '92b9ec77' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2119,6 +2108,9 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
+    'e2c315d9' => array(
+      'javelin-install',
+    ),
     'e2e0a072' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2440,7 +2432,6 @@ return array(
       'javelin-behavior-differential-populate',
       'javelin-behavior-differential-diff-radios',
       'javelin-behavior-differential-comment-jump',
-      'javelin-behavior-differential-keyboard-navigation',
       'javelin-behavior-aphront-drag-and-drop-textarea',
       'javelin-behavior-phabricator-object-selector',
       'javelin-behavior-repository-crossreference',

resources/celerity/packages.php

@@ -197,7 +197,6 @@ return array(
     'javelin-behavior-differential-populate',
     'javelin-behavior-differential-diff-radios',
     'javelin-behavior-differential-comment-jump',
-    'javelin-behavior-differential-keyboard-navigation',
     'javelin-behavior-aphront-drag-and-drop-textarea',
     'javelin-behavior-phabricator-object-selector',
     'javelin-behavior-repository-crossreference',

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -465,7 +465,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
     }
 
     Javelin::initBehavior('differential-user-select');
-    Javelin::initBehavior('differential-keyboard-navigation');
 
     $view = id(new PHUITwoColumnView())
       ->setHeader($header)

src/applications/differential/view/DifferentialChangesetListView.php

@@ -234,6 +234,16 @@ final class DifferentialChangesetListView extends AphrontView {
         'Highlight As...' => pht('Highlight As...'),
 
         'Loading...' => pht('Loading...'),
+
+        'Jump to next change.' => pht('Jump to next change.'),
+        'Jump to previous change.' => pht('Jump to previous change.'),
+        'Jump to next file.' => pht('Jump to next file.'),
+        'Jump to previous file.' => pht('Jump to previous file.'),
+        'Jump to next inline comment.' => pht('Jump to next inline comment.'),
+        'Jump to previous inline comment.' =>
+          pht('Jump to previous inline comment.'),
+        'Jump to the table of contents.' =>
+          pht('Jump to the table of contents.'),
       ),
     ));
 

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -720,8 +720,6 @@ final class DiffusionCommitController extends DiffusionController {
     $request = $this->getRequest();
     $viewer = $request->getUser();
 
-    Javelin::initBehavior('differential-keyboard-navigation');
-
     // TODO: This is pretty awkward, unify the CSS between Diffusion and
     // Differential better.
     require_celerity_resource('differential-core-view-css');

webroot/rsrc/js/application/diff/DiffChangeset.js

@@ -318,6 +318,88 @@ JX.install('DiffChangeset', {
       return this._highlight;
     },
 
+    getSelectableItems: function() {
+      var items = [];
+
+      items.push({
+        type: 'file',
+        changeset: this,
+        target: this,
+        nodes: {
+          begin: this._node,
+          end: null
+        }
+      });
+
+      var rows = JX.DOM.scry(this._node, 'tr');
+
+      var blocks = [];
+      var block;
+      var ii;
+      for (ii = 0; ii < rows.length; ii++) {
+        var type = this._getRowType(rows[ii]);
+
+        if (!block || (block.type !== type)) {
+          block = {
+            type: type,
+            items: []
+          };
+          blocks.push(block);
+        }
+
+        block.items.push(rows[ii]);
+      }
+
+      for (ii = 0; ii < blocks.length; ii++) {
+        block = blocks[ii];
+
+        if (block.type == 'change') {
+          items.push({
+            type: block.type,
+            changeset: this,
+            target: block.items[0],
+            nodes: {
+              begin: block.items[0],
+              end: block.items[block.items.length - 1]
+            }
+          });
+        }
+
+        if (block.type == 'comment') {
+          for (var jj = 0; jj < block.items.length; jj++) {
+            items.push({
+              type: block.type,
+              changeset: this,
+              target: block.items[jj],
+              nodes: {
+                begin: block.items[jj],
+                end: block.items[jj]
+              }
+            });
+          }
+        }
+      }
+
+      return items;
+    },
+
+    _getRowType: function(row) {
+      // NOTE: Don't do "className.indexOf()" elsewhere. This is evil legacy
+      // magic.
+
+      if (row.className.indexOf('inline') !== -1) {
+        return 'comment';
+      }
+
+      var cells = JX.DOM.scry(row, 'td');
+      for (var ii = 0; ii < cells.length; ii++) {
+        if (cells[ii].className.indexOf('old') !== -1 ||
+            cells[ii].className.indexOf('new') !== -1) {
+          return 'change';
+        }
+      }
+    },
+
     _getNodeData: function() {
       return JX.Stratcom.getData(this._node);
     },

webroot/rsrc/js/application/diff/DiffChangesetList.js

@@ -55,15 +55,49 @@ JX.install('DiffChangesetList', {
   },
 
   members: {
+    _initialized: false,
     _asleep: true,
     _changesets: null,
 
+    _cursorItem: null,
+    _lastKeyboardManager: null,
+
     sleep: function() {
       this._asleep = true;
     },
 
     wake: function() {
       this._asleep = false;
+
+      if (this._initialized) {
+        return;
+      }
+
+      this._initialized = true;
+      var pht = this.getTranslations();
+
+      var label;
+
+      label = pht('Jump to next change.');
+      this._installJumpKey('j', label, 1);
+
+      label = pht('Jump to previous change.');
+      this._installJumpKey('k', label, -1);
+
+      label = pht('Jump to next file.');
+      this._installJumpKey('J', label, 1, 'file');
+
+      label = pht('Jump to previous file.');
+      this._installJumpKey('K', label, -1, 'file');
+
+      label = pht('Jump to next inline comment.');
+      this._installJumpKey('n', label, 1, 'comment');
+
+      label = pht('Jump to previous inline comment.');
+      this._installJumpKey('p', label, -1, 'comment');
+
+      label = pht('Jump to the table of contents.');
+      this._installKey('t', label, this._ontoc);
     },
 
     isAsleep: function() {
@@ -132,6 +166,134 @@ JX.install('DiffChangesetList', {
       }
     },
 
+    _installKey: function(key, label, handler) {
+      handler = JX.bind(this, this._ifawake, handler);
+
+      return new JX.KeyboardShortcut(key, label)
+        .setHandler(handler)
+        .register();
+    },
+
+    _installJumpKey: function(key, label, delta, filter) {
+      filter = filter || null;
+      var handler = JX.bind(this, this._onjumpkey, delta, filter);
+      return this._installKey(key, label, handler);
+    },
+
+    _ontoc: function(manager) {
+      var toc = JX.$('toc');
+      manager.scrollTo(toc);
+    },
+
+    _onjumpkey: function(delta, filter, manager) {
+      var state = this._getSelectionState();
+
+      var cursor = state.cursor;
+      var items = state.items;
+
+      // If there's currently no selection and the user tries to go back,
+      // don't do anything.
+      if ((cursor === null) && (delta < 0)) {
+        return;
+      }
+
+      while (true) {
+        if (cursor === null) {
+          cursor = 0;
+        } else {
+          cursor = cursor + delta;
+        }
+
+        // If we've gone backward past the first change, bail out.
+        if (cursor < 0) {
+          return;
+        }
+
+        // If we've gone forward off the end of the list, bail out.
+        if (cursor >= items.length) {
+          return;
+        }
+
+        // If we're selecting things of a particular type (like only files)
+        // and the next item isn't of that type, move past it.
+        if (filter !== null) {
+          if (items[cursor].type !== filter) {
+            continue;
+          }
+        }
+
+        // Otherwise, we've found a valid item to select.
+        break;
+      }
+
+      this._setSelectionState(items[cursor], manager);
+    },
+
+    _getSelectionState: function() {
+      var items = this._getSelectableItems();
+
+      var cursor = null;
+      if (this._cursorItem !== null) {
+        for (var ii = 0; ii < items.length; ii++) {
+          var item = items[ii];
+          if (this._cursorItem.target === item.target) {
+            cursor = ii;
+            break;
+          }
+        }
+      }
+
+      return {
+        cursor: cursor,
+        items: items
+      };
+    },
+
+    _setSelectionState: function(item, manager) {
+      this._cursorItem = item;
+
+      this._redrawSelection(manager, true);
+
+      return this;
+    },
+
+    _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);
+        return;
+      }
+
+      manager.focusOn(cursor.nodes.begin, cursor.nodes.end);
+
+      if (scroll) {
+        manager.scrollTo(cursor.nodes.begin);
+      }
+
+      return this;
+    },
+
+    _getSelectableItems: function() {
+      var result = [];
+
+      for (var ii = 0; ii < this._changesets.length; ii++) {
+        var items = this._changesets[ii].getSelectableItems();
+        for (var jj = 0; jj < items.length; jj++) {
+          result.push(items[jj]);
+        }
+      }
+
+      return result;
+    },
+
     _onmore: function(e) {
       e.kill();
 

webroot/rsrc/js/application/differential/behavior-keyboard-nav.js

@@ -1,264 +0,0 @@
-/**
- * @provides javelin-behavior-differential-keyboard-navigation
- * @requires javelin-behavior
- *           javelin-dom
- *           javelin-stratcom
- *           phabricator-keyboard-shortcut
- */
-
-JX.behavior('differential-keyboard-navigation', function(config) {
-
-  var cursor = -1;
-  var changesets;
-
-  var selection_begin = null;
-  var selection_end = null;
-
-  var refreshFocus = function() {};
-
-  function init() {
-    if (changesets) {
-      return;
-    }
-    changesets = JX.DOM.scry(document.body, 'div', 'differential-changeset');
-  }
-
-  function getBlocks(cursor) {
-    // TODO: This might not be terribly fast; we can't currently memoize it
-    // because it can change as ajax requests come in (e.g., content loads).
-
-    var rows = JX.DOM.scry(changesets[cursor], 'tr');
-    var blocks = [[changesets[cursor], changesets[cursor]]];
-    var start = null;
-    var type;
-    var ii;
-
-    // Don't show code blocks inside a collapsed file.
-    var diff = JX.DOM.scry(changesets[cursor], 'table', 'differential-diff');
-    if (diff.length == 1 && JX.Stratcom.getData(diff[0]).hidden) {
-      return blocks;
-    }
-
-    function push() {
-      if (start) {
-        blocks.push([start, rows[ii - 1]]);
-      }
-      start = null;
-    }
-
-    for (ii = 0; ii < rows.length; ii++) {
-      type = getRowType(rows[ii]);
-      if (type == 'comment') {
-        // If we see these types of rows, make a block for each one.
-        push();
-      }
-      if (!type) {
-        push();
-      } else if (type && !start) {
-        start = rows[ii];
-      }
-    }
-    push();
-
-    return blocks;
-  }
-
-  function getRowType(row) {
-    // NOTE: Being somewhat over-general here to allow other types of objects
-    // to be easily focused in the future (inline comments, 'show more..').
-
-    if (row.className.indexOf('inline') !== -1) {
-      return 'comment';
-    }
-
-    if (row.className.indexOf('differential-changeset') !== -1) {
-      return 'file';
-    }
-
-    var cells = JX.DOM.scry(row, 'td');
-
-    for (var ii = 0; ii < cells.length; ii++) {
-      // NOTE: The semantic use of classnames here is for performance; don't
-      // emulate this elsewhere since it's super terrible.
-      if (cells[ii].className.indexOf('old') !== -1 ||
-          cells[ii].className.indexOf('new') !== -1) {
-        return 'change';
-      }
-    }
-
-    return null;
-  }
-
-  function jump(manager, delta, jump_to_type) {
-    init();
-
-    if (cursor < 0) {
-      if (delta < 0) {
-        // If the user goes "back" without a selection, just reject the action.
-        return;
-      } else {
-        cursor = 0;
-      }
-    }
-
-    while (true) {
-      var blocks = getBlocks(cursor);
-      var focus;
-      if (delta < 0) {
-        focus = blocks.length;
-      } else {
-        focus = -1;
-      }
-
-      for (var ii = 0; ii < blocks.length; ii++) {
-        if (blocks[ii][0] == selection_begin) {
-          focus = ii;
-          break;
-        }
-      }
-
-      while (true) {
-        focus += delta;
-
-        if (blocks[focus]) {
-          var row_type = getRowType(blocks[focus][0]);
-          if (jump_to_type && row_type != jump_to_type) {
-            continue;
-          }
-
-          selection_begin = blocks[focus][0];
-          selection_end = blocks[focus][1];
-
-          manager.scrollTo(selection_begin);
-
-          refreshFocus = function() {
-            manager.focusOn(selection_begin, selection_end);
-          };
-
-          refreshFocus();
-
-          return;
-        } else {
-          var adjusted = (cursor + delta);
-          if (adjusted < 0 || adjusted >= changesets.length) {
-            // Stop cursor movement when the user reaches either end.
-            return;
-          }
-          cursor = adjusted;
-
-          // Break the inner loop and go to the next file.
-          break;
-        }
-      }
-    }
-
-  }
-
-  // When inline comments are updated, wipe out our cache of blocks since
-  // comments may have been added or deleted.
-  JX.Stratcom.listen(
-    null,
-    'differential-inline-comment-update',
-    function() {
-      changesets = null;
-    });
-  // Same thing when a file is hidden or shown; don't want to highlight
-  // invisible code.
-  JX.Stratcom.listen(
-    'differential-toggle-file-toggled',
-    null,
-    function() {
-      changesets = null;
-      init();
-      refreshFocus();
-    });
-
-  new JX.KeyboardShortcut('j', 'Jump to next change.')
-    .setHandler(function(manager) {
-      jump(manager, 1);
-    })
-    .register();
-
-  new JX.KeyboardShortcut('k', 'Jump to previous change.')
-    .setHandler(function(manager) {
-      jump(manager, -1);
-    })
-    .register();
-
-  new JX.KeyboardShortcut('J', 'Jump to next file.')
-    .setHandler(function(manager) {
-      jump(manager, 1, 'file');
-    })
-    .register();
-
-  new JX.KeyboardShortcut('K', 'Jump to previous file.')
-    .setHandler(function(manager) {
-      jump(manager, -1, 'file');
-    })
-    .register();
-
-  new JX.KeyboardShortcut('n', 'Jump to next inline comment.')
-    .setHandler(function(manager) {
-      jump(manager, 1, 'comment');
-    })
-    .register();
-
-  new JX.KeyboardShortcut('p', 'Jump to previous inline comment.')
-    .setHandler(function(manager) {
-      jump(manager, -1, 'comment');
-    })
-    .register();
-
-
-  new JX.KeyboardShortcut('t', 'Jump to the table of contents.')
-    .setHandler(function(manager) {
-      var toc = JX.$('toc');
-      manager.scrollTo(toc);
-    })
-    .register();
-
-  new JX.KeyboardShortcut(
-    'h',
-    'Collapse or expand the file display (after jump).')
-    .setHandler(function() {
-      if (!changesets || !changesets[cursor]) {
-        return;
-      }
-      JX.Stratcom.invoke('differential-toggle-file', null, {
-        diff: JX.DOM.scry(changesets[cursor], 'table', 'differential-diff')
-      });
-    })
-    .register();
-
-
-  function inline_op(node, op) {
-    // nothing selected
-    if (!node) {
-      return;
-    }
-    if (!JX.DOM.scry(node, 'a', 'differential-inline-' + op)) {
-      // No link for this operation, e.g. editing a comment you can't edit.
-      return;
-    }
-
-    var data = {
-      node: JX.DOM.find(node, 'div', 'differential-inline-comment'),
-      op: op
-    };
-
-    JX.Stratcom.invoke('differential-inline-action', null, data);
-  }
-
-  new JX.KeyboardShortcut('r', 'Reply to selected inline comment.')
-    .setHandler(function() {
-      inline_op(selection_begin, 'reply');
-    })
-    .register();
-
-  new JX.KeyboardShortcut('e', 'Edit selected inline comment.')
-    .setHandler(function() {
-      inline_op(selection_begin, 'edit');
-    })
-    .register();
-
-});