From 37593c117a9a18ceb91b7de2975e1b573d2a26f9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 1 Mar 2019 16:08:41 +0100 Subject: [PATCH 1/2] Revert "remove fix for old chrome bug" This reverts commit ecb074862ecfdf8a9c866d2c32438c2efc91111c. --- src/components/structures/ScrollPanel.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 2dbc711067..e9f0d12988 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -215,6 +215,24 @@ 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 65807c7a6655bd322332d69785d134dcab18348d Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 1 Mar 2019 16:08:58 +0100 Subject: [PATCH 2/2] Revert "remove test for #528 as we removed that fix" This reverts commit 42030796c735ad913d20d95e4028f4a86a8c7f4c. --- .../components/structures/ScrollPanel-test.js | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/test/components/structures/ScrollPanel-test.js b/test/components/structures/ScrollPanel-test.js index 6bdbcdb247..0e091cdddf 100644 --- a/test/components/structures/ScrollPanel-test.js +++ b/test/components/structures/ScrollPanel-test.js @@ -224,4 +224,57 @@ 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); + }); });