Give selected inline comments are more obvious selected state

Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.

Also fix a couple of minor issues with select interactions and offset comments.

Test Plan: Selected inlines, saw obvious visual cues.

Maniphest Tasks: T13513

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

Author: epriestley

resources/celerity/map.php

@@ -12,8 +12,8 @@
     'core.pkg.css' => 'a560707d',
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
-    'differential.pkg.css' => 'b042ee8b',
-    'differential.pkg.js' => 'e0600220',
+    'differential.pkg.css' => '319dca29',
+    'differential.pkg.js' => 'ccf7bdca',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -65,7 +65,7 @@
     'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
     'rsrc/css/application/differential/changeset-view.css' => 'df3afa61',
     'rsrc/css/application/differential/core.css' => '7300a73e',
-    'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
+    'rsrc/css/application/differential/phui-inline-comment.css' => 'd5749acc',
     'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
     'rsrc/css/application/differential/revision-history.css' => '8aa3eac5',
     'rsrc/css/application/differential/revision-list.css' => '93d2df7d',
@@ -379,9 +379,9 @@
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     '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' => 'd721533b',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '8b0eab21',
-    'rsrc/js/application/diff/DiffInline.js' => '734d3c33',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'bfdae878',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'a00bf62d',
+    'rsrc/js/application/diff/DiffInline.js' => 'b00168c1',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -774,9 +774,9 @@
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => 'd721533b',
-    'phabricator-diff-changeset-list' => '8b0eab21',
-    'phabricator-diff-inline' => '734d3c33',
+    'phabricator-diff-changeset' => 'bfdae878',
+    'phabricator-diff-changeset-list' => 'a00bf62d',
+    'phabricator-diff-inline' => 'b00168c1',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -854,7 +854,7 @@
     'phui-icon-view-css' => '4cbc684a',
     'phui-image-mask-css' => '62c7f4d2',
     'phui-info-view-css' => 'a10a909b',
-    'phui-inline-comment-view-css' => '48acce5b',
+    'phui-inline-comment-view-css' => 'd5749acc',
     'phui-invisible-character-view-css' => 'c694c4a4',
     'phui-left-right-css' => '68513c34',
     'phui-lightbox-css' => '4ebf22da',
@@ -1560,9 +1560,6 @@
       'javelin-util',
       'javelin-reactor-node-calmer',
     ),
-    '734d3c33' => array(
-      'javelin-dom',
-    ),
     '73ecc1f8' => array(
       'javelin-behavior',
       'javelin-behavior-device',
@@ -1670,11 +1667,6 @@
       'javelin-dom',
       'phabricator-draggable-list',
     ),
-    '8b0eab21' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     '8b5c7d65' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1816,6 +1808,11 @@
       'javelin-util',
       'phabricator-keyboard-shortcut',
     ),
+    'a00bf62d' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'a17b84f1' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1927,6 +1924,9 @@
       'javelin-behavior-device',
       'javelin-vector',
     ),
+    'b00168c1' => array(
+      'javelin-dom',
+    ),
     'b105a3a6' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2019,6 +2019,19 @@
     'bcec20f0' => array(
       'phui-theme-css',
     ),
+    'bfdae878' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+    ),
     'c03f2fb4' => array(
       'javelin-install',
     ),
@@ -2090,19 +2103,6 @@
     'd4cc2d2a' => array(
       'javelin-install',
     ),
-    'd721533b' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-      'phuix-button-view',
-    ),
     'd8a86cfb' => array(
       'javelin-behavior',
       'javelin-dom',

src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php

@@ -82,6 +82,7 @@ public function buildVariables() {
       'alphablack'          => '0,0,0',
 
       // Base Greys
+      'thingreyborder'     => '#dadee8',
       'lightgreyborder'     => '#C7CCD9',
       'greyborder'          => '#A1A6B0',
       'darkgreyborder'      => '#676A70',
@@ -207,6 +208,7 @@ public function buildVariables() {
       // Usually light yellow
       'gentle.highlight' => '#fdf3da',
       'gentle.highlight.border' => '#c9b8a8',
+      'gentle.highlight.background' => '#fffdf6',
 
       'highlight.bright' => '#fdf320',
 

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

@@ -79,10 +79,6 @@ public function render() {
     require_celerity_resource('phui-inline-comment-view-css');
     $inline = $this->getInlineComment();
 
-    $classes = array(
-      'differential-inline-comment',
-    );
-
     $is_synthetic = false;
     if ($inline->getSyntheticAuthor()) {
       $is_synthetic = true;
@@ -92,14 +88,18 @@ public function render() {
 
     $metadata = $this->getInlineCommentMetadata();
 
+    $classes = array(
+      'differential-inline-comment',
+    );
+
     $sigil = 'differential-inline-comment';
     if ($is_preview) {
       $sigil = $sigil.' differential-inline-comment-preview';
-    }
 
-    $classes = array(
-      'differential-inline-comment',
-    );
+      $classes[] = 'inline-comment-preview';
+    } else {
+      $classes[] = 'inline-comment-element';
+    }
 
     $content = $inline->getContent();
     $handles = $this->handles;

webroot/rsrc/css/application/differential/phui-inline-comment.css

@@ -25,8 +25,6 @@
 
 .differential-inline-comment,
 .differential-inline-comment-edit {
-  background: {$page.content};
-  border: 1px solid {$gentle.highlight.border};
   font: {$basefont};
   -moz-box-sizing: border-box;
   -webkit-box-sizing: border-box;
@@ -35,6 +33,8 @@
   white-space: normal;
   border-radius: 3px;
   margin: 8px 12px;
+  background: {$page.content};
+  border: 1px solid {$blueborder};
 }
 
 .device .differential-inline-comment {
@@ -48,9 +48,10 @@
 .differential-inline-comment-head {
   font-weight:  bold;
   color: {$darkbluetext};
-  border-bottom: 1px solid {$gentle.highlight.border};
   padding: 4px 5px 4px 8px;
-  background-color: {$gentle.highlight};
+
+  background: {$lightbluebackground};
+  border-bottom: 1px solid {$blueborder};
 }
 
 .differential-inline-comment-content {
@@ -261,21 +262,14 @@
     color: {$sky};
 }
 
-
-/* - Inline Is Done -----------------------------------------------------------
-
-  Is Done for Diff Author = grey, for Diff Viewer = yellow.
-
-*/
-
 .differential-inline-comment.inline-is-done {
-  border-color: {$lightgreyborder};
+  border-color: {$thingreyborder};
 }
 
 .differential-inline-comment.inline-is-done
   .differential-inline-comment-head {
     background-color: {$lightgreybackground};
-    border-bottom-color: {$lightgreyborder};
+    border-bottom-color: {$thingreyborder};
 }
 
 .differential-inline-comment.inline-is-done .differential-inline-comment-head
@@ -427,3 +421,18 @@
 .differential-inline-comment-synthetic .inline-button-divider {
   border: none;
 }
+
+.inline-comment-element .differential-inline-comment-head {
+  cursor: pointer;
+}
+
+.inline-comment-selected .inline-comment-element {
+  border-color: {$yellow};
+  background: {$gentle.highlight.background};
+}
+
+.inline-comment-selected .inline-comment-element
+  .differential-inline-comment-head {
+  background: {$lightyellow};
+  border-color: {$yellow};
+}

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

@@ -50,8 +50,14 @@ JX.install('DiffChangeset', {
       this._loadChangesetState(data.changesetState);
     }
 
+    JX.enableDispatch(window, 'selectstart');
+
     var onselect = JX.bind(this, this._onClickHeader);
-    JX.DOM.listen(this._node, 'mousedown', 'changeset-header', onselect);
+    JX.DOM.listen(
+      this._node,
+      ['mousedown', 'selectstart'],
+      'changeset-header',
+      onselect);
   },
 
   members: {
@@ -907,6 +913,13 @@ JX.install('DiffChangeset', {
         return;
       }
 
+      // Don't allow repeatedly clicking a header to begin a "select word" or
+      // "select line" operation.
+      if (e.getType() === 'selectstart') {
+        e.kill();
+        return;
+      }
+
       // NOTE: Don't prevent or kill the event. If the user has text selected,
       // clicking a header should clear the selection (and dismiss any inline
       // context menu, if one exists) as clicking elsewhere in the document

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

@@ -29,9 +29,11 @@ JX.install('DiffChangesetList', {
     var onscroll = JX.bind(this, this._ifawake, this._onscroll);
     JX.Stratcom.listen('scroll', null, onscroll);
 
+    JX.enableDispatch(window, 'selectstart');
+
     var onselect = JX.bind(this, this._ifawake, this._onselect);
     JX.Stratcom.listen(
-      'mousedown',
+      ['mousedown', 'selectstart'],
       ['differential-inline-comment', 'differential-inline-header'],
       onselect);
 
@@ -717,7 +719,22 @@ JX.install('DiffChangesetList', {
     },
 
     _setSelectionState: function(item, scroll) {
+      var old = this._cursorItem;
+
+      if (old) {
+        if (old.type === 'comment') {
+          old.target.setIsSelected(false);
+        }
+      }
+
       this._cursorItem = item;
+
+      if (item) {
+        if (item.type === 'comment') {
+          item.target.setIsSelected(true);
+        }
+      }
+
       this._redrawSelection(scroll);
 
       return this;
@@ -1126,6 +1143,14 @@ JX.install('DiffChangesetList', {
         return;
       }
 
+      // If the user has double-clicked or triple-clicked a header, we want to
+      // toggle the inline selection mode, not select text. Kill select events
+      // originating with this element as the target.
+      if (e.getType() === 'selectstart') {
+        e.kill();
+        return;
+      }
+
       var inline = this._getInlineForEvent(e);
       if (!inline) {
         return;

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

@@ -50,6 +50,7 @@ JX.install('DiffInline', {
 
     _startOffset: null,
     _endOffset: null,
+    _isSelected: false,
 
     bindToRow: function(row) {
       this._row = row;
@@ -161,15 +162,38 @@ JX.install('DiffInline', {
       return this._endOffset;
     },
 
+    setIsSelected: function(is_selected) {
+      this._isSelected = is_selected;
+
+      if (this._row) {
+        JX.DOM.alterClass(
+          this._row,
+          'inline-comment-selected',
+          this._isSelected);
+      }
+
+      return this;
+    },
+
     bindToRange: function(data) {
       this._displaySide = data.displaySide;
       this._number = parseInt(data.number, 10);
       this._length = parseInt(data.length, 10);
       this._isNewFile = data.isNewFile;
       this._changesetID = data.changesetID;
       this._isNew = true;
-      this._startOffset = null;
-      this._endOffset = null;
+
+      if (data.hasOwnProperty('startOffset')) {
+        this._startOffset = data.startOffset;
+      } else {
+        this._startOffset = null;
+      }
+
+      if (data.hasOwnProperty('endOffset')) {
+        this._endOffset = data.endOffset;
+      } else {
+        this._endOffset = null;
+      }
 
       // Insert the comment after any other comments which already appear on
       // the same row.