From 51fe77122b61f6497157e5f10aa12136fa5d8d69 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sat, 2 Apr 2016 18:09:44 +0100 Subject: [PATCH 1/2] Fix an issue where the scroll stopped working. Under certain conditions, it was possible to get stuck in a state where any user-initiated scroll would be met with "Working around vector-im/vector-web#528" and overridden. Fix this by removing the duplication between _lastSetScroll and recentEventScroll, and using _lastSetScroll which is more reliable. --- karma.conf.js | 3 + src/components/structures/ScrollPanel.js | 26 +- .../components/structures/ScrollPanel-test.js | 275 ++++++++++++++++++ test/test-utils.js | 31 +- 4 files changed, 301 insertions(+), 34 deletions(-) create mode 100644 test/components/structures/ScrollPanel-test.js diff --git a/karma.conf.js b/karma.conf.js index 89437c203a..dedd30f878 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -26,6 +26,9 @@ module.exports = function (config) { // list of files / patterns to load in the browser files: [ 'test/tests.js', + + // XXX this will break on npm 3 + 'node_modules/react-gemini-scrollbar/node_modules/gemini-scrollbar/gemini-scrollbar.css', ], // list of files to exclude diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index ddec81c107..6825e41cdb 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -20,6 +20,7 @@ var GeminiScrollbar = require('react-gemini-scrollbar'); var q = require("q"); var DEBUG_SCROLL = false; +// var DEBUG_SCROLL = true; if (DEBUG_SCROLL) { // using bind means that we get to keep useful line numbers in the console @@ -144,7 +145,8 @@ module.exports = React.createClass({ onScroll: function(ev) { var sn = this._getScrollNode(); - debuglog("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); + 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 @@ -158,13 +160,10 @@ module.exports = React.createClass({ // 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.recentEventScroll !== undefined) { - if(sn.scrollTop < this.recentEventScroll-200) { - console.log("Working around vector-im/vector-web#528"); - this._restoreSavedScrollState(); - return; - } - this.recentEventScroll = undefined; + 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 @@ -395,17 +394,14 @@ module.exports = React.createClass({ var wrapperRect = ReactDOM.findDOMNode(this).getBoundingClientRect(); var boundingRect = node.getBoundingClientRect(); var scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom; + + debuglog("Scrolling to token '" + node.dataset.scrollToken + "'+" + + pixelOffset + " (delta: "+scrollDelta+")"); + if(scrollDelta != 0) { this._setScrollTop(scrollNode.scrollTop + scrollDelta); - - // see the comments in onScroll regarding recentEventScroll - this.recentEventScroll = scrollNode.scrollTop; } - debuglog("Scrolled to token", node.dataset.scrollToken, "+", - pixelOffset+":", scrollNode.scrollTop, - "(delta: "+scrollDelta+")"); - debuglog("recentEventScroll now "+this.recentEventScroll); }, _saveScrollState: function() { diff --git a/test/components/structures/ScrollPanel-test.js b/test/components/structures/ScrollPanel-test.js new file mode 100644 index 0000000000..13721c9ecd --- /dev/null +++ b/test/components/structures/ScrollPanel-test.js @@ -0,0 +1,275 @@ +/* +Copyright 2016 OpenMarket Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +var React = require('react'); +var ReactDOM = require("react-dom"); +var ReactTestUtils = require('react-addons-test-utils'); +var expect = require('expect'); +var q = require('q'); + +var sdk = require('matrix-react-sdk'); + +var ScrollPanel = sdk.getComponent('structures.ScrollPanel'); +var test_utils = require('test-utils'); + +var Tester = React.createClass({ + getInitialState: function() { + return { + tileKeys: [], + }; + }, + + componentWillMount: function() { + this.fillCounts = {'b': 0, 'f': 0}; + this._fillHandlers = {'b': null, 'f': null}; + this._fillDefers = {'b': null, 'f': null}; + this._scrollDefer = null; + + // scrollTop at the last scroll event + this.lastScrollEvent = null; + }, + + _onFillRequest: function(back) { + var dir = back ? 'b': 'f'; + console.log("FillRequest: " + dir); + this.fillCounts[dir]++; + + var handler = this._fillHandlers[dir]; + var defer = this._fillDefers[dir]; + + // don't use the same handler twice + this._fillHandlers[dir] = null; + this._fillDefers[dir] = null; + + var res; + if (handler) { + res = handler(); + } else { + res = q(false); + } + + if (defer) { + defer.resolve(); + } + return res; + }, + + addFillHandler: function(dir, handler) { + this._fillHandlers[dir] = handler; + }, + + /* returns a promise which will resolve when the fill happens */ + awaitFill: function(dir) { + var defer = q.defer(); + this._fillDefers[dir] = defer; + return defer.promise; + }, + + _onScroll: function(ev) { + var st = ev.target.scrollTop; + console.log("Scroll event; scrollTop: " + st); + this.lastScrollEvent = st; + + var d = this._scrollDefer; + if (d) { + this._scrollDefer = null; + d.resolve(); + } + }, + + /* returns a promise which will resolve when a scroll event happens */ + awaitScroll: function() { + console.log("Awaiting scroll"); + this._scrollDefer = q.defer(); + return this._scrollDefer.promise; + }, + + setTileKeys: function(keys) { + console.log("Updating keys: len=" + keys.length); + this.setState({tileKeys: keys.slice()}); + }, + + scrollPanel: function() { + return this.refs.sp; + }, + + _mkTile: function(key) { + // each tile is 150 pixels high: + // 98 pixels of body + // 2 pixels of border + // 50 pixels of margin + // + // there is an extra 50 pixels of margin at the bottom. + return ( +
  • +
    + {key} +
    +
    • + ); + }, + + render: function() { + var tiles = this.state.tileKeys.map(this._mkTile); + console.log("rendering with " + tiles.length + " tiles"); + return ( + + {tiles} + + ); + }, +}); + +describe('ScrollPanel', function() { + var parentDiv; + var tester; + var scrollingDiv; + + beforeEach(function(done) { + test_utils.beforeEach(this); + + // create a div of a useful size to put our panel in, and attach it to + // the document so that we can interact with it properly. + parentDiv = document.createElement('div'); + parentDiv.style.width = '800px'; + parentDiv.style.height = '600px'; + parentDiv.style.overflow = 'hidden'; + document.body.appendChild(parentDiv); + + tester = ReactDOM.render(, parentDiv); + expect(tester.fillCounts.b).toEqual(1); + expect(tester.fillCounts.f).toEqual(1); + + scrollingDiv = ReactTestUtils.findRenderedDOMComponentWithClass( + tester, "gm-scroll-view"); + + // wait for a browser tick to let the initial paginates complete + setTimeout(function() { + done(); + }, 0); + }); + + afterEach(function() { + if (parentDiv) { + document.body.removeChild(parentDiv); + parentDiv = null; + } + }); + + it('should handle scrollEvent strangeness', function(done) { + var events = []; + + q().then(() => { + // initialise with a few events + for (var i = 0; i < 10; i++) { + events.push(i+90); + } + tester.setTileKeys(events); + expect(tester.fillCounts.b).toEqual(1); + expect(tester.fillCounts.f).toEqual(2); + expect(scrollingDiv.scrollHeight).toEqual(1550) // 10*150 + 50 + expect(scrollingDiv.scrollTop).toEqual(1550 - 600); + return tester.awaitScroll(); + }).then(() => { + expect(tester.lastScrollEvent).toBe(950); + + // we want to simulate back-filling as we scroll up + tester.addFillHandler('b', function() { + var newEvents = []; + for (var i = 0; i < 10; i++) { + newEvents.push(i+80); + } + events.unshift.apply(events, newEvents); + tester.setTileKeys(events); + return q(true); + }); + + // simulate scrolling up; this should trigger the backfill + scrollingDiv.scrollTop = 200; + + return tester.awaitFill('b'); + }).then(() => { + console.log('filled'); + + // at this point, ScrollPanel will have updated scrollTop, but + // the event hasn't fired. Stamp over the scrollTop. + expect(tester.lastScrollEvent).toEqual(200); + expect(scrollingDiv.scrollTop).toEqual(10*150 + 200); + scrollingDiv.scrollTop = 500; + + return tester.awaitScroll(); + }).then(() => { + expect(tester.lastScrollEvent).toBe(10*150 + 200); + expect(scrollingDiv.scrollTop).toEqual(10*150 + 200); + }).done(done); + }); + + it('should not get stuck in #528 workaround', function(done) { + var events = []; + q().then(() => { + // initialise with a bunch of events + for (var 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 (var 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); + }); +}); diff --git a/test/test-utils.js b/test/test-utils.js index 956ced0554..ed14306fbe 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -5,6 +5,18 @@ var jssdk = require('matrix-js-sdk'); var MatrixEvent = jssdk.MatrixEvent; var sinon = require('sinon'); +/** + * Perform common actions before each test case, e.g. printing the test case + * name to stdout. + * @param {Mocha.Context} context The test context + */ +module.exports.beforeEach = function(context) { + var desc = context.currentTest.fullTitle(); + console.log(); + console.log(desc); + console.log(new Array(1 + desc.length).join("=")); +}; + /** * Stub out the MatrixClient, and configure the MatrixClientPeg object to @@ -128,22 +140,3 @@ module.exports.mkMessage = function(opts) { }; return module.exports.mkEvent(opts); }; - -/** - * make the test fail, with the given exception - * - *

    This is useful for use with integration tests which use asyncronous - * methods: it can be added as a 'catch' handler in a promise chain. - * - * @param {Error} error exception to be reported - * - * @example - * it("should not throw", function(done) { - * asynchronousMethod().then(function() { - * // some tests - * }).catch(utils.failTest).done(done); - * }); - */ -module.exports.failTest = function(error) { - expect(error.stack).toBe(null); -}; From ae1220a6a410cc8957c55cec1716ebb6e2fc6e04 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 5 Apr 2016 11:08:02 +0100 Subject: [PATCH 2/2] Fix test for npm 3 npm 3 puts files in slightly saner places, so we have to look for the gemini css in two places --- karma.conf.js | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/karma.conf.js b/karma.conf.js index dedd30f878..eed6d580fa 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -1,6 +1,7 @@ // karma.conf.js - the config file for karma, which runs our tests. var path = require('path'); +var fs = require('fs'); /* * We use webpack to build our tests. It's a pain to have to wait for webpack @@ -17,6 +18,21 @@ var path = require('path'); process.env.PHANTOMJS_BIN = 'node_modules/.bin/phantomjs'; +function fileExists(name) { + try { + fs.statSync(gsCss); + return true; + } catch (e) { + return false; + } +} + +// try find the gemini-scrollbar css in an npm-version-agnostic way +var gsCss = 'node_modules/gemini-scrollbar/gemini-scrollbar.css'; +if (!fileExists(gsCss)) { + gsCss = 'node_modules/react-gemini-scrollbar/'+gsCss; +} + module.exports = function (config) { config.set({ // frameworks to use @@ -26,9 +42,7 @@ module.exports = function (config) { // list of files / patterns to load in the browser files: [ 'test/tests.js', - - // XXX this will break on npm 3 - 'node_modules/react-gemini-scrollbar/node_modules/gemini-scrollbar/gemini-scrollbar.css', + gsCss, ], // list of files to exclude