From b3e6cbfddd7115bfd9aa1d44fd6535bba3ec2883 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 21 Feb 2019 18:51:18 +0100 Subject: [PATCH 01/13] restore scroll state when timeline resizes using ResizeObserver (only where supported, polyfill doesn't give good results) --- src/components/structures/ScrollPanel.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index a1a7d08e0b3..db19b1d0ccd 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -160,6 +160,13 @@ module.exports = React.createClass({ componentDidMount: function() { this.checkScroll(); + + if (typeof ResizeObserver !== "undefined") { + this._timelineSizeObserver = new ResizeObserver(() => { + this._restoreSavedScrollState(); + }); + this._timelineSizeObserver.observe(this.refs.itemlist); + } }, componentDidUpdate: function() { @@ -181,6 +188,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) { From ecb074862ecfdf8a9c866d2c32438c2efc91111c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 21 Feb 2019 18:51:58 +0100 Subject: [PATCH 02/13] remove fix for old chrome bug --- src/components/structures/ScrollPanel.js | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index db19b1d0ccd..55ac753edb6 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -199,24 +199,6 @@ 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 From 03784e586c7416fdc03670bc076ecee6cbb89c37 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 21 Feb 2019 19:28:55 +0100 Subject: [PATCH 03/13] replace getBoundingClientRect() with offset/scrollTop & clientHeight as they are an order of magnitude faster in most browsers, getBoundingClientRect() tends to cause relayout. --- src/components/structures/ScrollPanel.js | 47 ++++++++++++++---------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 55ac753edb6..1433baeeb98 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -576,9 +576,10 @@ module.exports = React.createClass({ } const scrollNode = this._getScrollNode(); - const wrapperRect = ReactDOM.findDOMNode(this).getBoundingClientRect(); - const boundingRect = node.getBoundingClientRect(); - const scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom; + const scrollBottom = scrollNode.scrollTop + scrollNode.clientHeight; + + const nodeBottom = node.offsetTop + node.clientHeight; + const scrollDelta = nodeBottom + pixelOffset - scrollBottom; debuglog("ScrollPanel: scrolling to token '" + scrollToken + "'+" + pixelOffset + " (delta: "+scrollDelta+")"); @@ -595,37 +596,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; + let nodeBottom; + // TODO: change this to not use getBoundingClientRect and save the domnode for (let i = messages.length-1; i >= 0; --i) { - const node = messages[i]; - if (!node.dataset.scrollTokens) continue; - - const boundingRect = node.getBoundingClientRect(); - newScrollState = { - stuckAtBottom: false, - trackedScrollToken: node.dataset.scrollTokens.split(',')[0], - pixelOffset: wrapperRect.bottom - boundingRect.bottom, - }; + if (!messages[i].dataset.scrollTokens) { + continue; + } + node = messages[i]; + + nodeBottom = node.offsetTop + node.clientHeight; // 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) { + if (nodeBottom >= scrollBottom) { // Use this node as the scrollToken break; } } - // This is only false if there were no nodes with `node.dataset.scrollTokens` set. - if (newScrollState) { - this.scrollState = newScrollState; - debuglog("ScrollPanel: saved scroll state", this.scrollState); - } else { + + if (!node) { debuglog("ScrollPanel: unable to save scroll state: found no children in the viewport"); + return; } + + debuglog("ScrollPanel: saved scroll state", this.scrollState); + this.scrollState = { + stuckAtBottom: false, + trackedScrollToken: node.dataset.scrollTokens.split(',')[0], + pixelOffset: scrollBottom - nodeBottom, + }; }, _restoreSavedScrollState: function() { From 41ae618d0ea7a56e9bcf3d88827aaa269f2b3f63 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Feb 2019 13:12:52 +0100 Subject: [PATCH 04/13] only clear min-height on scroll & adding items (componentDidUpdate) before we would clear it as soon as you were 1px away from the bottom of the timeline, which would still create jumping as the whitespace would around 36px. To play it safe, we only clear it after moving 200px from the bottom. Also include "local echo" scroll events, caused by setting scrollTop --- src/components/structures/ScrollPanel.js | 35 ++++++++++++------------ 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 1433baeeb98..154f7dfc12b 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -176,10 +176,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() { @@ -204,23 +200,16 @@ module.exports = React.createClass({ // 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(); @@ -228,8 +217,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(); }, @@ -238,6 +225,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(); }, @@ -379,8 +367,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); } @@ -717,6 +703,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 From 38236428630bf0d93628939dcb3912f6a3ea5cd5 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Feb 2019 13:16:16 +0100 Subject: [PATCH 05/13] some cleanup --- src/components/structures/ScrollPanel.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 154f7dfc12b..ecfdfd5db26 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -590,7 +590,6 @@ module.exports = React.createClass({ let node = null; let nodeBottom; - // TODO: change this to not use getBoundingClientRect and save the domnode for (let i = messages.length-1; i >= 0; --i) { if (!messages[i].dataset.scrollTokens) { continue; @@ -623,7 +622,6 @@ module.exports = React.createClass({ _restoreSavedScrollState: function() { const scrollState = this.scrollState; - const scrollNode = this._getScrollNode(); if (scrollState.stuckAtBottom) { this._setScrollTop(Number.MAX_VALUE); From db7203ed7153cc44bad0ccca571bf722d815b3ce Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Feb 2019 14:48:33 +0100 Subject: [PATCH 06/13] make sure the min-height doesn't get cleared by checkScroll --- src/components/structures/MessagePanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 185af4cd6d2..2e8197c1d72 100644 --- a/src/components/structures/MessagePanel.js +++ b/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(); } }, From 32f055bec26424911460b16ef5f14009b291e9c2 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Feb 2019 14:48:56 +0100 Subject: [PATCH 07/13] clarify why we need this --- src/components/structures/MessagePanel.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 2e8197c1d72..291769fa036 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -648,6 +648,10 @@ 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(); } From 8bb8ec141eea4230768c75b2eec3ea32f3c9e9fd Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Feb 2019 16:23:05 +0100 Subject: [PATCH 08/13] clear min-height on timeline resets and other occasions where we load it --- src/components/structures/MessagePanel.js | 7 +++++++ src/components/structures/TimelinePanel.js | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 291769fa036..9034123e527 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -658,6 +658,13 @@ module.exports = React.createClass({ } }, + clearTimelineHeight: function() { + const scrollPanel = this.refs.scrollPanel; + if (scrollPanel) { + scrollPanel.clearBlockShrinking(); + } + }, + onResize: function() { dis.dispatch({ action: 'timeline_resize' }, true); }, diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 8890c26d42a..911499e3149 100644 --- a/src/components/structures/TimelinePanel.js +++ b/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 From ba5f16358f0233811c6e73086573f04c731930cb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Feb 2019 18:30:22 +0100 Subject: [PATCH 09/13] fall back to InteractionObserver for detecting timeline resizes this is not nearly as smooth as using ResizeObserver, as the callback rate is a lot lower, but seems to be quite a bit better than what we have right now, without the 7% cpu hog that the requestAnimationFrame polling fallback has. --- src/components/structures/ScrollPanel.js | 33 +++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index ecfdfd5db26..ecbb5ab8687 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -78,6 +78,28 @@ 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); + } + threshold.push(1); + const io = new IntersectionObserver( + callback, + {root: scrollNode, threshold}, + ); + io.observe(itemlist); + return io; + } +} + module.exports = React.createClass({ displayName: 'ScrollPanel', @@ -161,12 +183,11 @@ module.exports = React.createClass({ componentDidMount: function() { this.checkScroll(); - if (typeof ResizeObserver !== "undefined") { - this._timelineSizeObserver = new ResizeObserver(() => { - this._restoreSavedScrollState(); - }); - this._timelineSizeObserver.observe(this.refs.itemlist); - } + this._timelineSizeObserver = createTimelineResizeDetector( + this._getScrollNode(), + this.refs.itemlist, + () => { this._restoreSavedScrollState(); }, + ); }, componentDidUpdate: function() { From 42030796c735ad913d20d95e4028f4a86a8c7f4c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 25 Feb 2019 18:48:42 +0100 Subject: [PATCH 10/13] remove test for #528 as we removed that fix --- .../components/structures/ScrollPanel-test.js | 53 ------------------- 1 file changed, 53 deletions(-) diff --git a/test/components/structures/ScrollPanel-test.js b/test/components/structures/ScrollPanel-test.js index 0e091cdddf6..6bdbcdb2471 100644 --- a/test/components/structures/ScrollPanel-test.js +++ b/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); - }); }); From c920dd2e8ad56281e9cd0fa3c7e9c04f0898ac0e Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 26 Feb 2019 16:26:24 +0100 Subject: [PATCH 11/13] check top of node instead of bottom, since coming in from last as we're approaching from the last node, if we're scrolled up, the first node we encounter would be below the bottom of the viewport change the logic to stop at the first node that has its top above the viewport bottom. When completely scrolled up, this was causing nodes way below the viewport to be selected as the reference for the pixelOffset, and when pagination came in, it would immediately try to apply the big negative pixelOffset, scrolling to a negative scrollTop, thus going to the top again, and triggering another pagination, entering in an infinite pagination loop until you scrolled down. --- src/components/structures/ScrollPanel.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index ecbb5ab8687..10344a619f5 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -609,20 +609,16 @@ module.exports = React.createClass({ const itemlist = this.refs.itemlist; const messages = itemlist.children; let node = null; - let nodeBottom; + // loop backwards, from bottom-most message (as that is the most common case) for (let i = messages.length-1; i >= 0; --i) { if (!messages[i].dataset.scrollTokens) { continue; } node = messages[i]; - - nodeBottom = node.offsetTop + node.clientHeight; - // 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 (nodeBottom >= scrollBottom) { + // break at the first message (coming from the bottom) + // that has it's offsetTop above the bottom of the viewport. + if (node.offsetTop < scrollBottom) { // Use this node as the scrollToken break; } @@ -633,6 +629,7 @@ module.exports = React.createClass({ return; } + const nodeBottom = node.offsetTop + node.clientHeight; debuglog("ScrollPanel: saved scroll state", this.scrollState); this.scrollState = { stuckAtBottom: false, From 712522a16d960298487542c476c34fd1bc25db33 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 28 Feb 2019 12:04:22 +0100 Subject: [PATCH 12/13] set chrome path for travis CI explicitly karma seems to be giving priority to a location where an old version is installed. --- scripts/travis/unit-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/travis/unit-tests.sh b/scripts/travis/unit-tests.sh index a8e0a63b315..a7e8425fa09 100755 --- a/scripts/travis/unit-tests.sh +++ b/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 From 0c06a702dc75ad542b63aeaf7f27fb485f230f87 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 28 Feb 2019 16:05:55 +0100 Subject: [PATCH 13/13] pr feedback --- 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 10344a619f5..2dbc7110673 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -87,10 +87,9 @@ function createTimelineResizeDetector(scrollNode, itemlist, callback) { return ro; } else if (typeof IntersectionObserver !== "undefined") { const threshold = []; - for (let i = 0; i < 1000; ++i) { + for (let i = 0; i <= 1000; ++i) { threshold.push(i / 1000); } - threshold.push(1); const io = new IntersectionObserver( callback, {root: scrollNode, threshold},