From dc13b944bcedbbba207fddad2feb3d4f80b925d3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 16 Dec 2015 17:57:32 +0000 Subject: [PATCH 1/8] Hacky fixes to jumpy scroll when backfilling Keep resetting our scroll offset until the DOM believes us. Hopefully this will fix vector-im/vector-web#528. --- src/components/structures/RoomView.js | 77 ++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 7 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 6db2659986..9ba7ba3f23 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -40,6 +40,8 @@ var dis = require("../../dispatcher"); var PAGINATE_SIZE = 20; var INITIAL_SIZE = 20; +var DEBUG_SCROLL = false; + module.exports = React.createClass({ displayName: 'RoomView', propTypes: { @@ -151,6 +153,12 @@ module.exports = React.createClass({ } }, + // get the DOM node which has the scrollTop property we care about for our + // message panel. + // + // If the gemini scrollbar is doing its thing, this will be a div within + // the message panel (ie, the gemini container); otherwise it will be the + // message panel itself. _getScrollNode: function() { var panel = ReactDOM.findDOMNode(this.refs.messagePanel); if (!panel) return null; @@ -309,12 +317,11 @@ module.exports = React.createClass({ if (!this.refs.messagePanel) return; if (this.state.searchResults) return; - var scrollState = this.savedScrollState; - if (scrollState.atBottom) { - this.scrollToBottom(); - } else if (scrollState.lastDisplayedEvent) { - this.scrollToEvent(scrollState.lastDisplayedEvent, - scrollState.pixelOffset); + + if (this.needsScrollReset) { + if (DEBUG_SCROLL) console.log("Resetting scroll position after tile count change"); + this._restoreSavedScrollState(); + this.needsScrollReset = false; } // have to fill space in case we're accepting an invite @@ -322,6 +329,8 @@ module.exports = React.createClass({ }, _paginateCompleted: function() { + if (DEBUG_SCROLL) console.log("paginate complete"); + this.setState({ room: MatrixClientPeg.get().getRoom(this.props.roomId) }); @@ -350,9 +359,11 @@ module.exports = React.createClass({ if (this.state.messageCap < this.state.room.timeline.length) { var cap = Math.min(this.state.messageCap + PAGINATE_SIZE, this.state.room.timeline.length); + if (DEBUG_SCROLL) console.log("winding back message cap to", cap); this.setState({messageCap: cap}); } else { var cap = this.state.messageCap + PAGINATE_SIZE; + if (DEBUG_SCROLL) console.log("starting paginate to cap", cap); this.setState({messageCap: cap, paginating: true}); MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE).finally(this._paginateCompleted).done(); return true; @@ -387,8 +398,33 @@ module.exports = React.createClass({ }, onMessageListScroll: function(ev) { + var sn = this._getScrollNode(); + if (DEBUG_SCROLL) console.log("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); + + // 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, when scrolling using the trackpad in OS + // X. 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.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.refs.messagePanel && !this.state.searchResults) { this.savedScrollState = this._calculateScrollState(); + if (DEBUG_SCROLL) console.log("Saved scroll state", this.savedScrollState); if (this.savedScrollState.atBottom && this.state.numUnreadMessages != 0) { this.setState({numUnreadMessages: 0}); } @@ -647,6 +683,11 @@ module.exports = React.createClass({ } ++count; } + if (count != this.lastEventTileCount) { + if (DEBUG_SCROLL) console.log("Queuing scroll reset (event count changed; now "+count+"; was "+this.lastEventTileCount+")"); + this.needsScrollReset = true; + } + this.lastEventTileCount = count; return ret; }, @@ -869,6 +910,7 @@ module.exports = React.createClass({ var scrollNode = this._getScrollNode(); if (!scrollNode) return; scrollNode.scrollTop = scrollNode.scrollHeight; + if (DEBUG_SCROLL) console.log("Scrolled to bottom; offset now", scrollNode.scrollTop); }, // scroll the event view to put the given event at the bottom. @@ -919,7 +961,28 @@ module.exports = React.createClass({ var wrapperRect = ReactDOM.findDOMNode(messageWrapper).getBoundingClientRect(); var boundingRect = node.getBoundingClientRect(); - scrollNode.scrollTop += boundingRect.bottom + pixelOffset - wrapperRect.bottom; + var scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom; + if(scrollDelta != 0) { + scrollNode.scrollTop += scrollDelta; + + // see the comments in onMessageListScroll regarding recentEventScroll + this.recentEventScroll = scrollNode.scrollTop; + } + + if (DEBUG_SCROLL) { + console.log("Scrolled to event", eventId, "+", pixelOffset+":", scrollNode.scrollTop, "(delta: "+scrollDelta+")"); + console.log("recentEventScroll now "+this.recentEventScroll); + } + }, + + _restoreSavedScrollState: function() { + var scrollState = this.savedScrollState; + if (scrollState.atBottom) { + this.scrollToBottom(); + } else if (scrollState.lastDisplayedEvent) { + this.scrollToEvent(scrollState.lastDisplayedEvent, + scrollState.pixelOffset); + } }, _calculateScrollState: function() { From 4327a2302d9ba70b09c522feb9bbfac60144b649 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 16 Dec 2015 18:05:15 +0000 Subject: [PATCH 2/8] spell out affected versions --- src/components/structures/RoomView.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 9ba7ba3f23..a7944f91f5 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -405,10 +405,10 @@ module.exports = React.createClass({ // ignored. (Or rather, it is successfully written, but on the next // scroll event, it's been reset again). // - // This was observed on Chrome, when scrolling using the trackpad in OS - // X. 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. + // 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 From 1eeb732625632e1b676bb72d96143786b3b55e42 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 17 Dec 2015 14:34:45 +0000 Subject: [PATCH 3/8] Supply bind_email=true at registration time - required for 3pid invites to work. --- src/Signup.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Signup.js b/src/Signup.js index 02ddaacc6d..74c4ad5f19 100644 --- a/src/Signup.js +++ b/src/Signup.js @@ -116,8 +116,17 @@ class Register extends Signup { _tryRegister(authDict) { var self = this; + + var bindEmail; + + if (this.username && this.password) { + // only need to bind_email when sending u/p - sending it at other + // times clobbers the u/p resulting in M_MISSING_PARAM (password) + bindEmail = true; + } + return MatrixClientPeg.get().register( - this.username, this.password, this.params.sessionId, authDict + this.username, this.password, this.params.sessionId, authDict, bindEmail ).then(function(result) { self.credentials = result; self.setStep("COMPLETE"); From 32bd9d155cfd00e1e0a66d050f1bd2d0e5d5e98e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 17 Dec 2015 14:56:55 +0000 Subject: [PATCH 4/8] Pass a new prop 'startingQueryParams' to pluck out the email from the URL This is preferable to doing the way other QPs are passed (secret, etc) because the link in the email wants to look like "#/room/" for guest read-access (only bouncing you to /login if that room is not readable by guests). This is hard to do in the current arch because we don't preserve QPs on /room paths, and we do conditional executions depending on if it is a room ID or alias. Rather than threading through the email in each section and creating a fragile mess, just pass the *starting* set of query parameters through to MatrixChat which can then do the Right Thing when the time comes. --- src/components/structures/MatrixChat.js | 14 +++++++++++--- src/components/structures/login/Registration.js | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index f2424dc06e..f38c75d676 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -41,7 +41,8 @@ module.exports = React.createClass({ config: React.PropTypes.object.isRequired, ConferenceHandler: React.PropTypes.any, onNewScreen: React.PropTypes.func, - registrationUrl: React.PropTypes.string + registrationUrl: React.PropTypes.string, + startingQueryParams: React.PropTypes.object }, PageTypes: { @@ -75,6 +76,12 @@ module.exports = React.createClass({ return s; }, + getDefaultProps: function() { + return { + startingQueryParams: {} + }; + }, + componentDidMount: function() { this.dispatcherRef = dis.register(this.onAction); if (this.state.logged_in) { @@ -540,9 +547,9 @@ module.exports = React.createClass({ } }, - notifyNewScreen: function(screen) { + notifyNewScreen: function(screen, queryParamString) { if (this.props.onNewScreen) { - this.props.onNewScreen(screen); + this.props.onNewScreen(screen, queryParamString); } }, @@ -706,6 +713,7 @@ module.exports = React.createClass({ clientSecret={this.state.register_client_secret} sessionId={this.state.register_session_id} idSid={this.state.register_id_sid} + email={this.props.startingQueryParams.email} hsUrl={this.props.config.default_hs_url} isUrl={this.props.config.default_is_url} registrationUrl={this.props.registrationUrl} diff --git a/src/components/structures/login/Registration.js b/src/components/structures/login/Registration.js index 1570641556..19e55c4d76 100644 --- a/src/components/structures/login/Registration.js +++ b/src/components/structures/login/Registration.js @@ -39,6 +39,7 @@ module.exports = React.createClass({ idSid: React.PropTypes.string, hsUrl: React.PropTypes.string, isUrl: React.PropTypes.string, + email: React.PropTypes.string, // registration shouldn't know or care how login is done. onLoginClick: React.PropTypes.func.isRequired }, @@ -185,6 +186,7 @@ module.exports = React.createClass({ registerStep = ( From 6e324a0dd159afadc2f626333f489a07e50dfcf4 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 17 Dec 2015 15:12:09 +0000 Subject: [PATCH 5/8] Whoops, didn't mean to add this --- src/components/structures/MatrixChat.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index f38c75d676..8ad5b44762 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -547,9 +547,9 @@ module.exports = React.createClass({ } }, - notifyNewScreen: function(screen, queryParamString) { + notifyNewScreen: function(screen) { if (this.props.onNewScreen) { - this.props.onNewScreen(screen, queryParamString); + this.props.onNewScreen(screen); } }, From 17a8eb010996d1235c803b36b668f9113356c8b5 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 17 Dec 2015 15:48:14 +0000 Subject: [PATCH 6/8] Display m.room.third_party_invite events. Display sensible text transitions. --- src/TextForEvent.js | 18 +++++++++++++++++- src/components/views/rooms/EventTile.js | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/TextForEvent.js b/src/TextForEvent.js index 439d7b9ef5..22ada8849e 100644 --- a/src/TextForEvent.js +++ b/src/TextForEvent.js @@ -9,7 +9,16 @@ function textForMemberEvent(ev) { ) : ""; switch (ev.getContent().membership) { case 'invite': - return senderName + " invited " + targetName + "."; + var threePidContent = ev.getContent().third_party_invite; + if (threePidContent) { + // TODO: When we have third_party_invite.display_name we should + // do this as "$displayname received the invitation from $sender" + // or equiv + return targetName + " received an invitation from " + senderName; + } + else { + return senderName + " invited " + targetName + "."; + } case 'ban': return senderName + " banned " + targetName + "." + reason; case 'join': @@ -101,6 +110,12 @@ function textForCallInviteEvent(event) { return senderName + " placed a " + type + " call." + supported; }; +function textForThreePidInviteEvent(event) { + var senderName = event.sender ? event.sender.name : event.getSender(); + return senderName + " sent an invitation to " + event.getContent().display_name + + " to join the room."; +}; + var handlers = { 'm.room.message': textForMessageEvent, 'm.room.name': textForRoomNameEvent, @@ -109,6 +124,7 @@ var handlers = { 'm.call.invite': textForCallInviteEvent, 'm.call.answer': textForCallAnswerEvent, 'm.call.hangup': textForCallHangupEvent, + 'm.room.third_party_invite': textForThreePidInviteEvent }; module.exports = { diff --git a/src/components/views/rooms/EventTile.js b/src/components/views/rooms/EventTile.js index 8292cdf826..0b26000207 100644 --- a/src/components/views/rooms/EventTile.js +++ b/src/components/views/rooms/EventTile.js @@ -44,6 +44,7 @@ var eventTileTypes = { 'm.call.hangup' : 'messages.TextualEvent', 'm.room.name' : 'messages.TextualEvent', 'm.room.topic' : 'messages.TextualEvent', + 'm.room.third_party_invite': 'messages.TextualEvent' }; var MAX_READ_AVATARS = 5; From 478ca91b4fea46e7ca20fad4037ed1f7e653aca0 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Thu, 17 Dec 2015 16:27:56 +0000 Subject: [PATCH 7/8] s/Conversations/Rooms/ as per https://github.com/vector-im/vector-web/issues/535 --- src/components/views/rooms/RoomList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/rooms/RoomList.js b/src/components/views/rooms/RoomList.js index a89dd55f1a..6af1af31dd 100644 --- a/src/components/views/rooms/RoomList.js +++ b/src/components/views/rooms/RoomList.js @@ -257,7 +257,7 @@ module.exports = React.createClass({ collapsed={ self.props.collapsed } /> Date: Thu, 17 Dec 2015 16:40:46 +0000 Subject: [PATCH 8/8] Full Stop. --- src/TextForEvent.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/TextForEvent.js b/src/TextForEvent.js index 22ada8849e..5296ef833e 100644 --- a/src/TextForEvent.js +++ b/src/TextForEvent.js @@ -14,7 +14,8 @@ function textForMemberEvent(ev) { // TODO: When we have third_party_invite.display_name we should // do this as "$displayname received the invitation from $sender" // or equiv - return targetName + " received an invitation from " + senderName; + return targetName + " received an invitation from " + senderName + + "."; } else { return senderName + " invited " + targetName + ".";