Allow users to create inline comments by directly selecting text directly

Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.

Test Plan:
  - Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
  - Caveats: no unified support, doesn't work across lines in Firefox.

Maniphest Tasks: T13513

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

resources/celerity/map.php

@@ -10,10 +10,10 @@ return array(
     'conpherence.pkg.css' => '0e3cf785',
     'conpherence.pkg.js' => '020aebcf',
     'core.pkg.css' => 'a560707d',
-    'core.pkg.js' => '1e667bcb',
+    'core.pkg.js' => '0efaf0ac',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'd71d4531',
-    'differential.pkg.js' => 'b3e29cb8',
+    'differential.pkg.js' => '06a7949c',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -380,7 +380,7 @@ 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' => '40d6c41c',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '4a3639a1',
     'rsrc/js/application/diff/DiffInline.js' => '6227a0e3',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -488,7 +488,7 @@ return array(
     'rsrc/js/core/behavior-linked-container.js' => '74446546',
     'rsrc/js/core/behavior-more.js' => '506aa3f4',
     'rsrc/js/core/behavior-object-selector.js' => '98ef467f',
-    'rsrc/js/core/behavior-oncopy.js' => 'ff7b3f22',
+    'rsrc/js/core/behavior-oncopy.js' => 'b475aae5',
     'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '54262396',
     'rsrc/js/core/behavior-read-only-warning.js' => 'b9109f8f',
     'rsrc/js/core/behavior-redirect.js' => '407ee861',
@@ -523,7 +523,7 @@ return array(
     'rsrc/js/phuix/PHUIXActionView.js' => 'a8f573a9',
     'rsrc/js/phuix/PHUIXAutocomplete.js' => '2fbe234d',
     'rsrc/js/phuix/PHUIXButtonView.js' => '55a24e84',
-    'rsrc/js/phuix/PHUIXDropdownMenu.js' => '7acfd98b',
+    'rsrc/js/phuix/PHUIXDropdownMenu.js' => 'b557770a',
     'rsrc/js/phuix/PHUIXExample.js' => 'c2c500a7',
     'rsrc/js/phuix/PHUIXFormControl.js' => '38c1f3fb',
     'rsrc/js/phuix/PHUIXFormationColumnView.js' => '4bcc1f78',
@@ -648,7 +648,7 @@ return array(
     'javelin-behavior-phabricator-line-linker' => '590e6527',
     'javelin-behavior-phabricator-notification-example' => '29819b75',
     'javelin-behavior-phabricator-object-selector' => '98ef467f',
-    'javelin-behavior-phabricator-oncopy' => 'ff7b3f22',
+    'javelin-behavior-phabricator-oncopy' => 'b475aae5',
     'javelin-behavior-phabricator-remarkup-assist' => '54262396',
     'javelin-behavior-phabricator-reveal-content' => 'b105a3a6',
     'javelin-behavior-phabricator-search-typeahead' => '1cb7d027',
@@ -775,7 +775,7 @@ return array(
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '20715b98',
-    'phabricator-diff-changeset-list' => '40d6c41c',
+    'phabricator-diff-changeset-list' => '4a3639a1',
     'phabricator-diff-inline' => '6227a0e3',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -886,7 +886,7 @@ return array(
     'phuix-action-view' => 'a8f573a9',
     'phuix-autocomplete' => '2fbe234d',
     'phuix-button-view' => '55a24e84',
-    'phuix-dropdown-menu' => '7acfd98b',
+    'phuix-dropdown-menu' => 'b557770a',
     'phuix-form-control-view' => '38c1f3fb',
     'phuix-formation-column-view' => '4bcc1f78',
     'phuix-formation-flank-view' => '6648270a',
@@ -1259,11 +1259,6 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
-    '40d6c41c' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     '42c44e8b' => array(
       'javelin-behavior',
       'javelin-workflow',
@@ -1332,6 +1327,11 @@ return array(
     '490e2e2e' => array(
       'phui-oi-list-view-css',
     ),
+    '4a3639a1' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     '4a7fb02b' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1614,13 +1614,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    '7acfd98b' => array(
-      'javelin-install',
-      'javelin-util',
-      'javelin-dom',
-      'javelin-vector',
-      'javelin-stratcom',
-    ),
     '7ad020a5' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1975,6 +1968,10 @@ return array(
       'javelin-workboard-card-template',
       'javelin-workboard-order-template',
     ),
+    'b475aae5' => array(
+      'javelin-behavior',
+      'javelin-dom',
+    ),
     'b49fd60c' => array(
       'multirow-row-manager',
       'trigger-rule',
@@ -1982,6 +1979,13 @@ return array(
     'b517bfa0' => array(
       'phui-oi-list-view-css',
     ),
+    'b557770a' => array(
+      'javelin-install',
+      'javelin-util',
+      'javelin-dom',
+      'javelin-vector',
+      'javelin-stratcom',
+    ),
     'b58d1a2a' => array(
       'javelin-behavior',
       'javelin-behavior-device',
@@ -2203,10 +2207,6 @@ return array(
       'owners-path-editor',
       'javelin-behavior',
     ),
-    'ff7b3f22' => array(
-      'javelin-behavior',
-      'javelin-dom',
-    ),
   ),
   'packages' => array(
     'conpherence.pkg.css' => array(

src/applications/differential/view/DifferentialChangesetListView.php

@@ -365,6 +365,14 @@ final class DifferentialChangesetListView extends AphrontView {
           pht('Jump to the comment area.'),
 
         'Show Changeset' => pht('Show Changeset'),
+
+        'You must select source text to create a new inline comment.' =>
+          pht('You must select source text to create a new inline comment.'),
+
+        'New Inline Comment' => pht('New Inline Comment'),
+
+        'Add new inline comment on selected source text.' =>
+          pht('Add new inline comment on selected source text.'),
       ),
     ));
 

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

@@ -59,6 +59,10 @@ JX.install('DiffChangesetList', {
       null,
       onrangeup);
 
+    var onrange = JX.bind(this, this._ifawake, this._onSelectRange);
+    JX.enableDispatch(window, 'selectionchange');
+    JX.Stratcom.listen('selectionchange', null, onrange);
+
     this._setupInlineCommentListeners();
   },
 
@@ -194,6 +198,10 @@ JX.install('DiffChangesetList', {
       this._installKey('R', 'inline', label,
         JX.bind(this, this._onkeyreply, true));
 
+      label = pht('Add new inline comment on selected source text.');
+      this._installKey('c', 'inline', label,
+        JX.bind(this, this._onKeyCreate));
+
       label = pht('Edit selected inline comment.');
       this._installKey('e', 'inline', label, this._onkeyedit);
 
@@ -417,6 +425,24 @@ JX.install('DiffChangesetList', {
       this._warnUser(pht('You must select a comment to edit.'));
     },
 
+    _onKeyCreate: function() {
+      var start = this._sourceSelectionStart;
+      var end = this._sourceSelectionEnd;
+
+      if (!this._sourceSelectionStart) {
+        var pht = this.getTranslations();
+        this._warnUser(
+          pht(
+            'You must select source text to create a new inline comment.'));
+        return;
+      }
+
+      this._setSourceSelection(null, null);
+
+      var changeset = start.changeset;
+      changeset.newInlineForRange(start.targetNode, end.targetNode);
+    },
+
     _onkeydone: function() {
       var cursor = this._cursorItem;
 
@@ -2197,8 +2223,278 @@ JX.install('DiffChangesetList', {
           inline.activateMenu(node, e);
           break;
       }
-    }
+    },
 
+    _onSelectRange: function(e) {
+      this._updateSourceSelection();
+    },
+
+    _updateSourceSelection: function() {
+      var ranges = this._getSelectedRanges();
+
+      // If we have zero or more than one range, don't do anything.
+      if (ranges.length === 1) {
+        for (var ii = 0; ii < ranges.length; ii++) {
+          var range = ranges[ii];
+
+          var head = range.startContainer;
+          var last = range.endContainer;
+
+          var head_loc = this._getFragmentLocation(head);
+          var last_loc = this._getFragmentLocation(last);
+
+          if (head_loc === null || last_loc === null) {
+            break;
+          }
+
+          if (head_loc.changesetID !== last_loc.changesetID) {
+            break;
+          }
+
+          head_loc.offset += range.startOffset;
+          last_loc.offset += range.endOffset;
+
+          this._setSourceSelection(head_loc, last_loc);
+          return;
+        }
+      }
+
+      this._setSourceSelection(null, null);
+    },
+
+    _setSourceSelection: function(start, end) {
+      var start_updated =
+        !this._isSameSourceSelection(this._sourceSelectionStart, start);
+
+      var end_updated =
+        !this._isSameSourceSelection(this._sourceSelectionEnd, end);
+
+      if (!start_updated && !end_updated) {
+        return;
+      }
+
+      this._sourceSelectionStart = start;
+      this._sourceSelectionEnd = end;
+
+      if (!start) {
+        this._closeSourceSelectionMenu();
+        return;
+      }
+
+      var menu;
+      if (this._sourceSelectionMenu) {
+        menu = this._sourceSelectionMenu;
+      } else {
+        menu = this._newSourceSelectionMenu();
+        this._sourceSelectionMenu = menu;
+      }
+
+      var pos = JX.$V(start.node)
+        .add(0, -menu.getMenuNodeDimensions().y)
+        .add(0, -24);
+
+      menu.setPosition(pos);
+      menu.open();
+    },
+
+    _newSourceSelectionMenu: function() {
+      var pht = this.getTranslations();
+
+      var menu = new JX.PHUIXDropdownMenu(null)
+        .setWidth(240);
+
+      // We need to disable autofocus for this menu, since it operates on the
+      // text selection in the document. If we leave this enabled, opening the
+      // menu immediately discards the selection.
+      menu.setDisableAutofocus(true);
+
+      var list = new JX.PHUIXActionListView();
+      menu.setContent(list.getNode());
+
+      var oncreate = JX.bind(this, this._onSourceSelectionMenuAction, 'create');
+
+      var comment_item = new JX.PHUIXActionView()
+        .setIcon('fa-comment-o')
+        .setName(pht('New Inline Comment'))
+        .setKeyCommand('c')
+        .setHandler(oncreate);
+
+      list.addItem(comment_item);
+
+      return menu;
+    },
+
+    _onSourceSelectionMenuAction: function(action, e) {
+      e.kill();
+      this._closeSourceSelectionMenu();
+
+      switch (action) {
+        case 'create':
+          this._onKeyCreate();
+          break;
+      }
+    },
+
+    _closeSourceSelectionMenu: function() {
+      if (this._sourceSelectionMenu) {
+        this._sourceSelectionMenu.close();
+      }
+    },
+
+    _isSameSourceSelection: function(u, v) {
+      if (u === null && v === null) {
+        return true;
+      }
+
+      if (u === null && v !== null) {
+        return false;
+      }
+
+      if (u !== null && v === null) {
+        return false;
+      }
+
+      return (
+        (u.changesetID === v.changesetID) &&
+        (u.line === v.line) &&
+        (u.displayColumn === v.displayColumn) &&
+        (u.offset === v.offset)
+      );
+    },
+
+    _getFragmentLocation: function(fragment) {
+      // Find the changeset containing the fragment.
+      var changeset = null;
+      try {
+        var node = JX.DOM.findAbove(
+          fragment,
+          'div',
+          'differential-changeset');
+
+        changeset = this.getChangesetForNode(node);
+        if (!changeset) {
+          return null;
+        }
+      } catch (ex) {
+        return null;
+      }
+
+      // Find the line number and display column for the fragment.
+      var line = null;
+      var column_count = -1;
+      var offset = null;
+      var target_node = null;
+      var td;
+      try {
+
+        // NOTE: In Safari, you can carefully select an entire line and then
+        // move your mouse down slightly, causing selection of an empty
+        // document fragment which is an immediate child of the next "<tr />".
+
+        // If the fragment is a direct child of a "<tr />" parent, assume the
+        // user has done this and select the last child of the previous row
+        // instead. It's possible there are other ways to do this, so this may
+        // not always be the right rule.
+
+        // Otherwise, select the containing "<td />".
+
+        var is_end;
+        if (JX.DOM.isType(fragment.parentNode, 'tr')) {
+          // Assume this is Safari, and that the user has carefully selected a
+          // row and then moved their mouse down a few pixels to select the
+          // invisible fragment at the beginning of the next row.
+          var cells = fragment.parentNode.previousSibling.childNodes;
+          td = cells[cells.length - 1];
+          is_end = true;
+        } else {
+          td = JX.DOM.findAbove(fragment, 'td');
+          is_end = false;
+        }
+
+        var cursor = td;
+        while (cursor) {
+          if (cursor.getAttribute('data-copy-mode')) {
+            column_count++;
+          }
+
+          var n = parseInt(cursor.getAttribute('data-n'));
+
+          if (n) {
+            if (line === null) {
+              target_node = cursor;
+              line = n;
+            }
+          }
+
+          cursor = cursor.previousSibling;
+        }
+
+        if (!line) {
+          return null;
+        }
+
+        if (column_count < 0) {
+          return null;
+        }
+
+        var seen = 0;
+        for (var ii = 0; ii < td.childNodes.length; ii++) {
+          var child = td.childNodes[ii];
+          if (child === fragment) {
+            offset = seen;
+            break;
+          }
+          seen += child.textContent.length;
+        }
+
+        if (offset === null) {
+          if (is_end) {
+            offset = seen;
+          } else {
+            offset = 0;
+          }
+        }
+      } catch (ex) {
+        return null;
+      }
+
+      var changeset_id;
+      if (column_count > 0) {
+        changeset_id = changeset.getRightChangesetID();
+      } else {
+        changeset_id = changeset.getLeftChangesetID();
+      }
+
+      return {
+        node: td,
+        changeset: changeset,
+        changesetID: changeset_id,
+        line: line,
+        displayColumn: column_count,
+        offset: offset,
+        targetNode: target_node
+      };
+    },
+
+    _getSelectedRanges: function() {
+      var ranges = [];
+
+      if (!window.getSelection) {
+        return ranges;
+      }
+
+      var selection = window.getSelection();
+      for (var ii = 0; ii < selection.rangeCount; ii++) {
+        var range = selection.getRangeAt(ii);
+        if (range.collapsed) {
+          continue;
+        }
+
+        ranges.push(range);
+      }
+
+      return ranges;
+    }
   }
 
 });

webroot/rsrc/js/core/behavior-oncopy.js

@@ -119,7 +119,7 @@ JX.behavior('phabricator-oncopy', function() {
 
   // When the selection range changes, apply CSS classes if the selection is
   // nonempty. We don't want to make visual changes to the document immediately
-  // when the user press the mouse button, since we aren't yet sure that
+  // when the user presses the mouse button, since we aren't yet sure that
   // they are starting a selection: instead, wait for them to actually select
   // something.
   function onchangeselect() {

webroot/rsrc/js/phuix/PHUIXDropdownMenu.js

@@ -21,11 +21,13 @@ JX.install('PHUIXDropdownMenu', {
   construct : function(node) {
     this._node = node;
 
-    JX.DOM.listen(
+    if (node) {
-      this._node,
+      JX.DOM.listen(
-      'click',
+        this._node,
-      null,
+        'click',
-      JX.bind(this, this._onclick));
+        null,
+        JX.bind(this, this._onclick));
+    }
 
     JX.Stratcom.listen(
       'mousedown',
@@ -54,7 +56,8 @@ JX.install('PHUIXDropdownMenu', {
     width: null,
     align: 'right',
     offsetX: 0,
-    offsetY: 0
+    offsetY: 0,
+    disableAutofocus: false
   },
 
   members: {
@@ -62,6 +65,8 @@ JX.install('PHUIXDropdownMenu', {
     _menu: null,
     _open: false,
     _content: null,
+    _position: null,
+    _visible: false,
 
     setContent: function(content) {
       JX.DOM.setContent(this._getMenuNode(), content);
@@ -94,6 +99,12 @@ JX.install('PHUIXDropdownMenu', {
       return this;
     },
 
+    setPosition: function(pos) {
+      this._position = pos;
+      this._setMenuNodePosition(pos);
+      return this;
+    },
+
     _getMenuNode: function() {
       if (!this._menu) {
         var attrs = {
@@ -161,7 +172,10 @@ JX.install('PHUIXDropdownMenu', {
     },
 
     _show : function() {
-      document.body.appendChild(this._menu);
+      if (!this._visible) {
+        this._visible = true;
+        document.body.appendChild(this._menu);
+      }
 
       if (this.getWidth()) {
         new JX.Vector(this.getWidth(), null).setDim(this._menu);
@@ -169,23 +183,28 @@ JX.install('PHUIXDropdownMenu', {
 
       this._adjustposition();
 
-      JX.DOM.alterClass(this._node, 'phuix-dropdown-open', true);
+      if (this._node) {
-
+        JX.DOM.alterClass(this._node, 'phuix-dropdown-open', true);
-      this._node.setAttribute('aria-expanded', 'true');
+        this._node.setAttribute('aria-expanded', 'true');
+      }
 
       // Try to highlight the first link in the menu for assistive technologies.
-      var links = JX.DOM.scry(this._menu, 'a');
+      if (!this.getDisableAutofocus()) {
-      if (links[0]) {
+        var links = JX.DOM.scry(this._menu, 'a');
-        JX.DOM.focus(links[0]);
+        if (links[0]) {
+          JX.DOM.focus(links[0]);
+        }
       }
     },
 
     _hide : function() {
+      this._visible = false;
       JX.DOM.remove(this._menu);
 
-      JX.DOM.alterClass(this._node, 'phuix-dropdown-open', false);
+      if (this._node) {
-
+        JX.DOM.alterClass(this._node, 'phuix-dropdown-open', false);
-      this._node.setAttribute('aria-expanded', 'false');
+        this._node.setAttribute('aria-expanded', 'false');
+      }
     },
 
     _adjustposition : function() {
@@ -193,6 +212,15 @@ JX.install('PHUIXDropdownMenu', {
         return;
       }
 
+      if (this._position) {
+        this._setMenuNodePosition(this._position);
+        return;
+      }
+
+      if (!this._node) {
+        return;
+      }
+
       var m = JX.Vector.getDim(this._menu);
 
       var v = JX.$V(this._node);
@@ -236,11 +264,28 @@ JX.install('PHUIXDropdownMenu', {
           break;
       }
 
-      v = v.add(this.getOffsetX(), this.getOffsetY());
+      this._setMenuNodePosition(v);
+    },
 
+    _setMenuNodePosition: function(v) {
+      v = v.add(this.getOffsetX(), this.getOffsetY());
       v.setPos(this._menu);
     },
 
+    getMenuNodeDimensions: function() {
+      if (!this._visible) {
+        document.body.appendChild(this._menu);
+      }
+
+      var dim = JX.Vector.getDim(this._menu);
+
+      if (!this._visible) {
+        JX.DOM.remove(this._menu);
+      }
+
+      return dim;
+    },
+
     _onkey: function(e) {
       // When the user presses escape with a menu open, close the menu and
       // refocus the button which activates the menu. In particular, this makes
@@ -255,7 +300,10 @@ JX.install('PHUIXDropdownMenu', {
       }
 
       this.close();
-      JX.DOM.focus(this._node);
+
+      if (this._node) {
+        JX.DOM.focus(this._node);
+      }
 
       e.prevent();
     }