Add more keyboard navigation options for inline comments

Summary:
  - Use n/p to jump between comments.
  - Use r to reply to the selected comment.
  - Use e to edit the selected comment.

Test Plan: Verified n, p, r, e, j, k, J, K, "click edit" and "click reply"
behavior in as many weird cases as I could come up with.

Reviewers: btrahan, jungejason, nh, cpiro, jl

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T583

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

src/__celerity_resource_map__.php

@@ -330,6 +330,17 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/js/javelin/lib/behavior.js',
   ),
+  0 =>
+  array(
+    'uri' => '/res/b6096fdd/rsrc/js/javelin/lib/__tests__/URI.js',
+    'type' => 'js',
+    'requires' =>
+    array(
+      0 => 'javelin-uri',
+      1 => 'javelin-php-serializer',
+    ),
+    'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js',
+  ),
   'javelin-behavior-aphront-basic-tokenizer' =>
   array(
     'uri' => '/res/9be30797/rsrc/js/application/core/behavior-tokenizer.js',
@@ -461,7 +472,7 @@ celerity_register_resource_map(array(
   ),
   'javelin-behavior-differential-edit-inline-comments' =>
   array(
-    'uri' => '/res/b1168479/rsrc/js/application/differential/behavior-edit-inline-comments.js',
+    'uri' => '/res/c24338ce/rsrc/js/application/differential/behavior-edit-inline-comments.js',
     'type' => 'js',
     'requires' =>
     array(
@@ -491,13 +502,14 @@ celerity_register_resource_map(array(
   ),
   'javelin-behavior-differential-keyboard-navigation' =>
   array(
-    'uri' => '/res/e36415a2/rsrc/js/application/differential/behavior-keyboard-nav.js',
+    'uri' => '/res/f5ce5987/rsrc/js/application/differential/behavior-keyboard-nav.js',
     'type' => 'js',
     'requires' =>
     array(
       0 => 'javelin-behavior',
       1 => 'javelin-dom',
-      2 => 'phabricator-keyboard-shortcut',
+      2 => 'javelin-stratcom',
+      3 => 'phabricator-keyboard-shortcut',
     ),
     'disk' => '/rsrc/js/application/differential/behavior-keyboard-nav.js',
   ),
@@ -1023,17 +1035,6 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/js/javelin/lib/control/tokenizer/Tokenizer.js',
   ),
-  0 =>
-  array(
-    'uri' => '/res/b6096fdd/rsrc/js/javelin/lib/__tests__/URI.js',
-    'type' => 'js',
-    'requires' =>
-    array(
-      0 => 'javelin-uri',
-      1 => 'javelin-php-serializer',
-    ),
-    'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js',
-  ),
   'javelin-typeahead' =>
   array(
     'uri' => '/res/7dea2b98/rsrc/js/javelin/lib/control/typeahead/Typeahead.js',
@@ -1664,6 +1665,30 @@ celerity_register_resource_map(array(
       'uri' => '/res/pkg/03ef179e/diffusion.pkg.css',
       'type' => 'css',
     ),
+    '11a5c52c' =>
+    array(
+      'name' => 'differential.pkg.js',
+      'symbols' =>
+      array(
+        0 => 'phabricator-drag-and-drop-file-upload',
+        1 => 'phabricator-shaped-request',
+        2 => 'javelin-behavior-differential-feedback-preview',
+        3 => 'javelin-behavior-differential-edit-inline-comments',
+        4 => 'javelin-behavior-differential-populate',
+        5 => 'javelin-behavior-differential-show-more',
+        6 => 'javelin-behavior-differential-diff-radios',
+        7 => 'javelin-behavior-differential-accept-with-errors',
+        8 => 'javelin-behavior-differential-comment-jump',
+        9 => 'javelin-behavior-differential-add-reviewers-and-ccs',
+        10 => 'javelin-behavior-differential-keyboard-navigation',
+        11 => 'javelin-behavior-aphront-drag-and-drop',
+        12 => 'javelin-behavior-aphront-drag-and-drop-textarea',
+        13 => 'javelin-behavior-phabricator-object-selector',
+        14 => 'differential-inline-comment-editor',
+      ),
+      'uri' => '/res/pkg/11a5c52c/differential.pkg.js',
+      'type' => 'js',
+    ),
     '4e7acf1a' =>
     array(
       'name' => 'core.pkg.js',
@@ -1722,30 +1747,6 @@ celerity_register_resource_map(array(
       'uri' => '/res/pkg/540effd7/typeahead.pkg.js',
       'type' => 'js',
     ),
-    '882478c9' =>
-    array(
-      'name' => 'differential.pkg.js',
-      'symbols' =>
-      array(
-        0 => 'phabricator-drag-and-drop-file-upload',
-        1 => 'phabricator-shaped-request',
-        2 => 'javelin-behavior-differential-feedback-preview',
-        3 => 'javelin-behavior-differential-edit-inline-comments',
-        4 => 'javelin-behavior-differential-populate',
-        5 => 'javelin-behavior-differential-show-more',
-        6 => 'javelin-behavior-differential-diff-radios',
-        7 => 'javelin-behavior-differential-accept-with-errors',
-        8 => 'javelin-behavior-differential-comment-jump',
-        9 => 'javelin-behavior-differential-add-reviewers-and-ccs',
-        10 => 'javelin-behavior-differential-keyboard-navigation',
-        11 => 'javelin-behavior-aphront-drag-and-drop',
-        12 => 'javelin-behavior-aphront-drag-and-drop-textarea',
-        13 => 'javelin-behavior-phabricator-object-selector',
-        14 => 'differential-inline-comment-editor',
-      ),
-      'uri' => '/res/pkg/882478c9/differential.pkg.js',
-      'type' => 'js',
-    ),
     '8b139246' =>
     array(
       'name' => 'differential.pkg.css',
@@ -1801,7 +1802,7 @@ celerity_register_resource_map(array(
     'aphront-typeahead-control-css' => '4f6e449d',
     'differential-changeset-view-css' => '8b139246',
     'differential-core-view-css' => '8b139246',
-    'differential-inline-comment-editor' => '882478c9',
+    'differential-inline-comment-editor' => '11a5c52c',
     'differential-local-commits-view-css' => '8b139246',
     'differential-revision-add-comment-css' => '8b139246',
     'differential-revision-comment-css' => '8b139246',
@@ -1812,20 +1813,20 @@ celerity_register_resource_map(array(
     'diffusion-commit-view-css' => '03ef179e',
     'javelin-behavior' => 'b164acea',
     'javelin-behavior-aphront-basic-tokenizer' => '540effd7',
-    'javelin-behavior-aphront-drag-and-drop' => '882478c9',
+    'javelin-behavior-aphront-drag-and-drop' => '11a5c52c',
-    'javelin-behavior-aphront-drag-and-drop-textarea' => '882478c9',
+    'javelin-behavior-aphront-drag-and-drop-textarea' => '11a5c52c',
     'javelin-behavior-aphront-form-disable-on-submit' => '4e7acf1a',
-    'javelin-behavior-differential-accept-with-errors' => '882478c9',
+    'javelin-behavior-differential-accept-with-errors' => '11a5c52c',
-    'javelin-behavior-differential-add-reviewers-and-ccs' => '882478c9',
+    'javelin-behavior-differential-add-reviewers-and-ccs' => '11a5c52c',
-    'javelin-behavior-differential-comment-jump' => '882478c9',
+    'javelin-behavior-differential-comment-jump' => '11a5c52c',
-    'javelin-behavior-differential-diff-radios' => '882478c9',
+    'javelin-behavior-differential-diff-radios' => '11a5c52c',
-    'javelin-behavior-differential-edit-inline-comments' => '882478c9',
+    'javelin-behavior-differential-edit-inline-comments' => '11a5c52c',
-    'javelin-behavior-differential-feedback-preview' => '882478c9',
+    'javelin-behavior-differential-feedback-preview' => '11a5c52c',
-    'javelin-behavior-differential-keyboard-navigation' => '882478c9',
+    'javelin-behavior-differential-keyboard-navigation' => '11a5c52c',
-    'javelin-behavior-differential-populate' => '882478c9',
+    'javelin-behavior-differential-populate' => '11a5c52c',
-    'javelin-behavior-differential-show-more' => '882478c9',
+    'javelin-behavior-differential-show-more' => '11a5c52c',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '4e7acf1a',
-    'javelin-behavior-phabricator-object-selector' => '882478c9',
+    'javelin-behavior-phabricator-object-selector' => '11a5c52c',
     'javelin-behavior-phabricator-watch-anchor' => '4e7acf1a',
     'javelin-behavior-refresh-csrf' => '4e7acf1a',
     'javelin-behavior-workflow' => '4e7acf1a',
@@ -1850,12 +1851,12 @@ celerity_register_resource_map(array(
     'phabricator-core-buttons-css' => '4f6e449d',
     'phabricator-core-css' => '4f6e449d',
     'phabricator-directory-css' => '4f6e449d',
-    'phabricator-drag-and-drop-file-upload' => '882478c9',
+    'phabricator-drag-and-drop-file-upload' => '11a5c52c',
     'phabricator-keyboard-shortcut' => '4e7acf1a',
     'phabricator-keyboard-shortcut-manager' => '4e7acf1a',
     'phabricator-object-selector-css' => '8b139246',
     'phabricator-remarkup-css' => '4f6e449d',
-    'phabricator-shaped-request' => '882478c9',
+    'phabricator-shaped-request' => '11a5c52c',
     'phabricator-standard-page-view' => '4f6e449d',
     'syntax-highlighting-css' => '4f6e449d',
   ),

webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js

@@ -183,8 +183,13 @@ JX.behavior('differential-edit-inline-comments', function(config) {
     });
 
   var action_handler = function(op, e) {
-    var data = e.getNodeData('differential-inline-comment');
     var node = e.getNode('differential-inline-comment');
+    handle_inline_action(node, op);
+    e.kill();
+  }
+
+  var handle_inline_action = function(node, op) {
+    var data = JX.Stratcom.getData(node);
     var row  = node.parentNode.parentNode;
 
     var original = data.original;
@@ -203,8 +208,6 @@ JX.behavior('differential-edit-inline-comments', function(config) {
       .setRow(row)
       .setTable(row.parentNode)
       .start();
-
-    e.kill();
   }
 
   for (var op in {'edit' : 1, 'delete' : 1, 'reply' : 1}) {
@@ -214,5 +217,13 @@ JX.behavior('differential-edit-inline-comments', function(config) {
       JX.bind(null, action_handler, op));
   }
 
+  JX.Stratcom.listen(
+    'differential-inline-action',
+    null,
+    function(e) {
+      var data = e.getData();
+      handle_inline_action(data.node, data.op);
+    });
+
 });
 

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

@@ -2,6 +2,7 @@
  * @provides javelin-behavior-differential-keyboard-navigation
  * @requires javelin-behavior
  *           javelin-dom
+ *           javelin-stratcom
  *           phabricator-keyboard-shortcut
  */
 
@@ -28,21 +29,28 @@ JX.behavior('differential-keyboard-navigation', function(config) {
     var blocks = [[changesets[cursor], changesets[cursor]]];
     var start = null;
     var type;
-    for (var ii = 0; ii < rows.length; ii++) {
+    var ii;
-      type = getRowType(rows[ii]);
+
-      if (type == 'ignore') {
+    function push() {
-        continue;
+      if (start) {
-      }
-      if (!type && start) {
         blocks.push([start, rows[ii - 1]]);
-        start = null;
+      }
+      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];
       }
     }
-    if (start) {
+    push();
-      blocks.push([start, rows[ii - 1]]);
-    }
 
     return blocks;
   }
@@ -52,7 +60,7 @@ JX.behavior('differential-keyboard-navigation', function(config) {
     // to be easily focused in the future (inline comments, 'show more..').
 
     if (row.className.indexOf('inline') !== -1) {
-      return 'ignore';
+      return 'comment';
     }
 
     if (row.className.indexOf('differential-changeset') !== -1) {
@@ -73,7 +81,7 @@ JX.behavior('differential-keyboard-navigation', function(config) {
     return null;
   }
 
-  function jump(manager, delta, jump_to_file) {
+  function jump(manager, delta, jump_to_type) {
     init();
 
     if (cursor < 0) {
@@ -106,7 +114,7 @@ JX.behavior('differential-keyboard-navigation', function(config) {
 
         if (blocks[focus]) {
           var row_type = getRowType(blocks[focus][0]);
-          if (jump_to_file && row_type != 'file') {
+          if (jump_to_type && row_type != jump_to_type) {
             continue;
           }
 
@@ -133,6 +141,15 @@ JX.behavior('differential-keyboard-navigation', function(config) {
 
   }
 
+  // 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;
+    });
+
   var is_haunted = false;
   function haunt() {
     is_haunted = !is_haunted;
@@ -154,13 +171,52 @@ JX.behavior('differential-keyboard-navigation', function(config) {
 
   new JX.KeyboardShortcut('J', 'Jump to next file.')
     .setHandler(function(manager) {
-      jump(manager, 1, true);
+      jump(manager, 1, 'file');
     })
     .register();
 
   new JX.KeyboardShortcut('K', 'Jump to previous file.')
     .setHandler(function(manager) {
-      jump(manager, -1, true);
+      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();
+
+
+  function inline_op(node, op) {
+    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(manager) {
+      inline_op(selection_begin, 'reply');
+    })
+    .register();
+
+  new JX.KeyboardShortcut('e', 'Edit selected inline comment.')
+    .setHandler(function(manager) {
+      inline_op(selection_begin, 'edit');
     })
     .register();