Move inline comment actions into a dropdown menu

Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.

Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).

Maniphest Tasks: T13513, T11401

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '1e667bcb',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'd71d4531',
-    'differential.pkg.js' => '39781f05',
+    'differential.pkg.js' => '21616a78',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -380,8 +380,8 @@ return array(
     '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' => '20715b98',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '564cbd20',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '40d6c41c',
-    'rsrc/js/application/diff/DiffInline.js' => 'a0ef0b54',
+    'rsrc/js/application/diff/DiffInline.js' => '15de2478',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -775,8 +775,8 @@ return array(
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '20715b98',
-    'phabricator-diff-changeset-list' => '564cbd20',
+    'phabricator-diff-changeset-list' => '40d6c41c',
-    'phabricator-diff-inline' => 'a0ef0b54',
+    'phabricator-diff-inline' => '15de2478',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1034,6 +1034,9 @@ return array(
       'javelin-stratcom',
       'javelin-util',
     ),
+    '15de2478' => array(
+      'javelin-dom',
+    ),
     '1a844c06' => array(
       'javelin-install',
       'javelin-util',
@@ -1259,6 +1262,11 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
+    '40d6c41c' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     '42c44e8b' => array(
       'javelin-behavior',
       'javelin-workflow',
@@ -1418,11 +1426,6 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
-    '564cbd20' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     '5793d835' => array(
       'javelin-install',
       'javelin-util',
@@ -1833,9 +1836,6 @@ return array(
       'javelin-util',
       'phabricator-keyboard-shortcut',
     ),
-    'a0ef0b54' => array(
-      'javelin-dom',
-    ),
     'a17b84f1' => array(
       'javelin-behavior',
       'javelin-dom',

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

@@ -88,10 +88,12 @@ final class PHUIDiffInlineCommentDetailView
       $is_synthetic = true;
     }
 
+    $is_preview = $this->preview;
+
     $metadata = $this->getInlineCommentMetadata();
 
     $sigil = 'differential-inline-comment';
-    if ($this->preview) {
+    if ($is_preview) {
       $sigil = $sigil.' differential-inline-comment-preview';
     }
 
@@ -146,11 +148,6 @@ final class PHUIDiffInlineCommentDetailView
       $classes[] = 'inline-comment-ghost';
     }
 
-    // I think this is unused
-    if ($inline->getHasReplies()) {
-      $classes[] = 'inline-comment-has-reply';
-    }
-
     if ($inline->getReplyToCommentPHID()) {
       $classes[] = 'inline-comment-is-reply';
     }
@@ -167,47 +164,22 @@ final class PHUIDiffInlineCommentDetailView
     $anchor_name = $this->getAnchorName();
 
     $action_buttons = array();
+    $menu_items = array();
 
-    $can_reply =
+    if ($this->editable && !$is_preview) {
-      (!$this->editable) &&
+      $menu_items[] = array(
-      (!$this->preview) &&
+        'label' => pht('Edit Comment'),
-      ($this->allowReply) &&
+        'icon' => 'fa-pencil',
+        'action' => 'edit',
+        'key' => 'e',
+      );
 
-      // NOTE: No product reason why you can't reply to synthetic comments,
+      $menu_items[] = array(
-      // but the reply mechanism currently sends the inline comment ID to the
+        'label' => pht('Delete Comment'),
-      // server, not file/line information, and synthetic comments don't have
+        'icon' => 'fa-trash-o',
-      // an inline comment ID.
+        'action' => 'delete',
-      (!$is_synthetic);
+      );
-
+    } else if ($is_preview) {
-
-    if ($can_reply) {
-      $action_buttons[] = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setIcon('fa-reply')
-        ->setTooltip(pht('Reply'))
-        ->addSigil('differential-inline-reply')
-        ->setMustCapture(true)
-        ->setAuralLabel(pht('Reply'));
-    }
-
-    if ($this->editable && !$this->preview) {
-      $action_buttons[] = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setIcon('fa-pencil')
-        ->setTooltip(pht('Edit'))
-        ->addSigil('differential-inline-edit')
-        ->setMustCapture(true)
-        ->setAuralLabel(pht('Edit'));
-
-      $action_buttons[] = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setIcon('fa-trash-o')
-        ->setTooltip(pht('Delete'))
-        ->addSigil('differential-inline-delete')
-        ->setMustCapture(true)
-        ->setAuralLabel(pht('Delete'));
-
-    } else if ($this->preview) {
       $links[] = javelin_tag(
         'a',
         array(
@@ -228,14 +200,40 @@ final class PHUIDiffInlineCommentDetailView
         ->setAuralLabel(pht('Delete'));
     }
 
-    if (!$this->preview && $this->canHide()) {
+    if (!$is_preview && $this->canHide()) {
-      $action_buttons[] = id(new PHUIButtonView())
+      $menu_items[] = array(
-        ->setTag('a')
+        'label' => pht('Collapse'),
-        ->setTooltip(pht('Collapse'))
+        'icon' => 'fa-times',
-        ->setIcon('fa-times')
+        'action' => 'collapse',
-        ->addSigil('hide-inline')
+        'key' => 'q',
-        ->setMustCapture(true)
+      );
-        ->setAuralLabel(pht('Collapse'));
+    }
+
+    $can_reply =
+      (!$this->editable) &&
+      (!$is_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) {
+      $menu_items[] = array(
+        'label' => pht('Reply to Comment'),
+        'icon' => 'fa-reply',
+        'action' => 'reply',
+        'key' => 'r',
+      );
+
+      $menu_items[] = array(
+        'label' => pht('Quote Comment'),
+        'icon' => 'fa-quote-left',
+        'action' => 'quote',
+        'key' => 'R',
+      );
     }
 
     $done_button = null;
@@ -283,7 +281,7 @@ final class PHUIDiffInlineCommentDetailView
         $classes[] = 'inline-state-is-draft';
       }
 
-      if ($mark_done && !$this->preview) {
+      if ($mark_done && !$is_preview) {
         $done_input = javelin_tag(
           'input',
           array(
@@ -327,7 +325,7 @@ final class PHUIDiffInlineCommentDetailView
       $inline,
       PhabricatorInlineComment::MARKUP_FIELD_BODY);
 
-    if ($this->preview) {
+    if ($is_preview) {
       $anchor = null;
     } else {
       $anchor = phutil_tag(
@@ -364,13 +362,24 @@ final class PHUIDiffInlineCommentDetailView
     }
 
     $actions = null;
-    if ($action_buttons) {
+    if ($action_buttons || $menu_items) {
       $actions = new PHUIButtonBarView();
       $actions->setBorderless(true);
       $actions->addClass('inline-button-divider');
       foreach ($action_buttons as $button) {
         $actions->addButton($button);
       }
+
+      if (!$is_preview) {
+        $menu_button = id(new PHUIButtonView())
+          ->setTag('a')
+          ->setColor(PHUIButtonView::GREY)
+          ->setDropdown(true)
+          ->setAuralLabel(pht('Inline Actions'))
+          ->addSigil('inline-action-dropdown');
+
+        $actions->addButton($menu_button);
+      }
     }
 
     $group_left = phutil_tag(
@@ -401,6 +410,8 @@ final class PHUIDiffInlineCommentDetailView
       ->truncateString($inline->getContent());
     $metadata['snippet'] = pht('%s: %s', $author, $snippet);
 
+    $metadata['menuItems'] = $menu_items;
+
     $markup = javelin_tag(
       'div',
       array(

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

@@ -20,9 +20,6 @@ JX.install('DiffChangesetList', {
     var onmenu = JX.bind(this, this._ifawake, this._onmenu);
     JX.Stratcom.listen('click', 'differential-view-options', onmenu);
 
-    var oncollapse = JX.bind(this, this._ifawake, this._oncollapse, true);
-    JX.Stratcom.listen('click', 'hide-inline', oncollapse);
-
     var onexpand = JX.bind(this, this._ifawake, this._oncollapse, false);
     JX.Stratcom.listen('click', 'reveal-inline', onexpand);
 
@@ -336,16 +333,7 @@ JX.install('DiffChangesetList', {
           var inline = cursor.target;
           if (inline.canReply()) {
             this.setFocus(null);
-
+            inline.reply(true);
-            var text;
-            if (is_quote) {
-              text = inline.getRawText();
-              text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n';
-            } else {
-              text = '';
-            }
-
-            inline.reply(text);
             return;
           }
         }
@@ -2094,12 +2082,6 @@ JX.install('DiffChangesetList', {
         'differential-inline-comment-undo',
         onundo);
 
-      var onedit = JX.bind(this, this._onInlineEvent, 'edit');
-      JX.Stratcom.listen(
-        'click',
-        ['differential-inline-comment', 'differential-inline-edit'],
-        onedit);
-
       var ondone = JX.bind(this, this._onInlineEvent, 'done');
       JX.Stratcom.listen(
         'click',
@@ -2112,11 +2094,11 @@ JX.install('DiffChangesetList', {
         ['differential-inline-comment', 'differential-inline-delete'],
         ondelete);
 
-      var onreply = JX.bind(this, this._onInlineEvent, 'reply');
+      var onmenu = JX.bind(this, this._onInlineEvent, 'menu');
       JX.Stratcom.listen(
         'click',
-        ['differential-inline-comment', 'differential-inline-reply'],
+        ['differential-inline-comment', 'inline-action-dropdown'],
-        onreply);
+        onmenu);
 
       var ondraft = JX.bind(this, this._onInlineEvent, 'draft');
       JX.Stratcom.listen(
@@ -2156,7 +2138,7 @@ JX.install('DiffChangesetList', {
         return;
       }
 
-      if (action !== 'draft') {
+      if (action !== 'draft' && action !== 'menu') {
         e.kill();
       }
 
@@ -2201,21 +2183,19 @@ JX.install('DiffChangesetList', {
         case 'undo':
           inline.undo();
           break;
-        case 'edit':
-          inline.edit();
-          break;
         case 'done':
           inline.toggleDone();
           break;
         case 'delete':
           inline.delete(is_ref);
           break;
-        case 'reply':
-          inline.reply();
-          break;
         case 'draft':
           inline.triggerDraft();
           break;
+        case 'menu':
+          var node = e.getNode('inline-action-dropdown');
+          inline.activateMenu(node, e);
+          break;
       }
     }
 

webroot/rsrc/js/application/diff/DiffInline.js

@@ -21,6 +21,7 @@ JX.install('DiffInline', {
     _replyToCommentPHID: null,
     _originalText: null,
     _snippet: null,
+    _menuItems: null,
     _documentEngineKey: null,
 
     _isDeleted: false,
@@ -45,6 +46,7 @@ JX.install('DiffInline', {
 
     _draftRequest: null,
     _skipFocus: false,
+    _menu: null,
 
     bindToRow: function(row) {
       this._row = row;
@@ -89,6 +91,7 @@ JX.install('DiffInline', {
       this._changesetID = data.changesetID;
       this._isNew = false;
       this._snippet = data.snippet;
+      this._menuItems = data.menuItems;
       this._documentEngineKey = data.documentEngineKey;
 
       this._isEditing = data.isEditing;
@@ -252,19 +255,11 @@ JX.install('DiffInline', {
     },
 
     canReply: function() {
-      if (!this._hasAction('reply')) {
+      return this._hasMenuAction('reply');
-        return false;
-      }
-
-      return true;
     },
 
     canEdit: function() {
-      if (!this._hasAction('edit')) {
+      return this._hasMenuAction('edit');
-        return false;
-      }
-
-      return true;
     },
 
     canDone: function() {
@@ -276,22 +271,13 @@ JX.install('DiffInline', {
     },
 
     canCollapse: function() {
-      if (!JX.DOM.scry(this._row, 'a', 'hide-inline').length) {
+      return this._hasMenuAction('collapse');
-        return false;
-      }
-
-      return true;
     },
 
     getRawText: function() {
       return this._originalText;
     },
 
-    _hasAction: function(action) {
-      var nodes = JX.DOM.scry(this._row, 'a', 'differential-inline-' + action);
-      return (nodes.length > 0);
-    },
-
     _newRow: function() {
       var attributes = {
         sigil: 'inline-row'
@@ -312,6 +298,8 @@ JX.install('DiffInline', {
     },
 
     setCollapsed: function(collapsed) {
+      this._closeMenu();
+
       this._isCollapsed = collapsed;
 
       var op;
@@ -393,12 +381,24 @@ JX.install('DiffInline', {
         .send();
     },
 
-    reply: function(text) {
+    reply: function(with_quote) {
+      this._closeMenu();
+
+      var text;
+      if (with_quote) {
+        text = this.getRawText();
+        text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n';
+      } else {
+        text = '';
+      }
+
       var changeset = this.getChangeset();
       return changeset.newInlineReply(this, text);
     },
 
     edit: function(text, skip_focus) {
+      this._closeMenu();
+
       this._skipFocus = !!skip_focus;
 
       // If you edit an inline ("A"), modify the text ("AB"), cancel, and then
@@ -881,6 +881,101 @@ JX.install('DiffInline', {
       if (this._draftRequest) {
         this._draftRequest.trigger();
       }
+    },
+
+    activateMenu: function(button, e) {
+      // If we already have a menu for this button, let the menu handle the
+      // event.
+      var data = JX.Stratcom.getData(button);
+      if (data.menu) {
+        return;
+      }
+
+      e.prevent();
+
+      var menu = new JX.PHUIXDropdownMenu(button)
+        .setWidth(240);
+
+      var list = new JX.PHUIXActionListView();
+      var items = this._newMenuItems(menu);
+      for (var ii = 0; ii < items.length; ii++) {
+        list.addItem(items[ii]);
+      }
+
+      menu.setContent(list.getNode());
+
+      data.menu = menu;
+      this._menu = menu;
+
+      menu.listen('open', JX.bind(this, function() {
+        var changeset_list = this.getChangeset().getChangesetList();
+        changeset_list.selectInline(this, true);
+      }));
+
+      menu.open();
+    },
+
+    _newMenuItems: function(menu) {
+      var items = [];
+
+      for (var ii = 0; ii < this._menuItems.length; ii++) {
+        var spec = this._menuItems[ii];
+
+        var onmenu = JX.bind(this, this._onMenuItem, menu, spec.action);
+
+        var item = new JX.PHUIXActionView()
+          .setIcon(spec.icon)
+          .setName(spec.label)
+          .setHandler(onmenu);
+
+        if (spec.key) {
+          item.setKeyCommand(spec.key);
+        }
+
+        items.push(item);
+      }
+
+      return items;
+    },
+
+    _onMenuItem: function(menu, action, e) {
+      e.prevent();
+      menu.close();
+
+      switch (action) {
+        case 'reply':
+          this.reply();
+          break;
+        case 'quote':
+          this.reply(true);
+          break;
+        case 'collapse':
+          this.setCollapsed(true);
+          break;
+        case 'delete':
+          this.delete();
+          break;
+        case 'edit':
+          this.edit();
+          break;
+      }
+
+    },
+
+    _hasMenuAction: function(action) {
+      for (var ii = 0; ii < this._menuItems.length; ii++) {
+        var spec = this._menuItems[ii];
+        if (spec.action === action) {
+          return true;
+        }
+      }
+      return false;
+    },
+
+    _closeMenu: function() {
+      if (this._menu) {
+        this._menu.close();
+      }
     }
 
   }