Merge pull request #785 from matrix-org/luke/fix-scroll-past-big-event

Fix scroll token selection logic

Author: lukebarnard1

src/components/structures/ScrollPanel.js

@@ -333,33 +333,27 @@ 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++;
+        // 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).
+        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 -= tile.clientHeight;
+            // The tile may not have a scroll token, so guard it
+            if (tile.dataset.scrollToken) {
+                markerScrollToken = tile.dataset.scrollToken;
             }
-        } 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--;
+            if (tile.clientHeight > excessHeight) {
+                break;
             }
         }
 
@@ -589,24 +583,34 @@ 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) {
+                // 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() {

src/components/structures/TimelinePanel.js

@@ -251,10 +251,12 @@ var TimelinePanel = React.createClass({
     },
 
     onMessageListUnfillRequest: function(backwards, scrollToken) {
+        // 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);
 
-        // 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(