Remove "^" (Prev) and "V" (Next) actions on Differential inline comments

Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)

Now that the code is in a better place:

  - Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
  - Click again to deselect it.
  - You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
  - This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.

Also, make "Reply" render more consistently.

Test Plan:
  - Did all that stuff, things seemed to work OK.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12715, T12616

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '8c5f913d',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '58712637',
-    'differential.pkg.js' => 'e486afd0',
+    'differential.pkg.js' => 'bd321b6e',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -391,10 +391,9 @@ return array(
     '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' => '145c34e2',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'd93e34c2',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'f1101e6e',
     'rsrc/js/application/diff/DiffInline.js' => 'bdf6b568',
     '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',
@@ -619,7 +618,6 @@ return array(
     'javelin-behavior-detect-timezone' => '4c193c96',
     'javelin-behavior-device' => 'bb1dd507',
     'javelin-behavior-diff-preview-link' => '051c7832',
-    'javelin-behavior-differential-comment-jump' => '4fdb476d',
     'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
     'javelin-behavior-differential-edit-inline-comments' => '89d11432',
     'javelin-behavior-differential-feedback-preview' => 'b064af76',
@@ -782,7 +780,7 @@ return array(
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
     'phabricator-diff-changeset' => '145c34e2',
-    'phabricator-diff-changeset-list' => 'd93e34c2',
+    'phabricator-diff-changeset-list' => 'f1101e6e',
     'phabricator-diff-inline' => 'bdf6b568',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
@@ -1291,11 +1289,6 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
-    '4fdb476d' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-    ),
     '503e17fd' => array(
       'javelin-install',
       'javelin-typeahead-source',
@@ -2079,9 +2072,6 @@ return array(
       'javelin-util',
       'phabricator-shaped-request',
     ),
-    'd93e34c2' => array(
-      'javelin-install',
-    ),
     'de2e896f' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2184,6 +2174,9 @@ return array(
       'javelin-workflow',
       'javelin-json',
     ),
+    'f1101e6e' => array(
+      'javelin-install',
+    ),
     'f50152ad' => array(
       'phui-timeline-view-css',
     ),
@@ -2431,7 +2424,6 @@ return array(
       'javelin-behavior-differential-edit-inline-comments',
       'javelin-behavior-differential-populate',
       'javelin-behavior-differential-diff-radios',
-      'javelin-behavior-differential-comment-jump',
       'javelin-behavior-aphront-drag-and-drop-textarea',
       'javelin-behavior-phabricator-object-selector',
       'javelin-behavior-repository-crossreference',

resources/celerity/packages.php

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

src/applications/differential/view/DifferentialChangesetListView.php

@@ -270,8 +270,6 @@ final class DifferentialChangesetListView extends AphrontView {
       ),
     ));
 
-    $this->initBehavior('differential-comment-jump', array());
-
     if ($this->inlineURI) {
       Javelin::initBehavior('differential-edit-inline-comments', array(
         'uri' => $this->inlineURI,

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

@@ -190,61 +190,31 @@ final class PHUIDiffInlineCommentDetailView
       }
     }
 
-    $nextprev = null;
-    if (!$this->preview) {
-      $nextprev = new PHUIButtonBarView();
-      $nextprev->setBorderless(true);
-      $nextprev->addClass('inline-button-divider');
-
-
-      $up = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setTooltip(pht('Previous'))
-        ->setIcon('fa-chevron-up')
-        ->addSigil('differential-inline-prev')
-        ->setMustCapture(true);
-
-      $down = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setTooltip(pht('Next'))
-        ->setIcon('fa-chevron-down')
-        ->addSigil('differential-inline-next')
-        ->setMustCapture(true);
-
-      if ($this->canHide()) {
-        $hide = id(new PHUIButtonView())
-          ->setTag('a')
-          ->setTooltip(pht('Hide Comment'))
-          ->setIcon('fa-times')
-          ->addSigil('hide-inline')
-          ->setMustCapture(true);
-
-        $nextprev->addButton($hide);
-      }
-
-      $nextprev->addButton($up);
-      $nextprev->addButton($down);
-
-      $action_buttons = array();
-      if ($this->allowReply) {
-        if (!$is_synthetic) {
-          // NOTE: No product reason why you can't reply to these, but the reply
-          // mechanism currently sends the inline comment ID to the server, not
-          // file/line information, and synthetic comments don't have an inline
-          // comment ID.
-
-          $action_buttons[] = id(new PHUIButtonView())
-            ->setTag('a')
-            ->setIcon('fa-reply')
-            ->setTooltip(pht('Reply'))
-            ->addSigil('differential-inline-reply')
-            ->setMustCapture(true);
-        }
-      }
-    }
-
     $anchor_name = $this->getAnchorName();
 
+    $action_buttons = array();
+
+    $can_reply =
+      (!$this->editable) &&
+      (!$this->preview) &&
+      ($this->allowReply) &&
+
+      // NOTE: No product reason why you can't reply to synthetic comments,
+      // but the reply mechanism currently sends the inline comment ID to the
+      // server, not file/line information, and synthetic comments don't have
+      // an inline comment ID.
+      (!$is_synthetic);
+
+
+    if ($can_reply) {
+      $action_buttons[] = id(new PHUIButtonView())
+        ->setTag('a')
+        ->setIcon('fa-reply')
+        ->setTooltip(pht('Reply'))
+        ->addSigil('differential-inline-reply')
+        ->setMustCapture(true);
+    }
+
     if ($this->editable && !$this->preview) {
       $action_buttons[] = id(new PHUIButtonView())
         ->setTag('a')
@@ -280,6 +250,15 @@ final class PHUIDiffInlineCommentDetailView
         ->setMustCapture(true);
     }
 
+    if (!$this->preview && $this->canHide()) {
+      $action_buttons[] = id(new PHUIButtonView())
+        ->setTag('a')
+        ->setTooltip(pht('Hide Comment'))
+        ->setIcon('fa-times')
+        ->addSigil('hide-inline')
+        ->setMustCapture(true);
+    }
+
     $done_button = null;
 
     if (!$is_synthetic) {
@@ -430,7 +409,6 @@ final class PHUIDiffInlineCommentDetailView
         $done_button,
         $links,
         $actions,
-        $nextprev,
       ));
 
     $markup = javelin_tag(
@@ -441,10 +419,16 @@ final class PHUIDiffInlineCommentDetailView
         'meta'  => $metadata,
       ),
       array(
-        phutil_tag_div('differential-inline-comment-head grouped', array(
-          $group_left,
-          $group_right,
-        )),
+        javelin_tag(
+          'div',
+          array(
+            'class' => 'differential-inline-comment-head grouped',
+            'sigil' => 'differential-inline-header',
+          ),
+          array(
+            $group_left,
+            $group_right,
+          )),
         phutil_tag_div(
           'differential-inline-comment-content',
           phutil_tag_div('phabricator-remarkup', $content)),

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

@@ -50,6 +50,12 @@ JX.install('DiffChangesetList', {
 
     var onresize = JX.bind(this, this._ifawake, this._onresize);
     JX.Stratcom.listen('resize', null, onresize);
+
+    var onselect = JX.bind(this, this._ifawake, this._onselect);
+    JX.Stratcom.listen(
+      'mousedown',
+      ['differential-inline-comment', 'differential-inline-header'],
+      onselect);
   },
 
   properties: {
@@ -665,6 +671,47 @@ JX.install('DiffChangesetList', {
       this._redrawFocus();
     },
 
+    _onselect: function(e) {
+      // If the user clicked some element inside the header, like an action
+      // icon, ignore the event. They have to click the header element itself.
+      if (e.getTarget() !== e.getNode('differential-inline-header')) {
+        return;
+      }
+
+      var inline = this._getInlineForEvent(e);
+      if (!inline) {
+        return;
+      }
+
+      // The user definitely clicked an inline, so we're going to handle the
+      // event.
+      e.kill();
+
+      var selection = this._getSelectionState();
+      var item;
+
+      // If the comment the user clicked is currently selected, deselect it.
+      // This makes it easy to undo things if you clicked by mistake.
+      if (selection.cursor !== null) {
+        item = selection.items[selection.cursor];
+        if (item.target === inline) {
+          this._setSelectionState(null);
+          return;
+        }
+      }
+
+      // Otherwise, select the item that the user clicked. This makes it
+      // easier to resume keyboard operations after using the mouse to do
+      // something else.
+      var items = selection.items;
+      for (var ii = 0; ii < items.length; ii++) {
+        item = items[ii];
+        if (item.target === inline) {
+          this._setSelectionState(item);
+        }
+      }
+    },
+
     _onaction: function(action, e) {
       e.kill();
 

webroot/rsrc/js/application/differential/behavior-comment-jump.js

@@ -1,32 +0,0 @@
-/**
- * @provides javelin-behavior-differential-comment-jump
- * @requires javelin-behavior
- *           javelin-stratcom
- *           javelin-dom
- */
-
-JX.behavior('differential-comment-jump', function() {
-  function handle_jump(offset) {
-    return function(e) {
-      var parent = JX.$('differential-review-stage');
-      var clicked = e.getNode('differential-inline-comment');
-      var inlines = JX.DOM.scry(parent, 'div', 'differential-inline-comment');
-      var jumpto = null;
-
-      for (var ii = 0; ii < inlines.length; ii++) {
-        if (inlines[ii] == clicked) {
-          jumpto = inlines[(ii + offset + inlines.length) % inlines.length];
-          break;
-        }
-      }
-      JX.Stratcom.invoke('differential-toggle-file-request', null, {
-        element: jumpto
-      });
-      JX.DOM.scrollTo(jumpto);
-      e.kill();
-    };
-  }
-
-  JX.Stratcom.listen('click', 'differential-inline-prev', handle_jump(-1));
-  JX.Stratcom.listen('click', 'differential-inline-next', handle_jump(+1));
-});