Fix anchor-clicking scroll positions

Summary:
Fixes T7069. When jumping to a comment anchor, we get the scroll positions wrong.

Partly this is fixing some calcaulations; partly, the "show older comments" and "scroll anchor" stuff were fighting over the scroll position. Since the anchor can take care of things on its own, just let it handle stuff.

Test Plan:
  - Clicked comment anchors.
  - Loaded pages with anchors in the URI.
  - Loaded pages with anchors hidden behind "show older comments".

In all cases, got the right scroll position.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T7069

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

resources/celerity/map.php

@@ -7,8 +7,8 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => '04a24e98',
+    'core.pkg.css' => '3d6955ad',
-    'core.pkg.js' => '7a54aa14',
+    'core.pkg.js' => 'efa12ecc',
     'darkconsole.pkg.js' => '8ab24e01',
     'differential.pkg.css' => '8af45893',
     'differential.pkg.js' => '5c1f3896',
@@ -26,7 +26,7 @@ return array(
     'rsrc/css/aphront/multi-column.css' => '41a848c0',
     'rsrc/css/aphront/notification.css' => '9c279160',
     'rsrc/css/aphront/pager-view.css' => '2e3539af',
-    'rsrc/css/aphront/panel-view.css' => 'a5fee23a',
+    'rsrc/css/aphront/panel-view.css' => '5846dfa2',
     'rsrc/css/aphront/phabricator-nav-view.css' => '7aeaf435',
     'rsrc/css/aphront/table-view.css' => 'b22b7216',
     'rsrc/css/aphront/tokenizer.css' => '82ce2142',
@@ -71,7 +71,6 @@ return array(
     'rsrc/css/application/harbormaster/harbormaster.css' => '49d64eb4',
     'rsrc/css/application/herald/herald-test.css' => '778b008e',
     'rsrc/css/application/herald/herald.css' => '826075fa',
-    'rsrc/css/application/home/home.css' => 'e34bf140',
     'rsrc/css/application/maniphest/batch-editor.css' => '8f380ebc',
     'rsrc/css/application/maniphest/report.css' => '6fc16517',
     'rsrc/css/application/maniphest/task-edit.css' => '8e23031b',
@@ -137,7 +136,7 @@ return array(
     'rsrc/css/phui/phui-info-panel.css' => '27ea50a1',
     'rsrc/css/phui/phui-list.css' => '53deb25c',
     'rsrc/css/phui/phui-object-box.css' => '0d47b3c8',
-    'rsrc/css/phui/phui-object-item-list-view.css' => '832c58fe',
+    'rsrc/css/phui/phui-object-item-list-view.css' => '10297907',
     'rsrc/css/phui/phui-pinboard-view.css' => '3dd4a269',
     'rsrc/css/phui/phui-property-list-view.css' => '51480060',
     'rsrc/css/phui/phui-remarkup-preview.css' => '19ad512b',
@@ -189,7 +188,7 @@ return array(
     'rsrc/externals/javelin/ext/view/__tests__/ViewInterpreter.js' => '7a94d6a5',
     'rsrc/externals/javelin/ext/view/__tests__/ViewRenderer.js' => '6ea96ac9',
     'rsrc/externals/javelin/lib/Cookie.js' => '62dfea03',
-    'rsrc/externals/javelin/lib/DOM.js' => '2d66f6ec',
+    'rsrc/externals/javelin/lib/DOM.js' => '6f7962d5',
     'rsrc/externals/javelin/lib/History.js' => '2e0148bc',
     'rsrc/externals/javelin/lib/JSON.js' => '69adf288',
     'rsrc/externals/javelin/lib/Leader.js' => '331b1611',
@@ -199,9 +198,9 @@ return array(
     'rsrc/externals/javelin/lib/Resource.js' => '44959b73',
     'rsrc/externals/javelin/lib/Routable.js' => 'b3e7d692',
     'rsrc/externals/javelin/lib/Router.js' => '29274e2b',
-    'rsrc/externals/javelin/lib/Scrollbar.js' => '479fd9f1',
+    'rsrc/externals/javelin/lib/Scrollbar.js' => 'ef2ec0c6',
     'rsrc/externals/javelin/lib/URI.js' => '6eff08aa',
-    'rsrc/externals/javelin/lib/Vector.js' => 'cc1bd0b0',
+    'rsrc/externals/javelin/lib/Vector.js' => '2caa8fb8',
     'rsrc/externals/javelin/lib/WebSocket.js' => '3f840822',
     'rsrc/externals/javelin/lib/Workflow.js' => 'a2ccdfec',
     'rsrc/externals/javelin/lib/__tests__/Cookie.js' => '5ed109e8',
@@ -412,7 +411,7 @@ return array(
     'rsrc/js/application/repository/repository-crossreference.js' => 'f9539603',
     'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08',
     'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f',
-    'rsrc/js/application/transactions/behavior-show-older-transactions.js' => 'bde958eb',
+    'rsrc/js/application/transactions/behavior-show-older-transactions.js' => 'dbbf48b6',
     'rsrc/js/application/transactions/behavior-transaction-comment-form.js' => '9f7309fb',
     'rsrc/js/application/transactions/behavior-transaction-list.js' => '13c739ea',
     'rsrc/js/application/uiexample/JavelinViewExample.js' => 'd4a14807',
@@ -479,7 +478,7 @@ return array(
     'rsrc/js/core/behavior-toggle-class.js' => 'e566f52c',
     'rsrc/js/core/behavior-tokenizer.js' => 'b3a4b884',
     'rsrc/js/core/behavior-tooltip.js' => '3ee3408b',
-    'rsrc/js/core/behavior-watch-anchor.js' => '924cdd01',
+    'rsrc/js/core/behavior-watch-anchor.js' => '9f36c42d',
     'rsrc/js/core/behavior-workflow.js' => '0a3f3021',
     'rsrc/js/core/phtize.js' => 'd254d646',
     'rsrc/js/phui/behavior-phui-object-box-tabs.js' => '2bfa2836',
@@ -498,7 +497,7 @@ return array(
     'aphront-list-filter-view-css' => '2ae43867',
     'aphront-multi-column-view-css' => '41a848c0',
     'aphront-pager-view-css' => '2e3539af',
-    'aphront-panel-view-css' => 'a5fee23a',
+    'aphront-panel-view-css' => '5846dfa2',
     'aphront-table-view-css' => 'b22b7216',
     'aphront-tokenizer-control-css' => '82ce2142',
     'aphront-tooltip-css' => '4099b97e',
@@ -532,7 +531,6 @@ return array(
     'herald-css' => '826075fa',
     'herald-rule-editor' => '335fd41f',
     'herald-test-css' => '778b008e',
-    'homepage-panel-css' => 'e34bf140',
     'inline-comment-summary-css' => '8cfd34e8',
     'javelin-aphlict' => '2be71d56',
     'javelin-behavior' => '61cbc29a',
@@ -618,11 +616,11 @@ return array(
     'javelin-behavior-phabricator-remarkup-assist' => 'e32d14ab',
     'javelin-behavior-phabricator-reveal-content' => '60821bc7',
     'javelin-behavior-phabricator-search-typeahead' => '724b1247',
-    'javelin-behavior-phabricator-show-older-transactions' => 'bde958eb',
+    'javelin-behavior-phabricator-show-older-transactions' => 'dbbf48b6',
     'javelin-behavior-phabricator-tooltips' => '3ee3408b',
     'javelin-behavior-phabricator-transaction-comment-form' => '9f7309fb',
     'javelin-behavior-phabricator-transaction-list' => '13c739ea',
-    'javelin-behavior-phabricator-watch-anchor' => '924cdd01',
+    'javelin-behavior-phabricator-watch-anchor' => '9f36c42d',
     'javelin-behavior-phame-post-preview' => 'be807912',
     'javelin-behavior-pholio-mock-edit' => '9c2623f4',
     'javelin-behavior-pholio-mock-view' => 'e58bf807',
@@ -653,7 +651,7 @@ return array(
     'javelin-color' => '7e41274a',
     'javelin-cookie' => '62dfea03',
     'javelin-diffusion-locate-file-source' => 'b42eddc7',
-    'javelin-dom' => '2d66f6ec',
+    'javelin-dom' => '6f7962d5',
     'javelin-dynval' => 'f6555212',
     'javelin-event' => '85ea0626',
     'javelin-fx' => '54b612ba',
@@ -672,7 +670,7 @@ return array(
     'javelin-resource' => '44959b73',
     'javelin-routable' => 'b3e7d692',
     'javelin-router' => '29274e2b',
-    'javelin-scrollbar' => '479fd9f1',
+    'javelin-scrollbar' => 'ef2ec0c6',
     'javelin-stratcom' => '8b0ad945',
     'javelin-tokenizer' => '7644823e',
     'javelin-typeahead' => '70baed2f',
@@ -684,7 +682,7 @@ return array(
     'javelin-typeahead-static-source' => '316b8fa1',
     'javelin-uri' => '6eff08aa',
     'javelin-util' => '93cc50d6',
-    'javelin-vector' => 'cc1bd0b0',
+    'javelin-vector' => '2caa8fb8',
     'javelin-view' => '0f764c35',
     'javelin-view-html' => 'fe287620',
     'javelin-view-interpreter' => 'f829edb3',
@@ -783,7 +781,7 @@ return array(
     'phui-info-panel-css' => '27ea50a1',
     'phui-list-view-css' => '53deb25c',
     'phui-object-box-css' => '0d47b3c8',
-    'phui-object-item-list-view-css' => '832c58fe',
+    'phui-object-item-list-view-css' => '10297907',
     'phui-pinboard-view-css' => '3dd4a269',
     'phui-property-list-view-css' => '51480060',
     'phui-remarkup-preview-css' => '19ad512b',
@@ -994,12 +992,9 @@ return array(
       'javelin-stratcom',
       'phabricator-keyboard-shortcut',
     ),
-    '2d66f6ec' => array(
+    '2caa8fb8' => array(
-      'javelin-magical-init',
       'javelin-install',
-      'javelin-util',
+      'javelin-event',
-      'javelin-vector',
-      'javelin-stratcom',
     ),
     '2e0148bc' => array(
       'javelin-stratcom',
@@ -1117,12 +1112,6 @@ return array(
       'javelin-view-renderer',
       'javelin-install',
     ),
-    '479fd9f1' => array(
-      'javelin-install',
-      'javelin-dom',
-      'javelin-stratcom',
-      'javelin-vector',
-    ),
     '47c794d8' => array(
       'javelin-install',
       'javelin-dom',
@@ -1299,6 +1288,13 @@ return array(
       'javelin-util',
       'javelin-stratcom',
     ),
+    '6f7962d5' => array(
+      'javelin-magical-init',
+      'javelin-install',
+      'javelin-util',
+      'javelin-vector',
+      'javelin-stratcom',
+    ),
     '6f7a9da8' => array(
       'javelin-install',
     ),
@@ -1498,12 +1494,6 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
-    '924cdd01' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-vector',
-    ),
     '92eb531d' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1546,6 +1536,12 @@ return array(
       'phabricator-drag-and-drop-file-upload',
       'phabricator-draggable-list',
     ),
+    '9f36c42d' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-vector',
+    ),
     '9f7309fb' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1690,12 +1686,6 @@ return array(
       'phabricator-tooltip',
       'changeset-view-manager',
     ),
-    'bde958eb' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'phabricator-busy',
-    ),
     'be807912' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1730,10 +1720,6 @@ return array(
       'javelin-stratcom',
       'phabricator-phtize',
     ),
-    'cc1bd0b0' => array(
-      'javelin-install',
-      'javelin-event',
-    ),
     'd19198c8' => array(
       'javelin-install',
       'javelin-dom',
@@ -1767,6 +1753,12 @@ return array(
       'javelin-util',
       'phabricator-shaped-request',
     ),
+    'dbbf48b6' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'phabricator-busy',
+    ),
     'dd7e8ef5' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1869,6 +1861,12 @@ return array(
       'phabricator-phtize',
       'javelin-dom',
     ),
+    'ef2ec0c6' => array(
+      'javelin-install',
+      'javelin-dom',
+      'javelin-stratcom',
+      'javelin-vector',
+    ),
     'efe49472' => array(
       'javelin-install',
       'javelin-util',

webroot/rsrc/externals/javelin/lib/DOM.js

@@ -980,7 +980,8 @@ JX.install('DOM', {
      * @return void
      */
     scrollTo : function(node) {
-      JX.DOM.scrollToPosition(0, JX.$V(node).y);
+      var pos = JX.Vector.getPosWithScroll(node);
+      JX.DOM.scrollToPosition(0, pos.y);
     },
 
     /**

webroot/rsrc/externals/javelin/lib/Scrollbar.js

@@ -160,7 +160,12 @@ JX.install('Scrollbar', {
           var link = JX.$N('a', {href: '#', className: 'jx-scrollbar-link'});
           JX.DOM.listen(link, 'blur', null, function() {
             // When the user clicks anything else, remove this.
+            try {
               JX.DOM.remove(link);
+            } catch (ignored) {
+              // We can get a second blur event, likey related to T447.
+              // Fix doesn't seem trivial so just ignore it.
+            }
           });
           JX.DOM.listen(link, 'click', null, function(e) {
             // Don't respond to clicks. Since the link isn't visible, this

webroot/rsrc/externals/javelin/lib/Vector.js

@@ -326,6 +326,18 @@ JX.install('Vector', {
       return new JX.$V(x, y);
     },
 
+
+    /**
+     * Get the sum of a node's position and its parent scroll offsets.
+     *
+     * @param   Node        Node to determine aggregate position for.
+     * @return  JX.Vector   New vector with aggregate position.
+     */
+    getPosWithScroll: function(node) {
+      return JX.$V(node).add(JX.Vector.getAggregateScrollForNode(node));
+    },
+
+
     /**
      * Determine the size of the viewport (basically, the browser window) by
      * building a new vector where the 'x' component corresponds to the width

webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js

@@ -29,13 +29,6 @@ JX.behavior('phabricator-show-older-transactions', function(config) {
   function check_hash() {
     if (hash_is_hidden()) {
       load_older(load_hidden_hash_callback);
-    } else {
-      try {
-        var target = JX.$(get_hash());
-        JX.DOM.scrollTo(target);
-      } catch (ignored) {
-        // We did our best.
-      }
     }
   }
 
@@ -71,7 +64,9 @@ JX.behavior('phabricator-show-older-transactions', function(config) {
 
   var load_hidden_hash_callback = function(swap, r) {
     show_older(swap, r);
-    check_hash();
+
+    // We aren't actually doing a scroll position because
+    // `behavior-watch-anchor` will handle that for us.
   };
 
   var load_all_older_callback = function(swap, r) {
@@ -102,7 +97,6 @@ JX.behavior('phabricator-show-older-transactions', function(config) {
       JX.Router.getInstance().queue(routable);
     });
 
-  JX.Stratcom.listen('hashchange', null, check_hash);
   check_hash();
 
   new JX.KeyboardShortcut(['@'], 'Show all older changes in the timeline.')

webroot/rsrc/js/core/behavior-watch-anchor.js

@@ -39,7 +39,9 @@ JX.behavior('phabricator-watch-anchor', function() {
       var n = 50;
       var try_anchor_again = function () {
         try {
-          JX.DOM.scrollToPosition(0, JX.$V(JX.$(anchor)).y - 60);
+          var node = JX.$(anchor);
+          var pos = JX.Vector.getPosWithScroll(node);
+          JX.DOM.scrollToPosition(0, pos.y - 60);
           defer_highlight();
         } catch (e) {
           if (n--) {