From 37ffb71c4d5efdb25955105f627849690af0ba4f Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Jul 2020 14:56:39 -0700 Subject: [PATCH] In source views, wrap display tabs in "user-select: all" to improve cursor selection behavior Summary: Ref T2495. See PHI1814. Currently, Phabricator replaces tabs with spaces when rendering diffs. This may or may not be the best behavior in the long term, but it gives us more control over expansion of tabs than using tab literals. However, one downside is that you can use your mouse cursor to select "half a tab", and can't use your mouse cursor to distinguish between tabs and spaces. Although you probably shouldn't be doing this, this behavior is less accurate/correct than selecting the entire block as a single unit. A specific correctness issue with this behavior is that the entire block is copied to the clipboard as a tab literal if you select any of it, so two different visual selection ranges can produce the same clipboard content. This particular behavior can be improved with "user-select: all", to instruct browsers to select the entire element as a single logical element. Now, selecting part of the tab selects the whole thing, as though it were really a tab literal. (Some future change might abandon this approach and opt to use real tab literals with "tab-size" CSS, but we lose some ability to control alignment behavior if we do that and it doesn't have any obvious advantages over this approach other than cursor selection behavior.) Test Plan: - In Safari and Firefox, dragged text to select a whitespace-expanded tab literal. Saw browsers select the whole sequence as though it were a single tab. - In Chorme, this also mostly works, but there's some glitchiness and flickering. I think this is still a net improvement, it's just not as smooth as Safari and Firefox. Maniphest Tasks: T2495 Differential Revision: https://secure.phabricator.com/D21419 --- resources/celerity/map.php | 12 ++++++------ .../parser/DifferentialChangesetParser.php | 7 +++++++ .../__tests__/DifferentialTabReplacementTestCase.php | 4 ++-- webroot/rsrc/css/core/syntax.css | 5 +++++ 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d10f117847..77242a1384 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => 'da792a0f', + 'core.pkg.css' => '2e175364', 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', @@ -115,7 +115,7 @@ return array( 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => '1b29ed61', 'rsrc/css/core/remarkup.css' => '7d3ebc86', - 'rsrc/css/core/syntax.css' => '548567f6', + 'rsrc/css/core/syntax.css' => '98fdb17e', 'rsrc/css/core/z-index.css' => 'ac3bfcd4', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', 'rsrc/css/font/font-awesome.css' => '3883938a', @@ -909,7 +909,7 @@ return array( 'sprite-login-css' => '18b368a6', 'sprite-tokens-css' => 'f1896dc5', 'syntax-default-css' => '055fc231', - 'syntax-highlighting-css' => '548567f6', + 'syntax-highlighting-css' => '98fdb17e', 'tokens-css' => 'ce5a50bd', 'trigger-rule' => '41b7b4f6', 'trigger-rule-control' => '5faf27b9', @@ -1422,9 +1422,6 @@ return array( 'phuix-autocomplete', 'javelin-mask', ), - '548567f6' => array( - 'syntax-default-css', - ), '55a24e84' => array( 'javelin-install', 'javelin-dom', @@ -1803,6 +1800,9 @@ return array( 'javelin-request', 'javelin-util', ), + '98fdb17e' => array( + 'syntax-default-css', + ), '995f5102' => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index b7ea61b7a9..45da82118f 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1600,6 +1600,13 @@ final class DifferentialChangesetParser extends Phobject { 'span', array( 'data-copy-text' => "\t", + + // See PHI1814. Mark this as a single logical tab for the purposes + // of text selection behavior: when the user drags their mouse over + // the character sequence, we'd like the whole thing to select as + // a single unit. + + 'class' => 'logical-tab', ), str_repeat(' ', $ii)); $tag = phutil_string_cast($tag); diff --git a/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php b/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php index d63978cb61..494170c668 100644 --- a/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php @@ -4,8 +4,8 @@ final class DifferentialTabReplacementTestCase extends PhabricatorTestCase { public function testTabReplacement() { - $tab1 = " "; - $tab2 = " "; + $tab1 = " "; + $tab2 = " "; $cat = "\xF0\x9F\x90\xB1"; diff --git a/webroot/rsrc/css/core/syntax.css b/webroot/rsrc/css/core/syntax.css index 78aa83dbdc..822e1777d6 100644 --- a/webroot/rsrc/css/core/syntax.css +++ b/webroot/rsrc/css/core/syntax.css @@ -39,3 +39,8 @@ span.crossreference-item { color: #ffffff; cursor: default; } + +.logical-tab { + user-select: all; + -webkit-user-select: all; +}