Merge pull request #2676 from matrix-org/bwindels/improvedscrolling

Improved scrolling & pagination

scripts/travis/unit-tests.sh

@@ -7,4 +7,4 @@
 set -ev
 
 scripts/travis/build.sh
-npm run test
+CHROME_BIN='/usr/bin/google-chrome-stable' npm run test

src/components/structures/MessagePanel.js

@@ -635,9 +635,9 @@ module.exports = React.createClass({
     _onTypingVisible: function() {
         const scrollPanel = this.refs.scrollPanel;
         if (scrollPanel && scrollPanel.getScrollState().stuckAtBottom) {
-            scrollPanel.blockShrinking();
             // scroll down if at bottom
             scrollPanel.checkScroll();
+            scrollPanel.blockShrinking();
         }
     },
 
@@ -648,12 +648,23 @@ module.exports = React.createClass({
             const isAtBottom = scrollPanel.isAtBottom();
             const whoIsTyping = this.refs.whoIsTyping;
             const isTypingVisible = whoIsTyping && whoIsTyping.isVisible();
+            // when messages get added to the timeline,
+            // but somebody else is still typing,
+            // update the min-height, so once the last
+            // person stops typing, no jumping occurs
             if (isAtBottom && isTypingVisible) {
                 scrollPanel.blockShrinking();
             }
         }
     },
 
+    clearTimelineHeight: function() {
+        const scrollPanel = this.refs.scrollPanel;
+        if (scrollPanel) {
+            scrollPanel.clearBlockShrinking();
+        }
+    },
+
     onResize: function() {
         dis.dispatch({ action: 'timeline_resize' }, true);
     },

src/components/structures/ScrollPanel.js

@@ -78,6 +78,27 @@ if (DEBUG_SCROLL) {
  * scroll down further. If stickyBottom is disabled, we just save the scroll
  * offset as normal.
  */
+
+
+function createTimelineResizeDetector(scrollNode, itemlist, callback) {
+    if (typeof ResizeObserver !== "undefined") {
+        const ro = new ResizeObserver(callback);
+        ro.observe(itemlist);
+        return ro;
+    } else if (typeof IntersectionObserver !== "undefined") {
+        const threshold = [];
+        for (let i = 0; i <= 1000; ++i) {
+            threshold.push(i / 1000);
+        }
+        const io = new IntersectionObserver(
+            callback,
+            {root: scrollNode, threshold},
+        );
+        io.observe(itemlist);
+        return io;
+    }
+}
+
 module.exports = React.createClass({
     displayName: 'ScrollPanel',
 
@@ -160,6 +181,12 @@ module.exports = React.createClass({
 
     componentDidMount: function() {
         this.checkScroll();
+
+        this._timelineSizeObserver = createTimelineResizeDetector(
+            this._getScrollNode(),
+            this.refs.itemlist,
+            () => { this._restoreSavedScrollState(); },
+        );
     },
 
     componentDidUpdate: function() {
@@ -169,10 +196,6 @@ module.exports = React.createClass({
         //
         // This will also re-check the fill state, in case the paginate was inadequate
         this.checkScroll();
-
-        if (!this.isAtBottom()) {
-            this.clearBlockShrinking();
-        }
     },
 
     componentWillUnmount: function() {
@@ -181,6 +204,10 @@ module.exports = React.createClass({
         //
         // (We could use isMounted(), but facebook have deprecated that.)
         this.unmounted = true;
+        if (this._timelineSizeObserver) {
+            this._timelineSizeObserver.disconnect();
+            this._timelineSizeObserver = null;
+        }
     },
 
     onScroll: function(ev) {
@@ -188,46 +215,21 @@ module.exports = React.createClass({
         debuglog("Scroll event: offset now:", sn.scrollTop,
                  "_lastSetScroll:", this._lastSetScroll);
 
-        // Sometimes we see attempts to write to scrollTop essentially being
-        // ignored. (Or rather, it is successfully written, but on the next
-        // scroll event, it's been reset again).
-        //
-        // This was observed on Chrome 47, when scrolling using the trackpad in OS
-        // X Yosemite.  Can't reproduce on El Capitan. Our theory is that this is
-        // due to Chrome not being able to cope with the scroll offset being reset
-        // while a two-finger drag is in progress.
-        //
-        // By way of a workaround, we detect this situation and just keep
-        // resetting scrollTop until we see the scroll node have the right
-        // value.
-        if (this._lastSetScroll !== undefined && sn.scrollTop < this._lastSetScroll-200) {
-            console.log("Working around vector-im/vector-web#528");
-            this._restoreSavedScrollState();
-            return;
-        }
-
         // If there weren't enough children to fill the viewport, the scroll we
         // got might be different to the scroll we wanted; we don't want to
         // forget what we wanted, so don't overwrite the saved state unless
         // this appears to be a user-initiated scroll.
         if (sn.scrollTop != this._lastSetScroll) {
-            // when scrolling, we don't care about disappearing typing notifs shrinking the timeline
-            // this might cause the scrollbar to resize in case the max-height was not correct
-            // but that's better than ending up with a lot of whitespace at the bottom of the timeline.
-            // we need to above check because when showing the typing notifs, an onScroll event is also triggered
-            if (!this.isAtBottom()) {
-                this.clearBlockShrinking();
-            }
-
             this._saveScrollState();
         } else {
             debuglog("Ignoring scroll echo");
-
             // only ignore the echo once, otherwise we'll get confused when the
             // user scrolls away from, and back to, the autoscroll point.
             this._lastSetScroll = undefined;
         }
 
+        this._checkBlockShrinking();
+
         this.props.onScroll(ev);
 
         this.checkFillState();
@@ -235,8 +237,6 @@ module.exports = React.createClass({
 
     onResize: function() {
         this.props.onResize();
-        // clear min-height as the height might have changed
-        this.clearBlockShrinking();
         this.checkScroll();
         if (this._gemScroll) this._gemScroll.forceUpdate();
     },
@@ -245,6 +245,7 @@ module.exports = React.createClass({
     // where it ought to be, and set off pagination requests if necessary.
     checkScroll: function() {
         this._restoreSavedScrollState();
+        this._checkBlockShrinking();
         this.checkFillState();
     },
 
@@ -386,8 +387,6 @@ module.exports = React.createClass({
             }
             this._unfillDebouncer = setTimeout(() => {
                 this._unfillDebouncer = null;
-                // if timeline shrinks, min-height should be cleared
-                this.clearBlockShrinking();
                 this.props.onUnfillRequest(backwards, markerScrollToken);
             }, UNFILL_REQUEST_DEBOUNCE_MS);
         }
@@ -583,9 +582,10 @@ module.exports = React.createClass({
         }
 
         const scrollNode = this._getScrollNode();
-        const wrapperRect = ReactDOM.findDOMNode(this).getBoundingClientRect();
+        const scrollBottom = scrollNode.scrollTop + scrollNode.clientHeight;
-        const boundingRect = node.getBoundingClientRect();
+
-        const scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom;
+        const nodeBottom = node.offsetTop + node.clientHeight;
+        const scrollDelta = nodeBottom + pixelOffset - scrollBottom;
 
         debuglog("ScrollPanel: scrolling to token '" + scrollToken + "'+" +
                  pixelOffset + " (delta: "+scrollDelta+")");
@@ -602,42 +602,43 @@ module.exports = React.createClass({
             return;
         }
 
+        const scrollNode = this._getScrollNode();
+        const scrollBottom = scrollNode.scrollTop + scrollNode.clientHeight;
+
         const itemlist = this.refs.itemlist;
-        const wrapperRect = ReactDOM.findDOMNode(this).getBoundingClientRect();
         const messages = itemlist.children;
-        let newScrollState = null;
+        let node = null;
 
+        // loop backwards, from bottom-most message (as that is the most common case)
         for (let i = messages.length-1; i >= 0; --i) {
-            const node = messages[i];
+            if (!messages[i].dataset.scrollTokens) {
-            if (!node.dataset.scrollTokens) continue;
+                continue;
-
+            }
-            const boundingRect = node.getBoundingClientRect();
+            node = messages[i];
-            newScrollState = {
+            // break at the first message (coming from the bottom)
-                stuckAtBottom: false,
+            // that has it's offsetTop above the bottom of the viewport.
-                trackedScrollToken: node.dataset.scrollTokens.split(',')[0],
+            if (node.offsetTop < scrollBottom) {
-                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;
             }
         }
-        // This is only false if there were no nodes with `node.dataset.scrollTokens` set.
+
-        if (newScrollState) {
+        if (!node) {
-            this.scrollState = newScrollState;
-            debuglog("ScrollPanel: saved scroll state", this.scrollState);
-        } else {
             debuglog("ScrollPanel: unable to save scroll state: found no children in the viewport");
+            return;
         }
+
+        const nodeBottom = node.offsetTop + node.clientHeight;
+        debuglog("ScrollPanel: saved scroll state", this.scrollState);
+        this.scrollState = {
+            stuckAtBottom: false,
+            trackedScrollToken: node.dataset.scrollTokens.split(',')[0],
+            pixelOffset: scrollBottom - nodeBottom,
+        };
     },
 
     _restoreSavedScrollState: function() {
         const scrollState = this.scrollState;
-        const scrollNode = this._getScrollNode();
 
         if (scrollState.stuckAtBottom) {
             this._setScrollTop(Number.MAX_VALUE);
@@ -717,6 +718,21 @@ module.exports = React.createClass({
         }
     },
 
+    _checkBlockShrinking: function() {
+        const sn = this._getScrollNode();
+        const scrollState = this.scrollState;
+        if (!scrollState.stuckAtBottom) {
+            const spaceBelowViewport = sn.scrollHeight - (sn.scrollTop + sn.clientHeight);
+            // only if we've scrolled up 200px from the bottom
+            // should we clear the min-height used by the typing notifications,
+            // otherwise we might still see it jump as the whitespace disappears
+            // when scrolling up from the bottom
+            if (spaceBelowViewport >= 200) {
+                this.clearBlockShrinking();
+            }
+        }
+    },
+
     render: function() {
         const GeminiScrollbarWrapper = sdk.getComponent("elements.GeminiScrollbarWrapper");
         // TODO: the classnames on the div and ol could do with being updated to

src/components/structures/TimelinePanel.js

@@ -935,6 +935,11 @@ var TimelinePanel = React.createClass({
             {windowLimit: this.props.timelineCap});
 
         const onLoaded = () => {
+            // clear the timeline min-height when
+            // (re)loading the timeline
+            if (this.refs.messagePanel) {
+                this.refs.messagePanel.clearTimelineHeight();
+            }
             this._reloadEvents();
 
             // If we switched away from the room while there were pending

test/components/structures/ScrollPanel-test.js

@@ -224,57 +224,4 @@ describe('ScrollPanel', function() {
             expect(scrollingDiv.scrollTop).toEqual(1950);
         });
     });
-
-    it('should not get stuck in #528 workaround', function(done) {
-        let events = [];
-        Promise.resolve().then(() => {
-            // initialise with a bunch of events
-            for (let i = 0; i < 40; i++) {
-                events.push(i);
-            }
-            tester.setTileKeys(events);
-            expect(tester.fillCounts.b).toEqual(1);
-            expect(tester.fillCounts.f).toEqual(2);
-            expect(scrollingDiv.scrollHeight).toEqual(6050); // 40*150 + 50
-            expect(scrollingDiv.scrollTop).toEqual(6050 - 600);
-
-            // try to scroll up, to a non-integer offset.
-            tester.scrollPanel().scrollToToken("30", -101/3);
-
-            expect(scrollingDiv.scrollTop).toEqual(4616); // 31*150 - 34
-
-            // wait for the scroll event to land
-            return tester.awaitScroll(); // fails
-        }).then(() => {
-            expect(tester.lastScrollEvent).toEqual(4616);
-
-            // Now one more event; this will make it reset the scroll, but
-            // because the delta will be less than 1, will not trigger a
-            // scroll event, this leaving recentEventScroll defined.
-            console.log("Adding event 50");
-            events.push(50);
-            tester.setTileKeys(events);
-
-            // wait for the scrollpanel to stop trying to paginate
-        }).then(() => {
-            // Now, simulate hitting "scroll to bottom".
-            events = [];
-            for (let i = 100; i < 120; i++) {
-                events.push(i);
-            }
-            tester.setTileKeys(events);
-            tester.scrollPanel().scrollToBottom();
-
-            // wait for the scroll event to land
-            return tester.awaitScroll(); // fails
-        }).then(() => {
-            expect(scrollingDiv.scrollTop).toEqual(20*150 + 50 - 600);
-
-            // simulate a user-initiated scroll on the div
-            scrollingDiv.scrollTop = 1200;
-            return tester.awaitScroll();
-        }).then(() => {
-            expect(scrollingDiv.scrollTop).toEqual(1200);
-        }).done(done);
-    });
 });