From 87a4940924bcf7e19b234cae2ad63f6ee1925b5d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 20 May 2017 05:26:51 -0700
Subject: [PATCH] (stable) Fix a diff objective issue where objectives could
 appear in the wrong place

Summary:
Ref T12733. When creating a new comment, the objective could appear to far up in the scrollbar because we were anchoring it to an invisible row.

Instead, anchor to the "edit" row while editing.

Test Plan: Created a new comment at the very top of a file, saw "File, Star" icons instead of "Star, File".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12733

Differential Revision: https://secure.phabricator.com/D17978
---
 resources/celerity/map.php                    | 30 +++++++++----------
 .../rsrc/js/application/diff/DiffInline.js    | 16 ++++++++--
 .../js/application/diff/ScrollObjective.js    |  1 +
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 97b4f15a35..968862b535 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '599698a7',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '7d4cfa59',
-    'differential.pkg.js' => 'f94e941c',
+    'differential.pkg.js' => 'fc6a23eb',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -392,8 +392,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
     'rsrc/js/application/diff/DiffChangeset.js' => 'cf4e2140',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'a716ca27',
-    'rsrc/js/application/diff/DiffInline.js' => 'fa07d36e',
-    'rsrc/js/application/diff/ScrollObjective.js' => '2e069f79',
+    'rsrc/js/application/diff/DiffInline.js' => '93cbb03f',
+    'rsrc/js/application/diff/ScrollObjective.js' => '9df4e4e2',
     'rsrc/js/application/diff/ScrollObjectiveList.js' => '085dd101',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07',
@@ -779,7 +779,7 @@ return array(
     'phabricator-dashboard-css' => 'fe5b1869',
     'phabricator-diff-changeset' => 'cf4e2140',
     'phabricator-diff-changeset-list' => 'a716ca27',
-    'phabricator-diff-inline' => 'fa07d36e',
+    'phabricator-diff-inline' => '93cbb03f',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
     'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -799,7 +799,7 @@ return array(
     'phabricator-phtize' => 'd254d646',
     'phabricator-prefab' => 'c5af80a2',
     'phabricator-remarkup-css' => 'd1a5e11e',
-    'phabricator-scroll-objective' => '2e069f79',
+    'phabricator-scroll-objective' => '9df4e4e2',
     'phabricator-scroll-objective-list' => '085dd101',
     'phabricator-search-results-css' => 'f87d23ad',
     'phabricator-shaped-request' => '7cbe244b',
@@ -1113,13 +1113,6 @@ return array(
       'javelin-install',
       'javelin-event',
     ),
-    '2e069f79' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-    ),
     '2ee659ce' => array(
       'javelin-install',
     ),
@@ -1611,6 +1604,9 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
+    '93cbb03f' => array(
+      'javelin-dom',
+    ),
     '93d0c9e3' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1675,6 +1671,13 @@ return array(
     '9d9685d6' => array(
       'phui-oi-list-view-css',
     ),
+    '9df4e4e2' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+    ),
     '9f36c42d' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2206,9 +2209,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    'fa07d36e' => array(
-      'javelin-dom',
-    ),
     'fbe497e7' => array(
       'javelin-behavior',
       'javelin-util',
diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js
index 32cf0ac5a1..77a0da3ec7 100644
--- a/webroot/rsrc/js/application/diff/DiffInline.js
+++ b/webroot/rsrc/js/application/diff/DiffInline.js
@@ -74,9 +74,10 @@ JX.install('DiffInline', {
 
       this._changesetID = data.changesetID;
 
-      this.updateObjective();
       this.setInvisible(false);
 
+      this.updateObjective();
+
       return this;
     },
 
@@ -166,9 +167,11 @@ JX.install('DiffInline', {
       this._changeset = changeset;
 
       var objectives = changeset.getChangesetList().getObjectives();
+
+      // Create this inline's objective, but don't show it yet.
       this._objective = objectives.newObjective()
-        .setCallback(JX.bind(this, this._onobjective));
-      this.updateObjective();
+        .setCallback(JX.bind(this, this._onobjective))
+        .hide();
 
       return this;
     },
@@ -206,11 +209,17 @@ JX.install('DiffInline', {
       var icon = 'fa-comment';
       var color = 'bluegrey';
       var tooltip = null;
+      var anchor = this._row;
 
       if (this._isEditing) {
         icon = 'fa-star';
         color = 'pink';
         tooltip = pht('Editing Comment');
+
+        // If we're editing, anchor to the row with the editor instead of the
+        // actual comment row (which is invisible and can have a misleading
+        // position).
+        anchor = this._row.nextSibling;
       } else if (this._isDraft) {
         // This inline is an unsubmitted draft.
         icon = 'fa-pencil';
@@ -225,6 +234,7 @@ JX.install('DiffInline', {
       }
 
       objective
+        .setAnchor(anchor)
         .setIcon(icon)
         .setColor(color)
         .setTooltip(tooltip)
diff --git a/webroot/rsrc/js/application/diff/ScrollObjective.js b/webroot/rsrc/js/application/diff/ScrollObjective.js
index c9fd4bb67e..b8fb169914 100644
--- a/webroot/rsrc/js/application/diff/ScrollObjective.js
+++ b/webroot/rsrc/js/application/diff/ScrollObjective.js
@@ -111,6 +111,7 @@ JX.install('ScrollObjective', {
 
     hide: function() {
       this._visible = false;
+      return this;
     },
 
     isVisible: function() {