Move "hover over an inline to see the affected lines" code to the new class tree

Summary:
Fixes T8047. Ref T12616. Fixes T9270. This moves the "hover" part of the hover/drag behavior to the new code, leaving the "drag" part for a followup change.

The new hover UI behaves properly with Quicksand (T8047) and the filetree (T9270).

Test Plan: Hovered over inlines, saw lines select properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T9270, T8047

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

resources/celerity/map.php

@@ -12,8 +12,8 @@ return array(
     'core.pkg.css' => 'ee5f28cd',
     'core.pkg.js' => '8c5f913d',
     'darkconsole.pkg.js' => '1f9a31bc',
-    'differential.pkg.css' => '4ff77743',
+    'differential.pkg.css' => 'ea471cb0',
-    'differential.pkg.js' => '85543704',
+    'differential.pkg.js' => '7f24021f',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -64,7 +64,7 @@ return array(
     'rsrc/css/application/dashboard/dashboard.css' => 'fe5b1869',
     'rsrc/css/application/diff/inline-comment-summary.css' => '51efda3a',
     'rsrc/css/application/differential/add-comment.css' => 'c47f8c40',
-    'rsrc/css/application/differential/changeset-view.css' => '69a3c268',
+    'rsrc/css/application/differential/changeset-view.css' => '6a77323e',
     'rsrc/css/application/differential/core.css' => '5b7b8ff4',
     'rsrc/css/application/differential/phui-inline-comment.css' => 'ffd1a542',
     'rsrc/css/application/differential/revision-comment.css' => '14b8565a',
@@ -390,13 +390,13 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
     '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' => 'c5742feb',
+    'rsrc/js/application/diff/DiffChangeset.js' => '34e513e2',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'f1101e6e',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'fcbf80e9',
-    'rsrc/js/application/diff/DiffInline.js' => 'a81c29d4',
+    'rsrc/js/application/diff/DiffInline.js' => 'c2e9ff4c',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     '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',
+    'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'd59300f5',
     'rsrc/js/application/differential/behavior-populate.js' => '5e41c819',
     'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
     'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d',
@@ -567,7 +567,7 @@ return array(
     'conpherence-thread-manager' => '4d863052',
     'conpherence-transaction-css' => '85129c68',
     'd3' => 'a11a5ff2',
-    'differential-changeset-view-css' => '69a3c268',
+    'differential-changeset-view-css' => '6a77323e',
     'differential-core-view-css' => '5b7b8ff4',
     'differential-revision-add-comment-css' => 'c47f8c40',
     'differential-revision-comment-css' => '14b8565a',
@@ -619,7 +619,7 @@ return array(
     'javelin-behavior-device' => 'bb1dd507',
     'javelin-behavior-diff-preview-link' => '051c7832',
     'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
-    'javelin-behavior-differential-edit-inline-comments' => '89d11432',
+    'javelin-behavior-differential-edit-inline-comments' => 'd59300f5',
     'javelin-behavior-differential-feedback-preview' => 'b064af76',
     'javelin-behavior-differential-populate' => '5e41c819',
     'javelin-behavior-differential-toggle-files' => 'ca3f91eb',
@@ -779,9 +779,9 @@ return array(
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => 'c5742feb',
+    'phabricator-diff-changeset' => '34e513e2',
-    'phabricator-diff-changeset-list' => 'f1101e6e',
+    'phabricator-diff-changeset-list' => 'fcbf80e9',
-    'phabricator-diff-inline' => 'a81c29d4',
+    'phabricator-diff-inline' => 'c2e9ff4c',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
     'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -1129,6 +1129,17 @@ return array(
       'javelin-dom',
       'javelin-workflow',
     ),
+    '34e513e2' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     '3ab51e2c' => array(
       'javelin-behavior',
       'javelin-behavior-device',
@@ -1398,12 +1409,12 @@ return array(
     '6882e80a' => array(
       'javelin-dom',
     ),
-    '69a3c268' => array(
-      'phui-inline-comment-view-css',
-    ),
     '69adf288' => array(
       'javelin-install',
     ),
+    '6a77323e' => array(
+      'phui-inline-comment-view-css',
+    ),
     '6ad39b6f' => array(
       'javelin-install',
       'javelin-event',
@@ -1564,13 +1575,6 @@ return array(
       'phabricator-draggable-list',
       'javelin-workboard-column',
     ),
-    '89d11432' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-util',
-      'javelin-vector',
-    ),
     '8a41885b' => array(
       'javelin-install',
       'javelin-dom',
@@ -1718,9 +1722,6 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
-    'a81c29d4' => array(
-      'javelin-dom',
-    ),
     'a8beebea' => array(
       'phui-oi-list-view-css',
     ),
@@ -1909,23 +1910,15 @@ return array(
       'javelin-dom',
       'javelin-vector',
     ),
+    'c2e9ff4c' => array(
+      'javelin-dom',
+    ),
     'c420b0b9' => array(
       'javelin-behavior',
       'javelin-behavior-device',
       'javelin-stratcom',
       'phabricator-tooltip',
     ),
-    'c5742feb' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     'c587b80f' => array(
       'javelin-install',
     ),
@@ -2043,6 +2036,13 @@ return array(
       'javelin-dom',
       'javelin-stratcom',
     ),
+    'd59300f5' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-util',
+      'javelin-vector',
+    ),
     'd5a2d665' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2174,9 +2174,6 @@ return array(
       'javelin-workflow',
       'javelin-json',
     ),
-    'f1101e6e' => array(
-      'javelin-install',
-    ),
     'f50152ad' => array(
       'phui-timeline-view-css',
     ),
@@ -2224,6 +2221,9 @@ return array(
       'javelin-dom',
       'phortune-credit-card-form',
     ),
+    'fcbf80e9' => array(
+      'javelin-install',
+    ),
     'fe287620' => array(
       'javelin-install',
       'javelin-dom',

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

@@ -109,10 +109,6 @@
   color: {$darkgreytext};
 }
 
-.differential-diff th.selected {
-  background-color: {$sh-yellowbackground};
-}
-
 .differential-changeset-immutable .differential-diff th {
   cursor: auto;
 }
@@ -308,6 +304,7 @@ td.cov-I {
   top: 0;
   left: 0;
   box-sizing: border-box;
+  pointer-events: none;
 }
 
 .differential-diff .inline > td {

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

@@ -27,6 +27,9 @@ JX.install('DiffChangeset', {
     this._encoding = data.encoding;
     this._loaded = data.loaded;
 
+    this._leftID = data.left;
+    this._rightID = data.right;
+
     this._inlines = [];
   },
 
@@ -48,8 +51,19 @@ JX.install('DiffChangeset', {
     _encoding: null,
     _undoTemplates: null,
 
+    _leftID: null,
+    _rightID: null,
+
     _inlines: null,
 
+    getLeftChangesetID: function() {
+      return this._leftID;
+    },
+
+    getRightChangesetID: function() {
+      return this._rightID;
+    },
+
     /**
      * Has the content of this changeset been loaded?
      *

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

@@ -56,6 +56,12 @@ JX.install('DiffChangesetList', {
       'mousedown',
       ['differential-inline-comment', 'differential-inline-header'],
       onselect);
+
+    var onhover = JX.bind(this, this._ifawake, this._onhover);
+    JX.Stratcom.listen(
+      ['mouseover', 'mouseout'],
+      'differential-inline-comment',
+      onhover);
   },
 
   properties: {
@@ -74,16 +80,24 @@ JX.install('DiffChangesetList', {
     _focusStart: null,
     _focusEnd: null,
 
+    _hoverNode: null,
+    _hoverInline: null,
+    _hoverOrigin: null,
+    _hoverTarget: null,
+
     sleep: function() {
       this._asleep = true;
 
       this._redrawFocus();
+      this._redrawSelection();
+      this.resetHover();
     },
 
     wake: function() {
       this._asleep = false;
 
       this._redrawFocus();
+      this._redrawSelection();
 
       if (this._initialized) {
         return;
@@ -428,6 +442,21 @@ JX.install('DiffChangesetList', {
       return result;
     },
 
+    _onhover: function(e) {
+      if (e.getIsTouchEvent()) {
+        return;
+      }
+
+      var inline;
+      if (e.getType() == 'mouseout') {
+        inline = null;
+      } else {
+        inline = this._getInlineForEvent(e);
+      }
+
+      this._setHoverInline(inline);
+    },
+
     _onmore: function(e) {
       e.kill();
 
@@ -669,6 +698,8 @@ JX.install('DiffChangesetList', {
 
     _onresize: function() {
       this._redrawFocus();
+      this._redrawSelection();
+      this._redrawHover();
     },
 
     _onselect: function(e) {
@@ -769,6 +800,11 @@ JX.install('DiffChangesetList', {
       if (forms.length) {
         JX.DOM.invoke(forms[0], 'shouldRefresh');
       }
+
+      // Clear the mouse hover reticle after a substantive edit: we don't get
+      // a "mouseout" event if the row vanished because of row being removed
+      // after an edit.
+      this.reseHover();
     },
 
     setFocus: function(node, extended_node) {
@@ -812,6 +848,112 @@ JX.install('DiffChangesetList', {
       return this._focusNode;
     },
 
+    _setHoverInline: function(inline) {
+      this._hoverInline = inline;
+
+      if (inline) {
+        var changeset = inline.getChangeset();
+
+        var changeset_id;
+        var side = inline.getDisplaySide();
+        if (side == 'right') {
+          changeset_id = changeset.getRightChangesetID();
+        } else {
+          changeset_id = changeset.getLeftChangesetID();
+        }
+
+        var new_part;
+        if (inline.isNewFile()) {
+          new_part = 'N';
+        } else {
+          new_part = 'O';
+        }
+
+        var prefix = 'C' + changeset_id + new_part + 'L';
+
+        var number = inline.getLineNumber();
+        var length = inline.getLineLength();
+
+        var origin = JX.$(prefix + number);
+        var target = JX.$(prefix + (number + length));
+
+        this._hoverOrigin = origin;
+        this._hoverTarget = target;
+      } else {
+        this._hoverOrigin = null;
+        this._hoverTarget = null;
+      }
+
+      this._redrawHover();
+    },
+
+    resetHover: function() {
+      this._setHoverInline(null);
+    },
+
+    _redrawHover: function() {
+      var reticle = this._getHoverNode();
+      if (!this._hoverOrigin || this.isAsleep()) {
+        JX.DOM.remove(reticle);
+        return;
+      }
+
+      JX.DOM.getContentFrame().appendChild(reticle);
+
+      var top = this._hoverOrigin;
+      var bot = this._hoverTarget;
+      if (JX.$V(top).y > JX.$V(bot).y) {
+        var tmp = top;
+        top = bot;
+        bot = tmp;
+      }
+
+      // Find the leftmost cell that we're going to highlight: this is the next
+      // <td /> in the row. In 2up views, it should be directly adjacent. In
+      // 1up views, we may have to skip over the other line number column.
+      var l = top;
+      while (JX.DOM.isType(l, 'th')) {
+        l = l.nextSibling;
+      }
+
+      // Find the rightmost cell that we're going to highlight: this is the
+      // farthest consecutive, adjacent <td /> in the row. Sometimes the left
+      // and right nodes are the same (left side of 2up view); sometimes we're
+      // going to highlight several nodes (copy + code + coverage).
+      var r = l;
+      while (r.nextSibling && JX.DOM.isType(r.nextSibling, 'td')) {
+        r = r.nextSibling;
+      }
+
+      var pos = JX.$V(l)
+        .add(JX.Vector.getAggregateScrollForNode(l));
+
+      var dim = JX.$V(r)
+        .add(JX.Vector.getAggregateScrollForNode(r))
+        .add(-pos.x, -pos.y)
+        .add(JX.Vector.getDim(r));
+
+      var bpos = JX.$V(bot)
+        .add(JX.Vector.getAggregateScrollForNode(bot));
+      dim.y = (bpos.y - pos.y) + JX.Vector.getDim(bot).y;
+
+      pos.setPos(reticle);
+      dim.setDim(reticle);
+
+      JX.DOM.show(reticle);
+    },
+
+    _getHoverNode: function() {
+      if (!this._hoverNode) {
+        var attributes = {
+          className: 'differential-reticle'
+        };
+        this._hoverNode = JX.$N('div', attributes);
+      }
+
+      return this._hoverNode;
+    },
+
     _deleteInlineByID: function(id) {
       var uri = this.getInlineURI();
       var data = {

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

@@ -56,8 +56,8 @@ JX.install('DiffInline', {
         this._displaySide = 'left';
       }
 
-      this._number = data.number;
+      this._number = parseInt(data.number, 10);
-      this._length = data.length;
+      this._length = parseInt(data.length, 10);
       this._originalText = data.original;
       this._isNewFile =
         (this.getDisplaySide() == 'right') ||
@@ -70,8 +70,8 @@ JX.install('DiffInline', {
 
     bindToRange: function(data) {
       this._displaySide = data.displaySide;
-      this._number = data.number;
+      this._number = parseInt(data.number, 10);
-      this._length = data.length;
+      this._length = parseInt(data.length, 10);
       this._isNewFile = data.isNewFile;
       this._changesetID = data.changesetID;
 
@@ -365,9 +365,13 @@ JX.install('DiffInline', {
     },
 
     _oncreateresponse: function(response) {
+      try {
       var rows = JX.$H(response).getNode();
 
       this._drawEditRows(rows);
+      } catch (e) {
+        JX.log(e);
+      }
     },
 
     _ondeleteresponse: function() {
@@ -409,6 +413,7 @@ JX.install('DiffInline', {
       var first_row = JX.DOM.scry(rows, 'tr')[0];
       var first_meta;
       var row = first_row;
+      var anchor = cursor || this._row;
       cursor = cursor || this._row.nextSibling;
 
       var next_row;
@@ -417,7 +422,11 @@ JX.install('DiffInline', {
         // into the document.
         next_row = row.nextSibling;
 
-        cursor.parentNode.insertBefore(row, cursor);
+        // Bind edit and undo rows to this DiffInline object so that
+        // interactions like hovering work properly.
+        JX.Stratcom.getData(row).inline = this;
+
+        anchor.parentNode.insertBefore(row, cursor);
         cursor = row;
 
         var row_meta = {
@@ -528,6 +537,8 @@ JX.install('DiffInline', {
       this._removeRow(row);
 
       this.setInvisible(false);
+
+      this._didUpdate(true);
     },
 
     _readText: function(row) {
@@ -579,8 +590,9 @@ JX.install('DiffInline', {
       }
 
       this.getChangeset().getChangesetList().redrawCursor();
+      this.getChangeset().getChangesetList().resetHover();
 
-      // Emit a resize event so that UI elements like the keyboad focus
+      // Emit a resize event so that UI elements like the keyboard focus
       // reticle can redraw properly.
       JX.Stratcom.invoke('resize');
     },

webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js

@@ -11,7 +11,6 @@ JX.behavior('differential-edit-inline-comments', function(config) {
 
   var selecting = false;
   var reticle = JX.$N('div', {className: 'differential-reticle'});
-  var old_cells = [];
   JX.DOM.hide(reticle);
 
   var origin = null;
@@ -19,28 +18,6 @@ JX.behavior('differential-edit-inline-comments', function(config) {
   var root   = null;
   var changeset = null;
 
-  var editor = null;
-
-  function updateReticleForComment(e) {
-    root = e.getNode('differential-changeset');
-    if (!root) {
-      return;
-    }
-
-    var data = e.getNodeData('differential-inline-comment');
-    var change = e.getNodeData('differential-changeset');
-
-    var id_part = data.on_right ? change.right : change.left;
-    var new_part = data.isNewFile ? 'N' : 'O';
-    var prefix = 'C' + id_part + new_part + 'L';
-
-    origin = JX.$(prefix + data.number);
-    target = JX.$(prefix + (parseInt(data.number, 10) +
-                            parseInt(data.length, 10)));
-
-    updateReticle();
-  }
-
   function updateReticle() {
     JX.DOM.getContentFrame().appendChild(reticle);
 
@@ -85,44 +62,10 @@ JX.behavior('differential-edit-inline-comments', function(config) {
     dim.setDim(reticle);
 
     JX.DOM.show(reticle);
-
-    // Find all the cells in the same row position between the top and bottom
-    // cell, so we can highlight them.
-    var seq = 0;
-    var row = top.parentNode;
-    for (seq = 0; seq < row.childNodes.length; seq++) {
-      if (row.childNodes[seq] == top) {
-        break;
-      }
-    }
-
-    var cells = [];
-    while (true) {
-      cells.push(row.childNodes[seq]);
-      if (row.childNodes[seq] == bot) {
-        break;
-      }
-      row = row.nextSibling;
-    }
-
-    setSelectedCells(cells);
-  }
-
-  function setSelectedCells(new_cells) {
-    updateSelectedCellsClass(old_cells, false);
-    updateSelectedCellsClass(new_cells, true);
-    old_cells = new_cells;
-  }
-
-  function updateSelectedCellsClass(cells, selected) {
-    for (var ii = 0; ii < cells.length; ii++) {
-      JX.DOM.alterClass(cells[ii], 'selected', selected);
-    }
   }
 
   function hideReticle() {
     JX.DOM.hide(reticle);
-    setSelectedCells([]);
   }
 
   function isOnRight(node) {
@@ -180,13 +123,6 @@ JX.behavior('differential-edit-inline-comments', function(config) {
         return;
       }
 
-      if (editor) {
-        // Don't update the reticle if we're editing a comment, since this
-        // would be distracting and we want to keep the lines corresponding
-        // to the comment highlighted during the edit.
-        return;
-      }
-
       if (getRowNumber(e.getTarget()) === undefined) {
         // Don't update the reticle if this "<th />" doesn't correspond to a
         // line number. For instance, this may be a dead line number, like the
@@ -236,7 +172,7 @@ JX.behavior('differential-edit-inline-comments', function(config) {
     'mouseup',
     null,
     function(e) {
-      if (editor || !selecting) {
+      if (!selecting) {
         return;
       }
 
@@ -280,12 +216,7 @@ JX.behavior('differential-edit-inline-comments', function(config) {
       if (e.getIsTouchEvent()) {
         return;
       }
-
-      if (e.getType() == 'mouseout') {
       hideReticle();
-      } else {
-        updateReticleForComment(e);
-      }
     });
 
 });