Refine more Differential review state behaviors

Summary:
Ref T13516.

- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.

Test Plan: Fiddled with all these behaviors.

Maniphest Tasks: T13516

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

resources/celerity/map.php

@@ -11,8 +11,8 @@ return array(
     'conpherence.pkg.js' => '020aebcf',
     'core.pkg.css' => '61b7e380',
     'core.pkg.js' => 'fc49f65b',
-    'differential.pkg.css' => 'cb99cd21',
+    'differential.pkg.css' => '9b5ee013',
-    'differential.pkg.js' => 'ae77bf85',
+    'differential.pkg.js' => '1043ee5a',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -62,7 +62,7 @@ return array(
     'rsrc/css/application/diff/diff-tree-view.css' => 'e2d3e222',
     'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d',
     'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
-    'rsrc/css/application/differential/changeset-view.css' => '489b6995',
+    'rsrc/css/application/differential/changeset-view.css' => '5fb26c90',
     'rsrc/css/application/differential/core.css' => '7300a73e',
     'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
     'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
@@ -155,7 +155,7 @@ return array(
     'rsrc/css/phui/phui-fontkit.css' => '1ec937e5',
     'rsrc/css/phui/phui-form-view.css' => '01b796c0',
     'rsrc/css/phui/phui-form.css' => '1f177cb7',
-    'rsrc/css/phui/phui-formation-view.css' => '82a3b73e',
+    'rsrc/css/phui/phui-formation-view.css' => '9a1eff7e',
     'rsrc/css/phui/phui-head-thing.css' => 'd7f293df',
     'rsrc/css/phui/phui-header-view.css' => '36c86a58',
     'rsrc/css/phui/phui-hovercard.css' => '6ca90fa0',
@@ -378,8 +378,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '27305b60',
+    'rsrc/js/application/diff/DiffChangeset.js' => '9a713ba5',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '62a3a351',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'adf069cd',
     'rsrc/js/application/diff/DiffInline.js' => '16e97ebc',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -559,7 +559,7 @@ return array(
     'conpherence-transaction-css' => '3a3f5e7e',
     'd3' => '9d068042',
     'diff-tree-view-css' => 'e2d3e222',
-    'differential-changeset-view-css' => '489b6995',
+    'differential-changeset-view-css' => '5fb26c90',
     'differential-core-view-css' => '7300a73e',
     'differential-revision-add-comment-css' => '7e5900d9',
     'differential-revision-comment-css' => '7dbc8d1d',
@@ -775,8 +775,8 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '27305b60',
+    'phabricator-diff-changeset' => '9a713ba5',
-    'phabricator-diff-changeset-list' => '62a3a351',
+    'phabricator-diff-changeset-list' => 'adf069cd',
     'phabricator-diff-inline' => '16e97ebc',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -846,7 +846,7 @@ return array(
     'phui-fontkit-css' => '1ec937e5',
     'phui-form-css' => '1f177cb7',
     'phui-form-view-css' => '01b796c0',
-    'phui-formation-view-css' => '82a3b73e',
+    'phui-formation-view-css' => '9a1eff7e',
     'phui-head-thing-view-css' => 'd7f293df',
     'phui-header-view-css' => '36c86a58',
     'phui-hovercard' => '074f0783',
@@ -1146,18 +1146,6 @@ return array(
       'javelin-json',
       'phabricator-prefab',
     ),
-    '27305b60' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-    ),
     '289bf236' => array(
       'javelin-install',
       'javelin-util',
@@ -1334,9 +1322,6 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
-    '489b6995' => array(
-      'phui-inline-comment-view-css',
-    ),
     '48fe33d0' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1518,14 +1503,12 @@ return array(
     '5faf27b9' => array(
       'phuix-form-control-view',
     ),
+    '5fb26c90' => array(
+      'phui-inline-comment-view-css',
+    ),
     '60cd9241' => array(
       'javelin-behavior',
     ),
-    '62a3a351' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     '65bb0011' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1814,6 +1797,19 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
+    '9a713ba5' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+    ),
     '9aae2b66' => array(
       'javelin-install',
       'javelin-util',
@@ -1939,6 +1935,11 @@ return array(
       'javelin-typeahead-ondemand-source',
       'javelin-util',
     ),
+    'adf069cd' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'aec8e38c' => array(
       'javelin-dom',
       'javelin-util',

src/applications/differential/engine/DifferentialFileTreeEngine.php

@@ -77,7 +77,18 @@ final class DifferentialFileTreeEngine
           ->setHref('#'));
     $flank_view->setHead($head_view);
 
-    $tail_view = id(new PHUIListView())
+    $tail_view = id(new PHUIListView());
+
+    if ($viewer->isLoggedIn()) {
+      $tail_view->addMenuItem(
+        id(new PHUIListItemView())
+          ->setIcon('fa-comment-o')
+          ->setName(pht('Add Comment'))
+          ->setKeyCommand('x')
+          ->setHref('#'));
+    }
+
+    $tail_view
       ->addMenuItem(
         id(new PHUIListItemView())
           ->setIcon('fa-chevron-left')

src/applications/differential/view/DifferentialChangesetDetailView.php

@@ -270,9 +270,11 @@ final class DifferentialChangesetDetailView extends AphrontView {
           ->setNavigationMarker(true)
           ->render(),
         $buttons,
-        phutil_tag('h1',
+        javelin_tag(
+          'h1',
           array(
             'class' => 'differential-file-icon-header',
+            'sigil' => 'changeset-header',
           ),
           array(
             $icon,

src/applications/differential/view/DifferentialChangesetListView.php

@@ -261,9 +261,8 @@ final class DifferentialChangesetListView extends AphrontView {
         'Open in Editor' => pht('Open in Editor'),
         'Show All Context' => pht('Show All Context'),
         'All Context Shown' => pht('All Context Shown'),
-        "Can't Toggle Unloaded File" => pht("Can't Toggle Unloaded File"),
         'Expand File' => pht('Expand File'),
-        'Collapse File' => pht('Collapse File'),
+        'Hide Changeset' => pht('Hide Changeset'),
         'Show Path in Repository' => pht('Show Path in Repository'),
         'Show Directory in Repository' => pht('Show Directory in Repository'),
         'View Standalone' => pht('View Standalone'),
@@ -318,12 +317,8 @@ final class DifferentialChangesetListView extends AphrontView {
         'Jump to previous inline comment, including collapsed comments.' =>
           pht('Jump to previous inline comment, including collapsed comments.'),
 
-        'This file content has been collapsed.' =>
+        'Hide or show the current changeset.' =>
-          pht('This file content has been collapsed.'),
+          pht('Hide or show the current changeset.'),
-        'Show Content' => pht('Show Content'),
-
-        'Hide or show the current file.' =>
-          pht('Hide or show the current file.'),
         'You must select a file to hide or show.' =>
           pht('You must select a file to hide or show.'),
 
@@ -365,6 +360,11 @@ final class DifferentialChangesetListView extends AphrontView {
           pht('Show path in repository.'),
         'Show directory in repository.' =>
           pht('Show directory in repository.'),
+
+        'Jump to the comment area.' =>
+          pht('Jump to the comment area.'),
+
+        'Show Changeset' => pht('Show Changeset'),
       ),
     ));
 
@@ -439,7 +439,6 @@ final class DifferentialChangesetListView extends AphrontView {
       ->setHref(idx($meta, 'detailURI', '#'))
       ->setMetadata($meta)
       ->addSigil('differential-view-options');
-
   }
 
   private function appendDefaultQueryParams(PhutilURI $uri, array $params) {

src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php

@@ -270,10 +270,14 @@ final class PhabricatorApplicationTransactionCommentView
 
     $badge_view = $this->renderBadgeView();
 
+    $anchor = id(new PhabricatorAnchorView())
+      ->setAnchorName('reply');
+
     $comment_box = id(new PHUIObjectBoxView())
       ->setFlush(true)
       ->addClass('phui-comment-form-view')
       ->addSigil('phui-comment-form')
+      ->appendChild($anchor)
       ->appendChild(
         phutil_tag(
           'h3',

webroot/rsrc/css/application/differential/changeset-view.css

@@ -5,8 +5,6 @@
 
 .differential-changeset {
   position: relative;
-  margin: 0;
-  padding-top: 16px;
   overflow-x: auto;
 
   /* Fixes what seems to be a layout bug in Firefox which causes scrollbars,
@@ -312,14 +310,16 @@ td.cov-I {
   border-top: none;
 }
 
-.differential-changeset h1 {
+.differential-changeset .differential-file-icon-header {
   font-size: {$biggestfontsize};
-  padding: 2px 0 20px 12px;
+  padding: 18px 0 20px 12px;
+  margin-top: 4px;
   line-height: 20px;
   color: {$blacktext};
+  cursor: pointer;
 }
 
-.device-phone .differential-changeset h1 {
+.device-phone .differential-changeset .differential-file-icon-header {
   word-break: break-word;
   margin-right: 8px;
 }
@@ -343,19 +343,6 @@ td.cov-I {
   text-align: center;
 }
 
-.differential-collapse-undo {
-  color: {$darkbluetext};
-  padding: 12px;
-  border: 1px solid {$blue};
-  text-align: center;
-  background-color: {$lightblue};
-  margin: 8px;
-}
-
-.differential-collapse-undo a {
-  font-weight: bold;
-}
-
 .differential-file-icon-header .phui-icon-view {
   display: inline-block;
   margin: 0 6px 2px 0;
@@ -369,6 +356,7 @@ td.cov-I {
 
 .differential-changeset-buttons {
   float: right;
+  margin-top: 16px;
   margin-right: 12px;
 }
 
@@ -484,3 +472,13 @@ unselectable. */
   -webkit-user-select: none;
   user-select: none;
 }
+
+.changeset-content-hidden .differential-file-icon-header {
+  background: {$lightgreybackground};
+  color: {$greytext};
+}
+
+.changeset-selected .differential-file-icon-header {
+  background: {$lightyellow};
+  color: {$blacktext};
+}

webroot/rsrc/css/phui/phui-formation-view.css

@@ -181,3 +181,13 @@
   padding: 0;
   color: {$lightgreytext};
 }
+
+.phui-flank-view-head .phui-list-view {
+  box-shadow: 0 1px 1px rgba(0, 0, 0, 0.1);
+  padding-bottom: 4px;
+}
+
+.phui-flank-view-tail .phui-list-view {
+  box-shadow: 0 -1px 1px rgba(0, 0, 0, 0.1);
+  padding-top: 4px;
+}

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

@@ -10,6 +10,7 @@
  *           javelin-vector
  *           phabricator-diff-inline
  *           phabricator-diff-path-view
+ *           phuix-button-view
  * @javelin
  */
 
@@ -48,6 +49,9 @@ JX.install('DiffChangeset', {
     if (data.changesetState) {
       this._loadChangesetState(data.changesetState);
     }
+
+    var onselect = JX.bind(this, this._onClickHeader);
+    JX.DOM.listen(this._node, 'mousedown', 'changeset-header', onselect);
   },
 
   members: {
@@ -70,7 +74,6 @@ JX.install('DiffChangeset', {
     _inlines: null,
     _visible: true,
 
-    _undoNode: null,
     _displayPath: null,
 
     _changesetList: null,
@@ -88,6 +91,8 @@ JX.install('DiffChangeset', {
     _isLowImportance: null,
     _isOwned: null,
     _isHidden: null,
+    _isSelected: false,
+    _viewMenu: null,
 
     getEditorURI: function() {
       return this._editorURI;
@@ -118,6 +123,11 @@ JX.install('DiffChangeset', {
       return this;
     },
 
+    setViewMenu: function(menu) {
+      this._viewMenu = menu;
+      return this;
+    },
+
     getIcon: function() {
       if (!this._visible) {
         return 'fa-file-o';
@@ -871,6 +881,16 @@ JX.install('DiffChangeset', {
       JX.DOM.alterClass(node, 'diff-tree-path-inlines-completed', is_completed);
     },
 
+    _onClickHeader: function(e) {
+      e.prevent();
+
+      if (this._isSelected) {
+        this.getChangesetList().selectChangeset(null);
+      } else {
+        this.select(false);
+      }
+    },
+
     toggleVisibility: function() {
       this.setVisible(!this._visible);
 
@@ -888,57 +908,87 @@ JX.install('DiffChangeset', {
     setVisible: function(visible) {
       this._visible = visible;
 
-      var diff = JX.DOM.find(this._node, 'table', 'differential-diff');
+      var diff = this._getDiffNode();
-      var undo = this._getUndoNode();
+      var options = this._getViewButtonNode();
+      var show = this._getShowButtonNode();
 
       if (this._visible) {
         JX.DOM.show(diff);
-        JX.DOM.remove(undo);
+        JX.DOM.show(options);
+        JX.DOM.hide(show);
       } else {
         JX.DOM.hide(diff);
-        JX.DOM.appendContent(diff.parentNode, undo);
+        JX.DOM.hide(options);
+        JX.DOM.show(show);
+
+        if (this._viewMenu) {
+          this._viewMenu.close();
+        }
       }
 
       JX.Stratcom.invoke('resize');
 
+      var node = this._node;
+      JX.DOM.alterClass(node, 'changeset-content-hidden', !this._visible);
+
       this.getPathView().setIsHidden(!this._visible);
     },
 
+    setIsSelected: function(is_selected) {
+      this._isSelected = !!is_selected;
+
+      var node = this._node;
+      JX.DOM.alterClass(node, 'changeset-selected', this._isSelected);
+
+      return this;
+    },
+
+    _getDiffNode: function() {
+      if (!this._diffNode) {
+        this._diffNode = JX.DOM.find(this._node, 'table', 'differential-diff');
+      }
+      return this._diffNode;
+    },
+
+    _getViewButtonNode: function() {
+      if (!this._viewButtonNode) {
+        this._viewButtonNode = JX.DOM.find(
+          this._node,
+          'a',
+          'differential-view-options');
+      }
+      return this._viewButtonNode;
+    },
+
+    _getShowButtonNode: function() {
+      if (!this._showButtonNode) {
+        var pht = this.getChangesetList().getTranslations();
+
+        var show_button = new JX.PHUIXButtonView()
+          .setIcon('fa-angle-double-down')
+          .setText(pht('Show Changeset'))
+          .setColor('grey');
+
+        var button_node = show_button.getNode();
+        this._getViewButtonNode().parentNode.appendChild(button_node);
+
+        var onshow = JX.bind(this, this._onClickShowButton);
+        JX.DOM.listen(button_node, 'click', null, onshow);
+
+        this._showButtonNode = button_node;
+      }
+      return this._showButtonNode;
+    },
+
+    _onClickShowButton: function(e) {
+      e.prevent();
+      this.setVisible(true);
+    },
+
     isVisible: function() {
       return this._visible;
     },
 
-    _getUndoNode: function() {
-      if (!this._undoNode) {
-        var pht = this.getChangesetList().getTranslations();
-
-        var link_attributes = {
-          href: '#'
-        };
-
-        var undo_link = JX.$N('a', link_attributes, pht('Show Content'));
-
-        var onundo = JX.bind(this, this._onundo);
-        JX.DOM.listen(undo_link, 'click', null, onundo);
-
-        var node_attributes = {
-          className: 'differential-collapse-undo'
-        };
-
-        var node_content = [
-          pht('This file content has been collapsed.'),
-          ' ',
-          undo_link
-        ];
-
-        var undo_node = JX.$N('div', node_attributes, node_content);
-
-        this._undoNode = undo_node;
-      }
-
-      return this._undoNode;
-    },
-
     _onundo: function(e) {
       e.kill();
       this.toggleVisibility();

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

@@ -124,6 +124,7 @@ JX.install('DiffChangesetList', {
     _dropdownMenu: null,
     _menuButton: null,
     _menuItems: null,
+    _selectedChangeset: null,
 
     sleep: function() {
       this._asleep = true;
@@ -165,6 +166,9 @@ JX.install('DiffChangesetList', {
       if (!standalone) {
         label = pht('Jump to the table of contents.');
         this._installKey('t', 'diff-nav', label, this._ontoc);
+
+        label = pht('Jump to the comment area.');
+        this._installKey('x', 'diff-nav', label, this._oncomments);
       }
 
       label = pht('Jump to next change.');
@@ -203,7 +207,7 @@ JX.install('DiffChangesetList', {
       }
 
       if (!standalone) {
-        label = pht('Hide or show the current file.');
+        label = pht('Hide or show the current changeset.');
         this._installKey('h', 'diff-vis', label, this._onkeytogglefile);
       }
 
@@ -329,6 +333,11 @@ JX.install('DiffChangesetList', {
       manager.scrollTo(toc);
     },
 
+    _oncomments: function(manager) {
+      var reply = JX.$('reply');
+      manager.scrollTo(reply);
+    },
+
     getSelectedInline: function() {
       var cursor = this._cursorItem;
 
@@ -703,6 +712,8 @@ JX.install('DiffChangesetList', {
 
       if (cursor !== null) {
         this._setSelectionState(items[cursor], scroll);
+      } else {
+        this._setSelectionState(null, false);
       }
 
       return this;
@@ -731,13 +742,17 @@ JX.install('DiffChangesetList', {
         return;
       }
 
+      var changeset = cursor.changeset;
+
       var tree = this._getTreeView();
-      if (cursor.changeset) {
+      if (changeset) {
         tree.setSelectedPath(cursor.changeset.getPathView());
       } else {
         tree.setSelectedPath(null);
       }
 
+      this._selectChangeset(changeset);
+
       this.setFocus(cursor.nodes.begin, cursor.nodes.end);
 
       if (scroll) {
@@ -1047,8 +1062,9 @@ JX.install('DiffChangesetList', {
 
         visible_item
           .setDisabled(true)
-          .setIcon('fa-expand')
+          .setIcon('fa-eye-slash')
-          .setName(pht('Can\'t Toggle Unloaded File'));
+          .setName(pht('Hide Changeset'));
+
         var diffs = JX.DOM.scry(
           JX.$(data.containerID),
           'table',
@@ -1059,17 +1075,7 @@ JX.install('DiffChangesetList', {
             'More than one node with sigil "differential-diff" was found in "'+
             data.containerID+'."');
         } else if (diffs.length == 1) {
-          var diff = diffs[0];
           visible_item.setDisabled(false);
-          if (!changeset.isVisible()) {
-            visible_item
-              .setName(pht('Expand File'))
-              .setIcon('fa-expand');
-          } else {
-            visible_item
-              .setName(pht('Collapse File'))
-              .setIcon('fa-compress');
-          }
         } else {
           // Do nothing when there is no diff shown in the table. For example,
           // the file is binary.
@@ -1078,6 +1084,7 @@ JX.install('DiffChangesetList', {
       });
 
       data.menu = menu;
+      changeset.setViewMenu(menu);
       menu.open();
     },
 
@@ -1224,6 +1231,7 @@ JX.install('DiffChangesetList', {
       if (!node) {
         var tree = this._getTreeView();
         tree.setSelectedPath(null);
+        this._selectChangeset(null);
       }
 
       this._focusStart = node;
@@ -1231,6 +1239,22 @@ JX.install('DiffChangesetList', {
       this._redrawFocus();
     },
 
+    _selectChangeset: function(changeset) {
+      if (this._selectedChangeset === changeset) {
+        return;
+      }
+
+      if (this._selectedChangeset !== null) {
+        this._selectedChangeset.setIsSelected(false);
+        this._selectedChangeset = null;
+      }
+
+      this._selectedChangeset = changeset;
+      if (this._selectedChangeset !== null) {
+        this._selectedChangeset.setIsSelected(true);
+      }
+    },
+
     _redrawFocus: function() {
       var node = this._focusStart;
       var extended_node = this._focusEnd || node;
@@ -1247,13 +1271,13 @@ JX.install('DiffChangesetList', {
       var s = JX.Vector.getAggregateScrollForNode(node);
       var d = JX.Vector.getDim(node);
 
-      p.add(s).add(d.x + 1, 0).setPos(reticle);
+      p.add(s).add(d.x + 1, 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(0, JX.Vector.getDim(extended_node).y)
-        .add(10, 0)
+        .add(10, -4)
         .setDim(reticle);
 
       JX.DOM.getContentFrame().appendChild(reticle);