From efccd75ae37174e370a9c88197e60b178fc8e5fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 17 Feb 2019 07:58:26 -0800 Subject: [PATCH] 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 | 40 +++--- .../DifferentialChangesetHTMLRenderer.php | 12 +- .../DifferentialChangesetTwoUpRenderer.php | 20 ++- .../differential/changeset-view.css | 31 ++++- .../js/application/diff/DiffChangesetList.js | 22 ++- webroot/rsrc/js/core/behavior-oncopy.js | 127 ++++++++++++------ 6 files changed, 175 insertions(+), 77 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 086cb1b8b3..04036e70e4 100644 --- a/resources/celerity/map.php +++ b/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', diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index e4320b712b..df59db9228 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/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) { diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 290272db49..7c728c1ff7 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/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', + ); + } + } diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index bc1ca8b6ea..7d578ccc03 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/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; diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 8a1306967a..572faad987 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/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 "" 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); diff --git a/webroot/rsrc/js/core/behavior-oncopy.js b/webroot/rsrc/js/core/behavior-oncopy.js index a37474abf9..e7309a1e18 100644 --- a/webroot/rsrc/js/core/behavior-oncopy.js +++ b/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 "" 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 "" 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');