From 8e5a83a0568316020450d6f738801918cb2e55e3 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 30 Mar 2017 18:02:33 +0100 Subject: [PATCH 1/6] Reduce number of unpaginated events by 1 When unpaginating in the backwards direction --- src/components/structures/TimelinePanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index ffeb5b9a1f..0a7a2c90b7 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -263,7 +263,7 @@ var TimelinePanel = React.createClass({ } ); - let count = backwards ? marker + 1 : this.state.events.length - marker; + let count = backwards ? marker : this.state.events.length - marker; if (count > 0) { debuglog("TimelinePanel: Unpaginating", count, "in direction", dir); From 94fe9999db265589cc5dfeb0e7e7723e220c7abc Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 4 Apr 2017 11:55:53 +0100 Subject: [PATCH 2/6] Reimplement _saveScrollState The actual fix to https://github.com/vector-im/riot-web/issues/3175 is this change to `_saveScrollState`, which is to pick the trackedScrollToken based on which node is intersected by the bottom of the scroll panel. This is opposed to the previous logic that picked based on which node was the first from the bottom to be above the bottom of the viewport. In the case where the viewport bottom does not intersect any events, the topmost event is used. --- src/components/structures/ScrollPanel.js | 31 +++++++++++++++------- src/components/structures/TimelinePanel.js | 2 +- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 44176f73af..7460d6dac8 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -589,24 +589,35 @@ module.exports = React.createClass({ var itemlist = this.refs.itemlist; var wrapperRect = ReactDOM.findDOMNode(this).getBoundingClientRect(); var messages = itemlist.children; + let newScrollState = null; for (var i = messages.length-1; i >= 0; --i) { var node = messages[i]; if (!node.dataset.scrollToken) continue; var boundingRect = node.getBoundingClientRect(); - if (boundingRect.bottom < wrapperRect.bottom) { - this.scrollState = { - stuckAtBottom: false, - trackedScrollToken: node.dataset.scrollToken, - pixelOffset: wrapperRect.bottom - boundingRect.bottom, - }; - debuglog("ScrollPanel: saved scroll state", this.scrollState); - return; + newScrollState = { + stuckAtBottom: false, + trackedScrollToken: node.dataset.scrollToken, + pixelOffset: wrapperRect.bottom - boundingRect.bottom, + }; + // If the bottom of the panel intersects the ClientRect of node, use this node + // as the scrollToken. + // If this is false for the entire for-loop, we default to the last node + // (which is why newScrollState is set on every iteration). + if (boundingRect.top < wrapperRect.bottom && + wrapperRect.bottom < boundingRect.bottom) { + // Use this node as the scrollToken + break; } } - - debuglog("ScrollPanel: unable to save scroll state: found no children in the viewport"); + // This is only false if there were no nodes with `node.dataset.scrollToken` set. + if (newScrollState) { + this.scrollState = newScrollState; + debuglog("ScrollPanel: saved scroll state", this.scrollState); + } else { + debuglog("ScrollPanel: unable to save scroll state: found no children in the viewport"); + } }, _restoreSavedScrollState: function() { diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 0a7a2c90b7..ffeb5b9a1f 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -263,7 +263,7 @@ var TimelinePanel = React.createClass({ } ); - let count = backwards ? marker : this.state.events.length - marker; + let count = backwards ? marker + 1 : this.state.events.length - marker; if (count > 0) { debuglog("TimelinePanel: Unpaginating", count, "in direction", dir); From 573799495765c50a64bc69dd74e1911e67d5e059 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 4 Apr 2017 13:28:26 +0100 Subject: [PATCH 3/6] Clarify and simplfiy unpagination logic --- src/components/structures/ScrollPanel.js | 40 +++++++++------------- src/components/structures/TimelinePanel.js | 4 ++- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 7460d6dac8..cff6a86b2c 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -333,34 +333,28 @@ module.exports = React.createClass({ if (excessHeight <= 0) { return; } - var itemlist = this.refs.itemlist; - var tiles = itemlist.children; + const tiles = this.refs.itemlist.children; // The scroll token of the first/last tile to be unpaginated let markerScrollToken = null; - // Subtract clientHeights to simulate the events being unpaginated whilst counting - // the events to be unpaginated. - if (backwards) { - // Iterate forwards from start of tiles, subtracting event tile height - let i = 0; - while (i < tiles.length && excessHeight > tiles[i].clientHeight) { - excessHeight -= tiles[i].clientHeight; - if (tiles[i].dataset.scrollToken) { - markerScrollToken = tiles[i].dataset.scrollToken; - } - i++; - } - } else { - // Iterate backwards from end of tiles, subtracting event tile height - let i = tiles.length - 1; - while (i > 0 && excessHeight > tiles[i].clientHeight) { - excessHeight -= tiles[i].clientHeight; - if (tiles[i].dataset.scrollToken) { - markerScrollToken = tiles[i].dataset.scrollToken; - } - i--; + // Subtract heights of tiles to simulate the tiles being unpaginated until the + // excess height is less than the height of the next tile to subtract. This + // prevents excessHeight becoming negative, which could lead to future + // pagination. + // + // If backwards is true, we unpaginate (remove) tiles from the back (top). + let i = backwards ? 0 : tiles.length - 1; + while ( + (backwards ? i < tiles.length : i > 0) && excessHeight > tiles[i].clientHeight + ) { + // Subtract height of tile as if it were unpaginated + excessHeight -= tiles[i].clientHeight; + // The tile may not have a scroll token, so guard it + if (tiles[i].dataset.scrollToken) { + markerScrollToken = tiles[i].dataset.scrollToken; } + i += backwards ? 1 : -1; } if (markerScrollToken) { diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index ffeb5b9a1f..fe4a2f46fa 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -251,10 +251,12 @@ var TimelinePanel = React.createClass({ }, onMessageListUnfillRequest: function(backwards, scrollToken) { + // If backwards, unpaginate from the back let dir = backwards ? EventTimeline.BACKWARDS : EventTimeline.FORWARDS; debuglog("TimelinePanel: unpaginating events in direction", dir); - // All tiles are inserted by MessagePanel to have a scrollToken === eventId + // All tiles are inserted by MessagePanel to have a scrollToken === eventId, and + // this particular event should be the first or last to be unpaginated. let eventId = scrollToken; let marker = this.state.events.findIndex( From 47f29b945441cc4ab8367b1cde5ef8a1d5291c27 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 5 Apr 2017 17:48:24 +0100 Subject: [PATCH 4/6] Simplify simulated unfill --- src/components/structures/ScrollPanel.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index cff6a86b2c..3575d69b3f 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -344,17 +344,17 @@ module.exports = React.createClass({ // pagination. // // If backwards is true, we unpaginate (remove) tiles from the back (top). - let i = backwards ? 0 : tiles.length - 1; - while ( - (backwards ? i < tiles.length : i > 0) && excessHeight > tiles[i].clientHeight - ) { + for (let i = 0; i < tiles.length; i++) { + const tile = tiles[backwards ? tiles.length - 1 - i : i]; // Subtract height of tile as if it were unpaginated - excessHeight -= tiles[i].clientHeight; + excessHeight -= tile.clientHeight; // The tile may not have a scroll token, so guard it - if (tiles[i].dataset.scrollToken) { - markerScrollToken = tiles[i].dataset.scrollToken; + if (tile.dataset.scrollToken) { + markerScrollToken = tile.dataset.scrollToken; + } + if (tile.clientHeight > excessHeight) { + break; } - i += backwards ? 1 : -1; } if (markerScrollToken) { From 423babdb176d953989d485e89604c6da79303c12 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 5 Apr 2017 17:51:07 +0100 Subject: [PATCH 5/6] Remove fairly redundant condition Making sure that a node is intersected by the bottom of the wrapper is a bit overkill, given that we iterate from the bottom. This also prevents the scenario of having no nodes that are not precisely intersected, but possibly straddling the bottom of the wrapper. --- src/components/structures/ScrollPanel.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 3575d69b3f..63725b8b86 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -599,8 +599,7 @@ module.exports = React.createClass({ // as the scrollToken. // If this is false for the entire for-loop, we default to the last node // (which is why newScrollState is set on every iteration). - if (boundingRect.top < wrapperRect.bottom && - wrapperRect.bottom < boundingRect.bottom) { + if (boundingRect.top < wrapperRect.bottom) { // Use this node as the scrollToken break; } From b0a04e6f007f516168eec33093acb29c947a3408 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 5 Apr 2017 17:52:05 +0100 Subject: [PATCH 6/6] Clarify comment --- src/components/structures/TimelinePanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index fe4a2f46fa..8cd820c284 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -251,7 +251,7 @@ var TimelinePanel = React.createClass({ }, onMessageListUnfillRequest: function(backwards, scrollToken) { - // If backwards, unpaginate from the back + // If backwards, unpaginate from the back (i.e. the start of the timeline) let dir = backwards ? EventTimeline.BACKWARDS : EventTimeline.FORWARDS; debuglog("TimelinePanel: unpaginating events in direction", dir);