From eaa883cf37e354bbfe192ff64a515439ecf609ef Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Jan 2015 08:26:10 -0800 Subject: [PATCH] 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 | 88 +++++++++---------- webroot/rsrc/externals/javelin/lib/DOM.js | 3 +- .../rsrc/externals/javelin/lib/Scrollbar.js | 7 +- webroot/rsrc/externals/javelin/lib/Vector.js | 12 +++ .../behavior-show-older-transactions.js | 12 +-- webroot/rsrc/js/core/behavior-watch-anchor.js | 4 +- 6 files changed, 69 insertions(+), 57 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0d8a5e0d08..3d712abfc1 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,8 +7,8 @@ */ return array( 'names' => array( - 'core.pkg.css' => '04a24e98', - 'core.pkg.js' => '7a54aa14', + 'core.pkg.css' => '3d6955ad', + '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( - 'javelin-magical-init', + '2caa8fb8' => array( 'javelin-install', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', + 'javelin-event', ), '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', diff --git a/webroot/rsrc/externals/javelin/lib/DOM.js b/webroot/rsrc/externals/javelin/lib/DOM.js index 0c6d1bb1d6..420f9afe79 100644 --- a/webroot/rsrc/externals/javelin/lib/DOM.js +++ b/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); }, /** diff --git a/webroot/rsrc/externals/javelin/lib/Scrollbar.js b/webroot/rsrc/externals/javelin/lib/Scrollbar.js index f0373e58ca..37d6c166fb 100644 --- a/webroot/rsrc/externals/javelin/lib/Scrollbar.js +++ b/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. - JX.DOM.remove(link); + 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 diff --git a/webroot/rsrc/externals/javelin/lib/Vector.js b/webroot/rsrc/externals/javelin/lib/Vector.js index 2ef915ec8b..32cb29eb40 100644 --- a/webroot/rsrc/externals/javelin/lib/Vector.js +++ b/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 diff --git a/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js b/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js index fbdde1a773..5fe1caabea 100644 --- a/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js +++ b/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.') diff --git a/webroot/rsrc/js/core/behavior-watch-anchor.js b/webroot/rsrc/js/core/behavior-watch-anchor.js index f21d17eb80..49cac764ae 100644 --- a/webroot/rsrc/js/core/behavior-watch-anchor.js +++ b/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--) {