From b66bf6af9267b0ec40ab0383a1c2a914e05cac21 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 1 Jun 2017 10:41:57 -0700
Subject: [PATCH] When stabilizing document scroll position for diffs, stick to
 anchors harder

Summary: Ref T12779. Try a little harder to get the autoscroll heuristic right, but also just stick to anchors if the URL has an anchor and the scroll position is near that anchor.

Test Plan:
  - Loaded an anchored diff at a bunch of window sizes, seemed pretty sticky.
  - Added `usleep(100000 * mt_rand(1, 15))` to `ChangesetViewController` to make changesets load slowly and in random order, reloaded a bunch of times while scrolling around, things appeared reasonable.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12779

Differential Revision: https://secure.phabricator.com/D18060
---
 resources/celerity/map.php                    | 28 +++++++++----------
 .../rsrc/js/application/diff/DiffChangeset.js | 27 ++++++++++++++++--
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index fcd5a4bc50..0bde2d3862 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '1475bd91',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => 'a2755617',
-    'differential.pkg.js' => '5bf658f0',
+    'differential.pkg.js' => '9cab3335',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -391,7 +391,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
-    'rsrc/js/application/diff/DiffChangeset.js' => '1f6748ae',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'aaaf4cb5',
     'rsrc/js/application/diff/DiffChangesetList.js' => '85abc805',
     'rsrc/js/application/diff/DiffInline.js' => '1d17130f',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
@@ -770,7 +770,7 @@ return array(
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => '1f6748ae',
+    'phabricator-diff-changeset' => 'aaaf4cb5',
     'phabricator-diff-changeset-list' => '85abc805',
     'phabricator-diff-inline' => '1d17130f',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
@@ -1024,17 +1024,6 @@ return array(
       'javelin-uri',
       'javelin-routable',
     ),
-    '1f6748ae' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     '1f6794f6' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1716,6 +1705,17 @@ return array(
       'javelin-util',
       'phabricator-prefab',
     ),
+    'aaaf4cb5' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     'ab2f381b' => array(
       'javelin-request',
       'javelin-behavior',
diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js
index 6b5ebf5317..b5ebe4ce70 100644
--- a/webroot/rsrc/js/application/diff/DiffChangeset.js
+++ b/webroot/rsrc/js/application/diff/DiffChangeset.js
@@ -516,14 +516,35 @@ JX.install('DiffChangeset', {
       var target_bot = (target_pos.y + target_dim.y);
 
       // Detect if the changeset is entirely (or, at least, almost entirely)
-      // above us.
-      var above_screen = (target_bot < old_pos.y + 128);
+      // above us. The height here is roughly the height of the persistent
+      // banner.
+      var above_screen = (target_bot < old_pos.y + 64);
+
+      // If we have a URL anchor and are currently nearby, stick to it
+      // no matter what.
+      var on_target = null;
+      if (window.location.hash) {
+        try {
+          var anchor = JX.$(window.location.hash.replace('#', ''));
+          if (anchor) {
+            var anchor_pos = JX.$V(anchor);
+            if ((anchor_pos.y > old_pos.y) &&
+                (anchor_pos.y < old_pos.y + 96)) {
+              on_target = anchor;
+            }
+          }
+        } catch (ignored) {
+          // If we have a bogus anchor, just ignore it.
+        }
+      }
 
       var frame = this._getContentFrame();
       JX.DOM.setContent(frame, JX.$H(response.changeset));
 
       if (this._stabilize) {
-        if (!near_top) {
+        if (on_target) {
+          JX.DOM.scrollToPosition(old_pos.x, JX.$V(on_target).y - 60);
+        } else if (!near_top) {
           if (near_bot || above_screen) {
             // Figure out how much taller the document got.
             var delta = (JX.Vector.getDocument().y - old_dim.y);