mirror of https://github.com/vector-im/riot-web
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.pull/21833/head
Richard van der Hoff
parent
2d3a237101
commit
51fe77122b
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<li key={key} data-scroll-token={key}>
|
||||
<div style={{height: '98px', margin: '50px', border: '1px solid black',
|
||||
backgroundColor: '#fff8dc' }}>
|
||||
{key}
|
||||
</div>
|
||||
</li>
|
||||
);
|
||||
},
|
||||
|
||||
render: function() {
|
||||
var tiles = this.state.tileKeys.map(this._mkTile);
|
||||
console.log("rendering with " + tiles.length + " tiles");
|
||||
return (
|
||||
<ScrollPanel ref="sp"
|
||||
onScroll={ this._onScroll }
|
||||
onFillRequest={ this._onFillRequest }>
|
||||
{tiles}
|
||||
</ScrollPanel>
|
||||
);
|
||||
},
|
||||
});
|
||||
|
||||
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(<Tester/>, 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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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
|
||||
*
|
||||
* <p>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);
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in New Issue