Improve offset/range inline behavior for rich diffs and unified diffs

Summary:
Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust.

However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good.

Also, adjust unified diff behavior to work correctly with highlighting and range selection.

Test Plan:
  - Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block").
  - Used range selection in a unified diff, got equivalent behavior to 2-up diffs.

Maniphest Tasks: T13513

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

Author: epriestley

resources/celerity/map.php

@@ -13,7 +13,7 @@
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '319dca29',
-    'differential.pkg.js' => 'ccf7bdca',
+    'differential.pkg.js' => '695827fc',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -380,7 +380,7 @@
     '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' => 'bfdae878',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'a00bf62d',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'b1b8500b',
     'rsrc/js/application/diff/DiffInline.js' => 'b00168c1',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -775,7 +775,7 @@
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'bfdae878',
-    'phabricator-diff-changeset-list' => 'a00bf62d',
+    'phabricator-diff-changeset-list' => 'b1b8500b',
     'phabricator-diff-inline' => 'b00168c1',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1808,11 +1808,6 @@
       'javelin-util',
       'phabricator-keyboard-shortcut',
     ),
-    'a00bf62d' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     'a17b84f1' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1932,6 +1927,11 @@
       'javelin-stratcom',
       'javelin-dom',
     ),
+    'b1b8500b' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'b26a41e4' => array(
       'javelin-behavior',
       'javelin-stratcom',

src/applications/differential/render/DifferentialChangesetOneUpRenderer.php

@@ -45,13 +45,15 @@ protected function renderPrimitives(array $primitives, $rows) {
       'span',
       array(
         'aural' => true,
+        'data-aural' => true,
       ),
       '- ');
 
     $aural_plus = javelin_tag(
       'span',
       array(
         'aural' => true,
+        'data-aural' => true,
       ),
       '+ ');
 
@@ -171,7 +173,15 @@ protected function renderPrimitives(array $primitives, $rows) {
             }
 
             $cells[] = $no_copy;
-            $cells[] = phutil_tag('td', array('class' => $class), $render);
+
+            $cells[] = phutil_tag(
+              'td',
+              array(
+                'class' => $class,
+                'data-copy-mode' => 'copy-unified',
+              ),
+              $render);
+
             $cells[] = $no_coverage;
           }
 

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

@@ -441,12 +441,22 @@ JX.install('DiffChangesetList', {
 
       this._setSourceSelection(null, null);
 
-      var config = {
-        startOffset: start.offset,
-        endOffset: end.offset
-      };
-
       var changeset = start.changeset;
+
+      var config = {};
+      if (changeset.getResponseDocumentEngineKey() === null) {
+        // If the changeset is using a document renderer, we ignore the
+        // selection range and just treat this as a comment from the first
+        // block to the last block.
+
+        // If we don't discard the range, we later render a bogus highlight
+        // if the block content is complex (like a Jupyter notebook cell
+        // with images).
+
+        config.startOffset = start.offset;
+        config.endOffset = end.offset;
+      }
+
       changeset.newInlineForRange(start.targetNode, end.targetNode, config);
     },
 
@@ -2623,7 +2633,7 @@ JX.install('DiffChangesetList', {
           td = cells[cells.length - 1];
           is_end = true;
         } else {
-          td = JX.DOM.findAbove(fragment, 'td');
+          td = this._findContentCell(fragment);
           is_end = false;
         }
 
@@ -2707,6 +2717,16 @@ JX.install('DiffChangesetList', {
     },
 
     _getSelectionOffset: function(node, target) {
+      // If this is an aural hint node in a unified diff, ignore it when
+      // calculating the selection offset.
+      if (node.getAttribute && node.getAttribute('data-aural')) {
+        return {
+          offset: 0,
+          content: '',
+          found: false
+        };
+      }
+
       if (!node.childNodes || !node.childNodes.length) {
         return {
           offset: node.textContent.length,
@@ -2764,6 +2784,16 @@ JX.install('DiffChangesetList', {
 
     _isContentCell: function(node) {
       return !!node.getAttribute('data-copy-mode');
+    },
+
+    _findContentCell: function(node) {
+      var cursor = node;
+      while (true) {
+        cursor = JX.DOM.findAbove(cursor, 'td');
+        if (this._isContentCell(cursor)) {
+          return cursor;
+        }
+      }
     }
 
   }