From f02aed10e7691daa887182518184641349c66ed3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 9 Sep 2014 14:20:27 -0700 Subject: [PATCH] Fix three issues with scrolling while dragging items (e.g., on workboards) Summary: Fixes T5979. There are three issues here: - We cache document positions when you pick an item up, but don't recalculate them after you scroll, so they get out of date. Dirty the cache when the user scrolls. - When we rebuild the cache during a drag (previously, this never happened), the position of the object you're dragging is computed wrong (since it has been moved to be under the cursor). Adjust the effective position of the object you've picked up to put it back in the right place in the list. - When you fiddle around at the bottom of a column you can get jumpy redraws as the height adjusts. Put `min-height` on the container during a drag to prevent this. Test Plan: In Safari, Chrome and Firefox, dragged items around on columns before and after scrolling the workboard panel. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5979 Differential Revision: https://secure.phabricator.com/D10455 --- resources/celerity/map.php | 43 +++++++------ src/view/layout/AphrontMultiColumnView.php | 5 +- .../projects/behavior-project-boards.js | 39 +++++++++++ webroot/rsrc/js/core/DraggableList.js | 64 +++++++++++++++---- 4 files changed, 117 insertions(+), 34 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 5c6f65b8a3..6acdeb574b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,7 +8,7 @@ return array( 'names' => array( 'core.pkg.css' => '974635bb', - 'core.pkg.js' => '47fd11f0', + 'core.pkg.js' => 'f0e2c091', 'darkconsole.pkg.js' => 'df001cab', 'differential.pkg.css' => '36884139', 'differential.pkg.js' => '73337d1d', @@ -412,7 +412,7 @@ return array( 'rsrc/js/application/policy/behavior-policy-rule-editor.js' => 'fe9a552f', 'rsrc/js/application/ponder/behavior-votebox.js' => '4e9b766b', 'rsrc/js/application/projects/behavior-boards-dropdown.js' => '0ec56e1d', - 'rsrc/js/application/projects/behavior-project-boards.js' => 'a6c6a058', + 'rsrc/js/application/projects/behavior-project-boards.js' => '0676345e', 'rsrc/js/application/projects/behavior-project-create.js' => '065227cc', 'rsrc/js/application/projects/behavior-reorder-columns.js' => 'e1d25dfb', 'rsrc/js/application/releeph/releeph-preview-branch.js' => 'b2b4fbaf', @@ -438,7 +438,7 @@ return array( 'rsrc/js/application/uiexample/notification-example.js' => '7a9677fc', 'rsrc/js/core/Busy.js' => '6453c869', 'rsrc/js/core/DragAndDropFileUpload.js' => '8c49f386', - 'rsrc/js/core/DraggableList.js' => '2a6a1041', + 'rsrc/js/core/DraggableList.js' => 'a16ec1c6', 'rsrc/js/core/FileUpload.js' => 'a4ae61bf', 'rsrc/js/core/Hovercard.js' => '7e8468ae', 'rsrc/js/core/KeyboardShortcut.js' => '1ae869f2', @@ -636,7 +636,7 @@ return array( 'javelin-behavior-policy-control' => 'f3fef818', 'javelin-behavior-policy-rule-editor' => 'fe9a552f', 'javelin-behavior-ponder-votebox' => '4e9b766b', - 'javelin-behavior-project-boards' => 'a6c6a058', + 'javelin-behavior-project-boards' => '0676345e', 'javelin-behavior-project-create' => '065227cc', 'javelin-behavior-refresh-csrf' => '7814b593', 'javelin-behavior-releeph-preview-branch' => 'b2b4fbaf', @@ -713,7 +713,7 @@ return array( 'phabricator-crumbs-view-css' => 'a49339de', 'phabricator-dashboard-css' => 'a2bfdcbf', 'phabricator-drag-and-drop-file-upload' => '8c49f386', - 'phabricator-draggable-list' => '2a6a1041', + 'phabricator-draggable-list' => 'a16ec1c6', 'phabricator-fatal-config-template-css' => '25d446d6', 'phabricator-feed-css' => '7bfc6f12', 'phabricator-file-upload' => 'a4ae61bf', @@ -851,6 +851,15 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), + '0676345e' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + ), '06e05112' => array( 'javelin-behavior', 'javelin-stratcom', @@ -987,14 +996,6 @@ return array( 'javelin-install', 'javelin-util', ), - '2a6a1041' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'javelin-vector', - 'javelin-magical-init', - ), '2b228192' => array( 'javelin-behavior', 'javelin-dom', @@ -1477,6 +1478,14 @@ return array( 'javelin-dom', 'javelin-reactor-dom', ), + 'a16ec1c6' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'javelin-vector', + 'javelin-magical-init', + ), 'a4ae61bf' => array( 'javelin-install', 'javelin-dom', @@ -1485,14 +1494,6 @@ return array( 'a5d7cf86' => array( 'javelin-dom', ), - 'a6c6a058' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - ), 'a80d0378' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/view/layout/AphrontMultiColumnView.php b/src/view/layout/AphrontMultiColumnView.php index 07997c6ee3..a5fa4d99d8 100644 --- a/src/view/layout/AphrontMultiColumnView.php +++ b/src/view/layout/AphrontMultiColumnView.php @@ -139,11 +139,14 @@ final class AphrontMultiColumnView extends AphrontView { ->addPadding(PHUI::PADDING_MEDIUM_BOTTOM); } - return phutil_tag( + return javelin_tag( 'div', array( 'class' => 'aphront-multi-column-view', 'id' => $this->getID(), + // TODO: It would be nice to convert this to an AphrontTagView and + // use addSigil() from Workboards instead of hard-coding this. + 'sigil' => 'aphront-multi-column-view', ), $board); } diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index 21fb4735c9..0f405303e6 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -3,6 +3,7 @@ * @requires javelin-behavior * javelin-dom * javelin-util + * javelin-vector * javelin-stratcom * javelin-workflow * phabricator-draggable-list @@ -88,6 +89,41 @@ JX.behavior('project-boards', function(config) { return 0; } + function getcontainer() { + return JX.DOM.find( + JX.$(config.boardID), + 'div', + 'aphront-multi-column-view'); + } + + function onbegindrag(item) { + // If the longest column on the board is taller than the window, the board + // will scroll vertically. Dragging an item to the longest column may + // make it longer, by the total height of the board, plus the height of + // the drop target. + + // If this happens, the scrollbar will jump around and the scroll position + // can be adjusted in a disorienting way. To reproduce this, drag a task + // to the bottom of the longest column on a scrolling board and wave the + // task in and out of the column. The scroll bar will jump around and + // it will be hard to lock onto a target. + + // To fix this, set the minimum board height to the current board height + // plus the size of the drop target (which is the size of the item plus + // a bit of margin). This makes sure the scroll bar never needs to + // recalculate. + + var item_size = JX.Vector.getDim(item); + var container = getcontainer(); + var container_size = JX.Vector.getDim(container); + + container.style.minHeight = (item_size.y + container_size.y + 12) + 'px'; + } + + function onenddrag() { + getcontainer().style.minHeight = ''; + } + function ondrop(list, item, after) { list.lock(); JX.DOM.alterClass(item, 'drag-sending', true); @@ -151,6 +187,9 @@ JX.behavior('project-boards', function(config) { list.listen('didDrop', JX.bind(null, ondrop, list)); + list.listen('didBeginDrag', JX.bind(null, onbegindrag)); + list.listen('didEndDrag', JX.bind(null, onenddrag)); + lists.push(list); onupdate(cols[ii]); diff --git a/webroot/rsrc/js/core/DraggableList.js b/webroot/rsrc/js/core/DraggableList.js index 5ae0150ad5..1f8f50c7aa 100644 --- a/webroot/rsrc/js/core/DraggableList.js +++ b/webroot/rsrc/js/core/DraggableList.js @@ -53,6 +53,7 @@ JX.install('DraggableList', { _ghostNode : null, _group : null, _lastMousePosition: null, + _lastAdjust: null, getRootNode : function() { return this._root; @@ -174,7 +175,6 @@ JX.install('DraggableList', { for (var ii = 0; ii < this._group.length; ii++) { this._group[ii]._clearTarget(); - this._group[ii]._generateTargets(); } if (!this.invoke('didBeginDrag', this._dragging).getPrevented()) { @@ -189,17 +189,48 @@ JX.install('DraggableList', { } }, - _generateTargets : function() { - var targets = []; - var items = this.findItems(); - for (var ii = 0; ii < items.length; ii++) { - targets.push({ - item: items[ii], - y: JX.$V(items[ii]).y + (JX.Vector.getDim(items[ii]).y / 2) - }); + _getTargets : function() { + if (this._targets === null) { + var targets = []; + var items = this.findItems(); + for (var ii = 0; ii < items.length; ii++) { + var item = items[ii]; + + var ipos = JX.$V(item); + if (item == this._dragging) { + // If the item we're measuring is also the item we're dragging, + // we need to measure its position as though it was still in the + // list, not its current position in the document (which is + // under the cursor). To do this, adjust the measured position by + // removing the offsets we added to put the item underneath the + // cursor. + if (this._lastAdjust) { + ipos.x -= this._lastAdjust.x; + ipos.y -= this._lastAdjust.y; + } + } + + targets.push({ + item: items[ii], + y: ipos.y + (JX.Vector.getDim(items[ii]).y / 2) + }); + } + targets.sort(function(u, v) { return v.y - u.y; }); + this._targets = targets; + } + + return this._targets; + }, + + _dirtyTargetCache: function() { + if (this._hasGroup()) { + var group = this._group; + for (var ii = 0; ii < group.length; ii++) { + group[ii]._targets = null; + } + } else { + this._targets = null; } - targets.sort(function(u, v) { return v.y - u.y; }); - this._targets = targets; return this; }, @@ -264,7 +295,7 @@ JX.install('DraggableList', { _getCurrentTarget : function(p) { var ghost = this.getGhostNode(); - var targets = this._targets; + var targets = this._getTargets(); var dragging = this._dragging; var adjust_h = JX.Vector.getDim(ghost).y; @@ -348,6 +379,12 @@ JX.install('DraggableList', { return; } + if (e.getType() == 'scroll') { + // If this is a scroll event, the positions of drag targets may have + // changed. + this._dirtyTargetCache(); + } + var p = JX.$V(this._lastMousePosition.x, this._lastMousePosition.y); var group = this._group; @@ -402,6 +439,7 @@ JX.install('DraggableList', { } p.y -= origin.y; + this._lastAdjust = new JX.Vector(p.x, p.y); p.setPos(this._dragging); e.kill(); @@ -442,6 +480,8 @@ JX.install('DraggableList', { for (var ii = 0; ii < group.length; ii++) { JX.DOM.alterClass(group[ii].getRootNode(), 'drag-target-list', false); group[ii]._clearTarget(); + group[ii]._dirtyTargetCache(); + group[ii]._lastAdjust = null; } if (!this.invoke('didEndDrag', dragging).getPrevented()) {