Store inline comment offset information and show it when highlighting comments

Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.

When hovering the comment, highlight the original range.

Test Plan: {F7480926, size=full}

Maniphest Tasks: T13513

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

resources/celerity/map.php

@@ -12,8 +12,8 @@ return array(
     'core.pkg.css' => 'a560707d',
     'core.pkg.js' => '0efaf0ac',
     'dark-console.pkg.js' => '187792c2',
-    'differential.pkg.css' => 'd71d4531',
+    'differential.pkg.css' => 'b042ee8b',
-    'differential.pkg.js' => 'ac6914bb',
+    'differential.pkg.js' => '4b2b5659',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -63,7 +63,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' => 'a5cc67cf',
+    'rsrc/css/application/differential/changeset-view.css' => 'df3afa61',
     'rsrc/css/application/differential/core.css' => '7300a73e',
     'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
     'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
@@ -379,9 +379,9 @@ 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' => '20715b98',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'b6bb0240',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '9d5b137e',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '2347e0a6',
-    'rsrc/js/application/diff/DiffInline.js' => '6227a0e3',
+    'rsrc/js/application/diff/DiffInline.js' => '417b3cdb',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -559,7 +559,7 @@ return array(
     'conpherence-transaction-css' => '3a3f5e7e',
     'd3' => '9d068042',
     'diff-tree-view-css' => 'e2d3e222',
-    'differential-changeset-view-css' => 'a5cc67cf',
+    'differential-changeset-view-css' => 'df3afa61',
     'differential-core-view-css' => '7300a73e',
     'differential-revision-add-comment-css' => '7e5900d9',
     'differential-revision-comment-css' => '7dbc8d1d',
@@ -774,9 +774,9 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '20715b98',
+    'phabricator-diff-changeset' => 'b6bb0240',
-    'phabricator-diff-changeset-list' => '9d5b137e',
+    'phabricator-diff-changeset-list' => '2347e0a6',
-    'phabricator-diff-inline' => '6227a0e3',
+    'phabricator-diff-inline' => '417b3cdb',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1082,19 +1082,6 @@ return array(
       'javelin-behavior',
       'javelin-request',
     ),
-    '20715b98' => 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',
-    ),
     '225bbb98' => array(
       'javelin-install',
       'javelin-reactor',
@@ -1111,6 +1098,11 @@ return array(
       'javelin-request',
       'javelin-typeahead-source',
     ),
+    '2347e0a6' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     23631304 => array(
       'phui-fontkit-css',
     ),
@@ -1259,6 +1251,9 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
+    '417b3cdb' => array(
+      'javelin-dom',
+    ),
     '42c44e8b' => array(
       'javelin-behavior',
       'javelin-workflow',
@@ -1503,9 +1498,6 @@ return array(
     '60cd9241' => array(
       'javelin-behavior',
     ),
-    '6227a0e3' => array(
-      'javelin-dom',
-    ),
     '6337cf26' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1816,11 +1808,6 @@ return array(
       'javelin-uri',
       'phabricator-textareautils',
     ),
-    '9d5b137e' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     '9f081f05' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1865,9 +1852,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    'a5cc67cf' => array(
-      'phui-inline-comment-view-css',
-    ),
     'a77e2cbd' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1999,6 +1983,19 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
+    'b6bb0240' => 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',
+    ),
     'b7b73831' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2124,6 +2121,9 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    'df3afa61' => array(
+      'phui-inline-comment-view-css',
+    ),
     'e150bd50' => array(
       'javelin-behavior',
       'javelin-stratcom',

src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php

@@ -208,6 +208,8 @@ final class CelerityDefaultPostprocessor
       'gentle.highlight' => '#fdf3da',
       'gentle.highlight.border' => '#c9b8a8',
 
+      'highlight.bright' => '#fdf320',
+
       'paste.content' => '#fffef5',
       'paste.border' => '#e9dbcd',
       'paste.highlight' => '#fdf3da',

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -320,7 +320,9 @@ abstract class PhabricatorInlineCommentController
           ->setLineLength($length)
           ->setContent((string)$this->getCommentText())
           ->setReplyToCommentPHID($this->getReplyToCommentPHID())
-          ->setIsEditing(true);
+          ->setIsEditing(true)
+          ->setStartOffset($request->getInt('startOffset'))
+          ->setEndOffset($request->getInt('endOffset'));
 
         $document_engine_key = $request->getStr('documentEngineKey');
         if ($document_engine_key !== null) {

src/infrastructure/diff/interface/PhabricatorInlineComment.php

@@ -222,6 +222,24 @@ abstract class PhabricatorInlineComment
     return $this->getStorageObject()->getAttribute('documentEngineKey');
   }
 
+  public function setStartOffset($offset) {
+    $this->getStorageObject()->setAttribute('startOffset', $offset);
+    return $this;
+  }
+
+  public function getStartOffset() {
+    return $this->getStorageObject()->getAttribute('startOffset');
+  }
+
+  public function setEndOffset($offset) {
+    $this->getStorageObject()->setAttribute('endOffset', $offset);
+    return $this;
+  }
+
+  public function getEndOffset() {
+    return $this->getStorageObject()->getAttribute('endOffset');
+  }
+
   public function getDateModified() {
     return $this->getStorageObject()->getDateModified();
   }

src/infrastructure/diff/view/PHUIDiffInlineCommentView.php

@@ -91,6 +91,8 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
       'isDraftDone' => $is_draft_done,
       'isEditing' => $inline->getIsEditing(),
       'documentEngineKey' => $inline->getDocumentEngineKey(),
+      'startOffset' => $inline->getStartOffset(),
+      'endOffset' => $inline->getEndOffset(),
 
       'on_right' => $this->getIsOnRight(),
     );

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

@@ -487,3 +487,27 @@ unselectable. */
   background: {$lightyellow};
   color: {$blacktext};
 }
+
+.differential-diff tr td.inline-hover {
+  background: {$gentle.highlight};
+}
+
+.differential-diff tr td.inline-hover-bright {
+  background: {$highlight.bright};
+}
+
+.inline-hover-container {
+  position: absolute;
+  color: {$lightgreytext};
+  background: {$lightyellow};
+}
+
+.inline-hover-text {
+  padding-top: 2px;
+  padding-bottom: 2px;
+}
+
+.inline-hover-text-bright {
+  color: {$blacktext};
+  background: {$highlight.bright};
+}

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

@@ -711,7 +711,7 @@ JX.install('DiffChangeset', {
       return data.inline;
     },
 
-    newInlineForRange: function(origin, target) {
+    newInlineForRange: function(origin, target, options) {
       var list = this.getChangesetList();
 
       var src = list.getLineNumberFromHeader(origin);
@@ -742,6 +742,8 @@ JX.install('DiffChangeset', {
         isNewFile: is_new
       };
 
+      JX.copy(data, options || {});
+
       var inline = new JX.DiffInline()
         .setChangeset(this)
         .bindToRange(data);

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

@@ -439,8 +439,13 @@ JX.install('DiffChangesetList', {
 
       this._setSourceSelection(null, null);
 
+      var config = {
+        startOffset: start.offset,
+        endOffset: end.offset
+      };
+
       var changeset = start.changeset;
-      changeset.newInlineForRange(start.targetNode, end.targetNode);
+      changeset.newInlineForRange(start.targetNode, end.targetNode, config);
     },
 
     _onkeydone: function() {
@@ -1241,6 +1246,10 @@ JX.install('DiffChangesetList', {
     },
 
     _setHoverInline: function(inline) {
+      if (inline && (this._hoverInline === inline)) {
+        return;
+      }
+
       this._hoverInline = inline;
 
       if (inline) {
@@ -1305,13 +1314,30 @@ JX.install('DiffChangesetList', {
     },
 
     _redrawHover: function() {
-      var reticle = this._getHoverNode();
+      var ii;
-      if (!this._hoverOrigin || this.isAsleep()) {
+
-        JX.DOM.remove(reticle);
+      var map = this._hoverMap;
-        return;
+      if (map) {
+        for (ii = 0; ii < map.length; ii++) {
+          JX.DOM.alterClass(map[ii].cellNode, 'inline-hover', false);
+
+          if (map[ii].bright) {
+            JX.DOM.alterClass(map[ii].cellNode, 'inline-hover-bright', false);
+          }
+
+          if (map[ii].hoverNode) {
+            JX.DOM.remove(map[ii].hoverNode);
+          }
+        }
+        this._hoverMap = null;
       }
 
-      JX.DOM.getContentFrame().appendChild(reticle);
+      var reticle = this._getHoverNode();
+      JX.DOM.remove(reticle);
+
+      if (!this._hoverOrigin || this.isAsleep()) {
+        return;
+      }
 
       var top = this._hoverOrigin;
       var bot = this._hoverTarget;
@@ -1325,7 +1351,7 @@ JX.install('DiffChangesetList', {
       // next sibling with a "data-copy-mode" attribute, which is a marker
       // for the cell with actual content in it.
       var content_cell = top;
-      while (content_cell && !content_cell.getAttribute('data-copy-mode')) {
+      while (content_cell && !this._isContentCell(content_cell)) {
         content_cell = content_cell.nextSibling;
       }
 
@@ -1334,22 +1360,184 @@ JX.install('DiffChangesetList', {
         return;
       }
 
-      var pos = JX.$V(content_cell)
+      var inline = this._hoverInline;
-        .add(JX.Vector.getAggregateScrollForNode(content_cell));
+      if (!inline) {
+        var pos = JX.$V(content_cell)
+          .add(JX.Vector.getAggregateScrollForNode(content_cell));
 
-      var dim = JX.$V(content_cell)
+        var dim = JX.$V(content_cell)
-        .add(JX.Vector.getAggregateScrollForNode(content_cell))
+          .add(JX.Vector.getAggregateScrollForNode(content_cell))
-        .add(-pos.x, -pos.y)
+          .add(-pos.x, -pos.y)
-        .add(JX.Vector.getDim(content_cell));
+          .add(JX.Vector.getDim(content_cell));
 
-      var bpos = JX.$V(bot)
+        var bpos = JX.$V(bot)
-        .add(JX.Vector.getAggregateScrollForNode(bot));
+          .add(JX.Vector.getAggregateScrollForNode(bot));
-      dim.y = (bpos.y - pos.y) + JX.Vector.getDim(bot).y;
+        dim.y = (bpos.y - pos.y) + JX.Vector.getDim(bot).y;
 
-      pos.setPos(reticle);
+        pos.setPos(reticle);
-      dim.setDim(reticle);
+        dim.setDim(reticle);
 
-      JX.DOM.show(reticle);
+        JX.DOM.getContentFrame().appendChild(reticle);
+        JX.DOM.show(reticle);
+
+        return;
+      }
+
+      if (!inline.hoverMap) {
+        inline.hoverMap = this._newHoverMap(top, bot, content_cell, inline);
+      }
+
+      map = inline.hoverMap;
+      for (ii = 0; ii < map.length; ii++) {
+        JX.DOM.alterClass(map[ii].cellNode, 'inline-hover', true);
+        if (map[ii].bright) {
+          JX.DOM.alterClass(map[ii].cellNode, 'inline-hover-bright', true);
+        }
+        if (map[ii].hoverNode) {
+          map[ii].cellNode.insertBefore(
+            map[ii].hoverNode,
+            map[ii].cellNode.firstChild);
+        }
+      }
+      this._hoverMap = map;
+    },
+
+    _newHoverMap: function(top, bot, content_cell, inline) {
+      var start = inline.getStartOffset() || 0;
+      var end = inline.getEndOffset() || 0;
+
+      var head_row = JX.DOM.findAbove(top, 'tr');
+      var last_row = JX.DOM.findAbove(bot, 'tr');
+
+      var cursor = head_row;
+      var rows = [];
+      var idx = null;
+      var ii;
+      do {
+        for (ii = 0; ii < cursor.childNodes.length; ii++) {
+          var child = cursor.childNodes[ii];
+          if (!JX.DOM.isType(child, 'td')) {
+            continue;
+          }
+
+          if (child === content_cell) {
+            idx = ii;
+          }
+
+          if (ii === idx) {
+            if (!this._isContentCell(child)) {
+              break;
+            }
+            rows.push({
+              cellNode: child
+            });
+          }
+        }
+
+        if (cursor === last_row) {
+          break;
+        }
+
+        cursor = cursor.nextSibling;
+      } while (cursor);
+
+      var info;
+      var content;
+      for (ii = 0; ii < rows.length; ii++) {
+        info = this._getSelectionOffset(rows[ii].cellNode, null);
+
+        content = info.content;
+        content = content.replace(/\n+$/, '');
+
+        rows[ii].content = content;
+      }
+
+      var attr_dull = {
+        className: 'inline-hover-text'
+      };
+
+      var attr_bright = {
+        className: 'inline-hover-text inline-hover-text-bright'
+      };
+
+      var attr_container = {
+        className: 'inline-hover-container'
+      };
+
+      var min = 0;
+      var max = rows.length - 1;
+      var offset_min;
+      var offset_max;
+      var len;
+      var node;
+      var text;
+      var any_highlight = false;
+      for (ii = 0; ii < rows.length; ii++) {
+        content = rows[ii].content;
+        len = content.length;
+
+        if (ii === min) {
+          offset_min = start;
+        } else {
+          offset_min = 0;
+        }
+
+        if (ii === max) {
+          offset_max = Math.min(end, len);
+        } else {
+          offset_max = len;
+        }
+
+        var has_min = (offset_min > 0);
+        var has_max = (offset_max < len);
+
+        if (has_min || has_max) {
+          any_highlight = true;
+        }
+
+        rows[ii].min = offset_min;
+        rows[ii].max = offset_max;
+        rows[ii].hasMin = has_min;
+        rows[ii].hasMax = has_max;
+      }
+
+      for (ii = 0; ii < rows.length; ii++) {
+        content = rows[ii].content;
+        offset_min = rows[ii].min;
+        offset_max = rows[ii].max;
+
+        var has_highlight = (rows[ii].hasMin || rows[ii].hasMax);
+
+        if (any_highlight) {
+          var parts = [];
+
+          if (offset_min > 0) {
+            text = content.substring(0, offset_min);
+            node = JX.$N('span', attr_dull, text);
+            parts.push(node);
+          }
+
+          if (len) {
+            text = content.substring(offset_min, offset_max);
+            node = JX.$N('span', attr_bright, text);
+            parts.push(node);
+          }
+
+          if (offset_max < len) {
+            text = content.substring(offset_max, len);
+            node = JX.$N('span', attr_dull, text);
+            parts.push(node);
+          }
+
+          rows[ii].hoverNode = JX.$N('div', attr_container, parts);
+        } else {
+          rows[ii].hoverNode = null;
+        }
+
+        rows[ii].bright = (any_highlight && !has_highlight);
+      }
+
+      return rows;
     },
 
     _getHoverNode: function() {
@@ -2461,19 +2649,13 @@ JX.install('DiffChangesetList', {
           }
         }
 
-        var seen = 0;
+        var info = this._getSelectionOffset(td, fragment);
-        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 (info.found) {
+          offset = info.offset;
+        } else {
           if (is_end) {
-            offset = seen;
+            offset = info.offset;
           } else {
             offset = 0;
           }
@@ -2500,6 +2682,42 @@ JX.install('DiffChangesetList', {
       };
     },
 
+    _getSelectionOffset: function(node, target) {
+      if (!node.childNodes || !node.childNodes.length) {
+        return {
+          offset: node.textContent.length,
+          content: node.textContent,
+          found: false
+        };
+      }
+
+      var found = false;
+      var offset = 0;
+      var content = '';
+      for (var ii = 0; ii < node.childNodes.length; ii++) {
+        var child = node.childNodes[ii];
+
+        if (child === target) {
+          found = true;
+        }
+
+        var spec = this._getSelectionOffset(child, target);
+
+        content += spec.content;
+        if (!found) {
+          offset += spec.offset;
+        }
+
+        found = found || spec.found;
+      }
+
+      return {
+        offset: offset,
+        content: content,
+        found: found
+      };
+    },
+
     _getSelectedRanges: function() {
       var ranges = [];
 
@@ -2518,7 +2736,12 @@ JX.install('DiffChangesetList', {
       }
 
       return ranges;
+    },
+
+    _isContentCell: function(node) {
+      return !!node.getAttribute('data-copy-mode');
     }
+
   }
 
 });

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

@@ -48,6 +48,9 @@ JX.install('DiffInline', {
     _skipFocus: false,
     _menu: null,
 
+    _startOffset: null,
+    _endOffset: null,
+
     bindToRow: function(row) {
       this._row = row;
 
@@ -93,6 +96,8 @@ JX.install('DiffInline', {
       this._snippet = data.snippet;
       this._menuItems = data.menuItems;
       this._documentEngineKey = data.documentEngineKey;
+      this._startOffset = data.startOffset;
+      this._endOffset = data.endOffset;
 
       this._isEditing = data.isEditing;
 
@@ -147,6 +152,14 @@ JX.install('DiffInline', {
       return this._isGhost;
     },
 
+    getStartOffset: function() {
+      return this._startOffset;
+    },
+
+    getEndOffset: function() {
+      return this._endOffset;
+    },
+
     bindToRange: function(data) {
       this._displaySide = data.displaySide;
       this._number = parseInt(data.number, 10);
@@ -154,6 +167,8 @@ JX.install('DiffInline', {
       this._isNewFile = data.isNewFile;
       this._changesetID = data.changesetID;
       this._isNew = true;
+      this._startOffset = data.startOffset;
+      this._endOffset = data.endOffset;
 
       // Insert the comment after any other comments which already appear on
       // the same row.
@@ -506,19 +521,35 @@ JX.install('DiffInline', {
     },
 
     _newRequestData: function(operation, text) {
-      return {
+      var data = {
         op: operation,
-        id: this._id,
+        is_new: this.isNewFile(),
         on_right: ((this.getDisplaySide() == 'right') ? 1 : 0),
         renderer: this.getChangeset().getRendererKey(),
-        number: this.getLineNumber(),
+        text: text || null
-        length: this.getLineLength(),
-        is_new: this.isNewFile(),
-        changesetID: this.getChangesetID(),
-        replyToCommentPHID: this.getReplyToCommentPHID(),
-        text: text || null,
-        documentEngineKey: this._documentEngineKey,
       };
+
+      if (operation === 'new') {
+        var create_data = {
+          changesetID: this.getChangesetID(),
+          documentEngineKey: this._documentEngineKey,
+          replyToCommentPHID: this.getReplyToCommentPHID(),
+          startOffset: this._startOffset,
+          endOffset: this._endOffset,
+          number: this.getLineNumber(),
+          length: this.getLineLength()
+        };
+
+        JX.copy(data, create_data);
+      } else {
+        var edit_data = {
+          id: this._id
+        };
+
+        JX.copy(data, edit_data);
+      }
+
+      return data;
     },
 
     _oneditresponse: function(response) {