Correct various minor diff copy behaviors

Summary:
Ref T12822. Fixes a few things:

  - Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
  - "Show More Context" rows now render, highlight, and select properly.
  - Prepares for nodes to have copy-text which is different from display-text.
  - Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.

Test Plan:
  - Selected and copied weird ranges in Firefox.
  - Kept an eye on "Show More Context" rows across select and copy operations.
  - Generally poked around in Safari/Firefox/Chrome.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12822

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

resources/celerity/map.php

@@ -10,9 +10,9 @@ return array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
     'core.pkg.css' => '261ee8cf',
-    'core.pkg.js' => '5ba0b6d7',
-    'differential.pkg.css' => 'd1b29c9c',
-    'differential.pkg.js' => '0e2b0e2c',
+    'core.pkg.js' => 'e368deda',
+    'differential.pkg.css' => '249b542d',
+    'differential.pkg.js' => '53f8d00c',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '91192d85',
     'maniphest.pkg.css' => '35995d6d',
@@ -61,7 +61,7 @@ return array(
     'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6',
     'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d',
     'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
-    'rsrc/css/application/differential/changeset-view.css' => 'e2b81e85',
+    'rsrc/css/application/differential/changeset-view.css' => 'cc3fd795',
     'rsrc/css/application/differential/core.css' => 'bdb93065',
     'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
     'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
@@ -375,7 +375,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '9b1cbd76',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd0a85a85',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '26fb79ba',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '04023d82',
     'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94',
     'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -473,7 +473,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' => 'a4af0b4a',
-    'rsrc/js/core/behavior-oncopy.js' => 'f20d66c1',
+    'rsrc/js/core/behavior-oncopy.js' => 'de59bf15',
     'rsrc/js/core/behavior-phabricator-nav.js' => 'f166c949',
     'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '2f80333f',
     'rsrc/js/core/behavior-read-only-warning.js' => 'b9109f8f',
@@ -541,7 +541,7 @@ return array(
     'conpherence-thread-manager' => 'aec8e38c',
     'conpherence-transaction-css' => '3a3f5e7e',
     'd3' => 'd67475f5',
-    'differential-changeset-view-css' => 'e2b81e85',
+    'differential-changeset-view-css' => 'cc3fd795',
     'differential-core-view-css' => 'bdb93065',
     'differential-revision-add-comment-css' => '7e5900d9',
     'differential-revision-comment-css' => '7dbc8d1d',
@@ -636,7 +636,7 @@ return array(
     'javelin-behavior-phabricator-nav' => 'f166c949',
     'javelin-behavior-phabricator-notification-example' => '29819b75',
     'javelin-behavior-phabricator-object-selector' => 'a4af0b4a',
-    'javelin-behavior-phabricator-oncopy' => 'f20d66c1',
+    'javelin-behavior-phabricator-oncopy' => 'de59bf15',
     'javelin-behavior-phabricator-remarkup-assist' => '2f80333f',
     'javelin-behavior-phabricator-reveal-content' => 'b105a3a6',
     'javelin-behavior-phabricator-search-typeahead' => '1cb7d027',
@@ -754,7 +754,7 @@ return array(
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '4267d6c6',
     'phabricator-diff-changeset' => 'd0a85a85',
-    'phabricator-diff-changeset-list' => '26fb79ba',
+    'phabricator-diff-changeset-list' => '04023d82',
     'phabricator-diff-inline' => 'a4a14a94',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
     'phabricator-draggable-list' => '3c6bd549',
@@ -907,6 +907,10 @@ return array(
       'javelin-uri',
       'javelin-util',
     ),
+    '04023d82' => array(
+      'javelin-install',
+      'phuix-button-view',
+    ),
     '04f8a1e3' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1087,10 +1091,6 @@ return array(
       'javelin-json',
       'phabricator-draggable-list',
     ),
-    '26fb79ba' => array(
-      'javelin-install',
-      'phuix-button-view',
-    ),
     '27daef73' => array(
       'multirow-row-manager',
       'javelin-install',
@@ -1961,6 +1961,9 @@ return array(
       'javelin-util',
       'phabricator-keyboard-shortcut-manager',
     ),
+    'cc3fd795' => array(
+      'phui-inline-comment-view-css',
+    ),
     'cf32921f' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2007,6 +2010,10 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    'de59bf15' => array(
+      'javelin-behavior',
+      'javelin-dom',
+    ),
     'dfa1d313' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2032,9 +2039,6 @@ return array(
       'javelin-dom',
       'javelin-stratcom',
     ),
-    'e2b81e85' => array(
-      'phui-inline-comment-view-css',
-    ),
     'e562708c' => array(
       'javelin-install',
     ),
@@ -2086,10 +2090,6 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
-    'f20d66c1' => array(
-      'javelin-behavior',
-      'javelin-dom',
-    ),
     'f340a484' => array(
       'javelin-install',
       'javelin-dom',

src/applications/differential/render/DifferentialChangesetHTMLRenderer.php

@@ -432,11 +432,17 @@ abstract class DifferentialChangesetHTMLRenderer
     $classes[] = 'PhabricatorMonospaced';
     $classes[] = $this->getRendererTableClass();
 
+    $sigils = array();
+    $sigils[] = 'differential-diff';
+    foreach ($this->getTableSigils() as $sigil) {
+      $sigils[] = $sigil;
+    }
+
     return javelin_tag(
       'table',
       array(
         'class' => implode(' ', $classes),
-        'sigil' => 'differential-diff intercept-copy',
+        'sigil' => implode(' ', $sigils),
       ),
       array(
         $this->renderColgroup(),
@@ -444,6 +450,10 @@ abstract class DifferentialChangesetHTMLRenderer
       ));
   }
 
+  protected function getTableSigils() {
+    return array();
+  }
+
   protected function buildInlineComment(
     PhabricatorInlineCommentInterface $comment,
     $on_right = false) {

src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php

@@ -117,16 +117,20 @@ final class DifferentialChangesetTwoUpRenderer
             phutil_tag(
               'td',
               array(
-                'colspan' => 2,
+                'class' => 'show-context-line n left-context',
+              )),
+            phutil_tag(
+              'td',
+              array(
                 'class' => 'show-more',
               ),
               $contents),
             phutil_tag(
-              'th',
+              'td',
               array(
-                'class' => 'show-context-line',
-              ),
-              $context_line ? (int)$context_line : null),
+                'class' => 'show-context-line n',
+                'data-n' => $context_line,
+              )),
             phutil_tag(
               'td',
               array(
@@ -448,4 +452,10 @@ final class DifferentialChangesetTwoUpRenderer
     return $this->newOffsetMap;
   }
 
+  protected function getTableSigils() {
+    return array(
+      'intercept-copy',
+    );
+  }
+
 }

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

@@ -87,7 +87,7 @@
   color: {$darkgreytext};
 }
 
-.differential-changeset-immutable .differential-diff th {
+.differential-changeset-immutable .differential-diff td {
   cursor: auto;
 }
 
@@ -182,6 +182,10 @@ should always have a boring grey background. */
   content: attr(data-n);
 }
 
+.differential-diff td.show-context-line.n {
+  cursor: auto;
+}
+
 .differential-diff td.cov {
   padding: 0;
 }
@@ -222,7 +226,7 @@ td.cov-I {
 }
 
 .differential-diff td.show-more,
-.differential-diff th.show-context-line,
+.differential-diff td.show-context-line,
 .differential-diff td.show-context,
 .differential-diff td.differential-shield {
   background: {$lightbluebackground};
@@ -232,7 +236,7 @@ td.cov-I {
 }
 
 .device .differential-diff td.show-more,
-.device .differential-diff th.show-context-line,
+.device .differential-diff td.show-context-line,
 .device .differential-diff td.show-context,
 .device .differential-diff td.differential-shield {
   padding: 6px 0;
@@ -250,10 +254,14 @@ td.cov-I {
   color: {$bluetext};
 }
 
-.differential-diff th.show-context-line {
+.differential-diff td.show-context-line {
   padding-right: 6px;
 }
 
+.differential-diff td.show-context-line.left-context {
+  border-right: none;
+}
+
 .differential-diff td.show-context {
   padding-left: 14px;
 }
@@ -431,8 +439,7 @@ unselectable. */
 
 .differential-diff.copy-l > tbody > tr > td,
 .differential-diff.copy-r > tbody > tr > td {
-  -moz-user-select: -moz-none;
-  -khtml-user-select: none;
+  -moz-user-select: none;
   -ms-user-select: none;
   -webkit-user-select: none;
   user-select: none;
@@ -444,12 +451,24 @@ unselectable. */
 }
 
 .differential-diff.copy-l > tbody > tr > td:nth-child(2) {
+  -moz-user-select: auto;
+  -ms-user-select: auto;
   -webkit-user-select: auto;
   user-select: auto;
   opacity: 1;
 }
 
+.differential-diff.copy-l > tbody > tr > td.show-more:nth-child(2) {
+  -moz-user-select: none;
+  -ms-user-select: none;
+  -webkit-user-select: none;
+  user-select: none;
+  opacity: 0.25;
+}
+
 .differential-diff.copy-r > tbody > tr > td:nth-child(5) {
+  -moz-user-select: auto;
+  -ms-user-select: auto;
   -webkit-user-select: auto;
   user-select: auto;
   opacity: 1;

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

@@ -1246,8 +1246,24 @@ JX.install('DiffChangesetList', {
       return changeset.getInlineForRow(inline_row);
     },
 
-    getLineNumberFromHeader: function(th) {
-      return parseInt(th.getAttribute('data-n'));
+    getLineNumberFromHeader: function(node) {
+      var n = parseInt(node.getAttribute('data-n'));
+
+      if (!n) {
+        return null;
+      }
+
+      // If this is a line number that's part of a row showing more context,
+      // we don't want to let users leave inlines here.
+
+      try {
+        JX.DOM.findAbove(node, 'tr', 'context-target');
+        return null;
+      } catch (ex) {
+        // Ignore.
+      }
+
+      return n;
     },
 
     getDisplaySideFromHeader: function(th) {
@@ -1295,7 +1311,7 @@ JX.install('DiffChangesetList', {
     },
 
     _updateRange: function(target, is_out) {
-      // Don't update the range if this "<th />" doesn't correspond to a line
+      // Don't update the range if this target doesn't correspond to a line
       // number. For instance, this may be a dead line number, like the empty
       // line numbers on the left hand side of a newly added file.
       var number = this.getLineNumberFromHeader(target);

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

@@ -186,12 +186,12 @@ JX.behavior('phabricator-oncopy', function() {
       return;
     }
 
-    var text_nodes = [];
+    var text = [];
     for (var ii = 0; ii < ranges.length; ii++) {
       var range = ranges[ii];
 
       var fragment = range.cloneContents();
-      if (!fragment.children.length) {
+      if (!fragment.childNodes.length) {
         continue;
       }
 
@@ -217,48 +217,91 @@ JX.behavior('phabricator-oncopy', function() {
 
       for (var jj = 0; jj < fragment.childNodes.length; jj++) {
         var node = fragment.childNodes[jj];
-        if (JX.DOM.isType(node, 'tr')) {
-          // This is an inline comment row, so we never want to copy any
-          // content inside of it.
-          if (JX.Stratcom.hasSigil(node, 'inline-row')) {
-            continue;
-          }
-
-          // Assume anything else is a source code row. Keep only "<td>" cells
-          // with the correct mode.
-          for (var kk = 0; kk < node.childNodes.length; kk++) {
-            var child = node.childNodes[kk];
-
-            var node_mode = child.getAttribute('data-copy-mode');
-            if (node_mode === copy_mode) {
-              text_nodes.push(child);
-            }
-          }
-        } else {
-          // For anything else, assume this is a text fragment or part of
-          // a table cell or something and should be included in the selection
-          // range.
-          text_nodes.push(node);
-        }
+        text.push(extract_text(node));
       }
-
-      var text = [];
-      for (ii = 0; ii < text_nodes.length; ii++) {
-        text.push(text_nodes[ii].textContent);
-      }
-      text = text.join('');
-
-      var rawEvent = e.getRawEvent();
-      var data;
-      if ('clipboardData' in rawEvent) {
-        data = rawEvent.clipboardData;
-      } else {
-        data = window.clipboardData;
-      }
-      data.setData('Text', text);
-
-      e.prevent();
     }
+
+    text = flatten_list(text);
+    text = text.join('');
+
+    var rawEvent = e.getRawEvent();
+    var data;
+    if ('clipboardData' in rawEvent) {
+      data = rawEvent.clipboardData;
+    } else {
+      data = window.clipboardData;
+    }
+    data.setData('Text', text);
+
+    e.prevent();
+  }
+
+  function extract_text(node) {
+    var ii;
+    var text = [];
+
+    if (JX.DOM.isType(node, 'tr')) {
+      // This is an inline comment row, so we never want to copy any
+      // content inside of it.
+      if (JX.Stratcom.hasSigil(node, 'inline-row')) {
+        return null;
+      }
+
+      // This is a "Show More Context" row, so we never want to copy any
+      // of the content inside.
+      if (JX.Stratcom.hasSigil(node, 'context-target')) {
+        return null;
+      }
+
+      // Assume anything else is a source code row. Keep only "<td>" cells
+      // with the correct mode.
+      for (ii = 0; ii < node.childNodes.length; ii++) {
+        text.push(extract_text(node.childNodes[ii]));
+      }
+
+      return text;
+    }
+
+    if (JX.DOM.isType(node, 'td')) {
+      var node_mode = node.getAttribute('data-copy-mode');
+      if (node_mode !== copy_mode) {
+        return;
+      }
+
+      // Otherwise, fall through and extract this node's text normally.
+    }
+
+    if (!node.childNodes || !node.childNodes.length) {
+      return node.textContent;
+    }
+
+    for (ii = 0; ii < node.childNodes.length; ii++) {
+      var child = node.childNodes[ii];
+      text.push(extract_text(child));
+    }
+
+    return text;
+  }
+
+  function flatten_list(list) {
+    var stack = [list];
+    var result = [];
+    while (stack.length) {
+      var next = stack.pop();
+      if (JX.isArray(next)) {
+        for (var ii = 0; ii < next.length; ii++) {
+          stack.push(next[ii]);
+        }
+      } else if (next === null) {
+        continue;
+      } else if (next === undefined) {
+        continue;
+      } else {
+        result.push(next);
+      }
+    }
+
+    return result.reverse();
   }
 
   JX.enableDispatch(document.body, 'copy');