From a2e3f6496312d9def35d529e7ede9b5bfcc7622f Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 26 Nov 2019 19:06:02 +0000 Subject: [PATCH] Change read markers to use CSS transitions Removes one of the two places we use Velocity, so we're one step closer to getting rid of it for good. Should therefore fix the fact that Velocity is leaking data entries and therefore
elements. Hopefully also makes the logic in getEventTiles incrementally simpler, if still somwewhat byzantine. --- res/css/structures/_RoomView.scss | 3 + src/components/structures/MessagePanel.js | 228 ++++++++---------- .../structures/MessagePanel-test.js | 118 +++++---- 3 files changed, 176 insertions(+), 173 deletions(-) diff --git a/res/css/structures/_RoomView.scss b/res/css/structures/_RoomView.scss index 50d412ad58..5e826306c6 100644 --- a/res/css/structures/_RoomView.scss +++ b/res/css/structures/_RoomView.scss @@ -221,6 +221,9 @@ hr.mx_RoomView_myReadMarker { position: relative; top: -1px; z-index: 1; + transition: width 400ms easeInSine 1s, opacity 400ms easeInSine 1s; + width: 99%; + opacity: 1; } .mx_RoomView_callStatusBar .mx_UploadBar_uploadProgressInner { diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 912b865b9f..d1cc1b7caf 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -16,8 +16,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -/* global Velocity */ - import React from 'react'; import ReactDOM from 'react-dom'; import PropTypes from 'prop-types'; @@ -111,14 +109,12 @@ export default class MessagePanel extends React.Component { constructor() { super(); - // the event after which we put a visible unread marker on the last - // render cycle; null if readMarkerVisible was false or the RM was - // suppressed (eg because it was at the end of the timeline) - this.currentReadMarkerEventId = null; - // the event after which we are showing a disappearing read marker - // animation - this.currentGhostEventId = null; + this.state = { + // previous positions the read marker has been in, so we can + // display 'ghost' read markers that are animating away + ghostReadMarkers: [], + }; // opaque readreceipt info for each userId; used by ReadReceiptMarker // to manage its animations @@ -157,10 +153,6 @@ export default class MessagePanel extends React.Component { // displayed event in the current render cycle. this._readReceiptsByUserId = {}; - // Remember the read marker ghost node so we can do the cleanup that - // Velocity requires - this._readMarkerGhostNode = null; - // Cache hidden events setting on mount since Settings is expensive to // query, and we check this in a hot code path. this._showHiddenEventsInTimeline = @@ -177,6 +169,16 @@ export default class MessagePanel extends React.Component { this._isMounted = false; } + componentDidUpdate(prevProps, prevState) { + if (prevProps.readMarkerVisible && this.props.readMarkerEventId !== prevProps.readMarkerEventId) { + const ghostReadMarkers = this.state.ghostReadMarkers; + ghostReadMarkers.push(prevProps.readMarkerEventId); + this.setState({ + ghostReadMarkers, + }); + } + } + /* get the DOM node representing the given event */ getNodeForEventId(eventId) { if (!this.eventNodes) { @@ -325,6 +327,78 @@ export default class MessagePanel extends React.Component { return !shouldHideEvent(mxEv); } + _readMarkerForEvent(eventId, isLastEvent) { + const visible = !isLastEvent && this.props.readMarkerVisible; + + if (this.props.readMarkerEventId === eventId) { + let hr; + // if the read marker comes at the end of the timeline (except + // for local echoes, which are excluded from RMs, because they + // don't have useful event ids), we don't want to show it, but + // we still want to create the
  • for it so that the + // algorithms which depend on its position on the screen aren't + // confused. + if (visible) { + hr =
    ; + } + + return ( +
  • + { hr } +
    • + ); + } else if (this.state.ghostReadMarkers.includes(eventId)) { + // We render 'ghost' read markers in the DOM while they + // transition away. This allows the actual read marker + // to be in the right place straight away without having + // to wait for the transition to finish. + // There are probably much simpler ways to do this transition, + // possibly using react-transition-group which handles keeping + // elements in the DOM whilst they transition out, although our + // case is a little more complex because only some of the items + // transition (ie. the read markers do but the event tiles do not) + // and TransitionGroup requires that all its children are Transitions. + const hr =
    ; + + // give it a key which depends on the event id. That will ensure that + // we get a new DOM node (restarting the animation) when the ghost + // moves to a different event. + return ( +
  • + { hr } +
    • + ); + } + + return null; + } + + _collectGhostReadMarker = (node) => { + if (node) { + // now the element has appeared, change the style which will trigger the CSS transition + requestAnimationFrame(() => { + node.style.width = '10%'; + node.style.opacity = '0'; + }); + } + }; + + _onGhostTransitionEnd = (ev) => { + // we can now clean up the ghost element + const finishedEventId = ev.target.dataset.eventid; + this.setState({ + ghostReadMarkers: this.state.ghostReadMarkers.filter(eid => eid !== finishedEventId), + }); + }; + _getEventTiles() { const DateSeparator = sdk.getComponent('messages.DateSeparator'); const EventListSummary = sdk.getComponent('views.elements.EventListSummary'); @@ -332,7 +406,6 @@ export default class MessagePanel extends React.Component { this.eventNodes = {}; - let visible = false; let i; // first figure out which is the last event in the list which we're @@ -367,16 +440,6 @@ export default class MessagePanel extends React.Component { let prevEvent = null; // the last event we showed - // assume there is no read marker until proven otherwise - let readMarkerVisible = false; - - // if the readmarker has moved, cancel any active ghost. - if (this.currentReadMarkerEventId && this.props.readMarkerEventId && - this.props.readMarkerVisible && - this.currentReadMarkerEventId !== this.props.readMarkerEventId) { - this.currentGhostEventId = null; - } - this._readReceiptsByEvent = {}; if (this.props.showReadReceipts) { this._readReceiptsByEvent = this._getReadReceiptsByShownEvent(); @@ -401,7 +464,7 @@ export default class MessagePanel extends React.Component { return false; }; if (mxEv.getType() === "m.room.create") { - let readMarkerInSummary = false; + let summaryReadMarker = null; const ts1 = mxEv.getTs(); if (this._wantsDateSeparator(prevEvent, mxEv.getDate())) { @@ -410,9 +473,7 @@ export default class MessagePanel extends React.Component { } // If RM event is the first in the summary, append the RM after the summary - if (mxEv.getId() === this.props.readMarkerEventId) { - readMarkerInSummary = true; - } + summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(mxEv.getId()); // If this m.room.create event should be shown (room upgrade) then show it before the summary if (this._shouldShowEvent(mxEv)) { @@ -427,9 +488,7 @@ export default class MessagePanel extends React.Component { // Ignore redacted/hidden member events if (!this._shouldShowEvent(collapsedMxEv)) { // If this hidden event is the RM and in or at end of a summary put RM after the summary. - if (collapsedMxEv.getId() === this.props.readMarkerEventId) { - readMarkerInSummary = true; - } + summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); continue; } @@ -438,9 +497,7 @@ export default class MessagePanel extends React.Component { } // If RM event is in the summary, mark it as such and the RM will be appended after the summary. - if (collapsedMxEv.getId() === this.props.readMarkerEventId) { - readMarkerInSummary = true; - } + summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); summarisedEvents.push(collapsedMxEv); } @@ -468,8 +525,8 @@ export default class MessagePanel extends React.Component { { eventTiles } ); - if (readMarkerInSummary) { - ret.push(this._getReadMarkerTile(visible)); + if (summaryReadMarker) { + ret.push(summaryReadMarker); } prevEvent = mxEv; @@ -480,7 +537,7 @@ export default class MessagePanel extends React.Component { // Wrap consecutive member events in a ListSummary, ignore if redacted if (isMembershipChange(mxEv) && wantTile) { - let readMarkerInMels = false; + let summaryReadMarker = null; const ts1 = mxEv.getTs(); // Ensure that the key of the MemberEventListSummary does not change with new // member events. This will prevent it from being re-created unnecessarily, and @@ -498,9 +555,7 @@ export default class MessagePanel extends React.Component { } // If RM event is the first in the MELS, append the RM after MELS - if (mxEv.getId() === this.props.readMarkerEventId) { - readMarkerInMels = true; - } + summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(mxEv.getId()); const summarisedEvents = [mxEv]; for (;i + 1 < this.props.events.length; i++) { @@ -509,9 +564,7 @@ export default class MessagePanel extends React.Component { // Ignore redacted/hidden member events if (!this._shouldShowEvent(collapsedMxEv)) { // If this hidden event is the RM and in or at end of a MELS put RM after MELS. - if (collapsedMxEv.getId() === this.props.readMarkerEventId) { - readMarkerInMels = true; - } + summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); continue; } @@ -521,9 +574,7 @@ export default class MessagePanel extends React.Component { } // If RM event is in MELS mark it as such and the RM will be appended after MELS. - if (collapsedMxEv.getId() === this.props.readMarkerEventId) { - readMarkerInMels = true; - } + summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); summarisedEvents.push(collapsedMxEv); } @@ -554,8 +605,8 @@ export default class MessagePanel extends React.Component { { eventTiles } ); - if (readMarkerInMels) { - ret.push(this._getReadMarkerTile(visible)); + if (summaryReadMarker) { + ret.push(summaryReadMarker); } prevEvent = mxEv; @@ -570,40 +621,10 @@ export default class MessagePanel extends React.Component { prevEvent = mxEv; } - let isVisibleReadMarker = false; - - if (eventId === this.props.readMarkerEventId) { - visible = this.props.readMarkerVisible; - - // if the read marker comes at the end of the timeline (except - // for local echoes, which are excluded from RMs, because they - // don't have useful event ids), we don't want to show it, but - // we still want to create the
  • for it so that the - // algorithms which depend on its position on the screen aren't - // confused. - if (i >= lastShownNonLocalEchoIndex) { - visible = false; - } - ret.push(this._getReadMarkerTile(visible)); - readMarkerVisible = visible; - isVisibleReadMarker = visible; - } - - // XXX: there should be no need for a ghost tile - we should just use a - // a dispatch (user_activity_end) to start the RM animation. - if (eventId === this.currentGhostEventId) { - // if we're showing an animation, continue to show it. - ret.push(this._getReadMarkerGhostTile()); - } else if (!isVisibleReadMarker && - eventId === this.currentReadMarkerEventId) { - // there is currently a read-up-to marker at this point, but no - // more. Show an animation of it disappearing. - ret.push(this._getReadMarkerGhostTile()); - this.currentGhostEventId = eventId; - } + const readMarker = this._readMarkerForEvent(eventId, i >= lastShownNonLocalEchoIndex); + if (readMarker) ret.push(readMarker); } - this.currentReadMarkerEventId = readMarkerVisible ? this.props.readMarkerEventId : null; return ret; } @@ -797,53 +818,6 @@ export default class MessagePanel extends React.Component { return receiptsByEvent; } - _getReadMarkerTile(visible) { - let hr; - if (visible) { - hr =
    ; - } - - return ( -
  • - { hr } -
    • - ); - } - - _startAnimation = (ghostNode) => { - if (this._readMarkerGhostNode) { - Velocity.Utilities.removeData(this._readMarkerGhostNode); - } - this._readMarkerGhostNode = ghostNode; - - if (ghostNode) { - // eslint-disable-next-line new-cap - Velocity(ghostNode, {opacity: '0', width: '10%'}, - {duration: 400, easing: 'easeInSine', - delay: 1000}); - } - }; - - _getReadMarkerGhostTile() { - const hr =
    ; - - // give it a key which depends on the event id. That will ensure that - // we get a new DOM node (restarting the animation) when the ghost - // moves to a different event. - return ( -
  • - { hr } -
    • - ); - } - _collectEventNode = (eventId, node) => { this.eventNodes[eventId] = node; } diff --git a/test/components/structures/MessagePanel-test.js b/test/components/structures/MessagePanel-test.js index f58f1b925c..7c52512bc2 100644 --- a/test/components/structures/MessagePanel-test.js +++ b/test/components/structures/MessagePanel-test.js @@ -1,5 +1,6 @@ /* Copyright 2016 OpenMarket Ltd +Copyright 2019 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -81,6 +82,7 @@ describe('MessagePanel', function() { // HACK: We assume all settings want to be disabled SettingsStore.getValue = sinon.stub().returns(false); + SettingsStore.getValue.withArgs('showDisplaynameChanges').returns(true); // This option clobbers the duration of all animations to be 1ms // which makes unit testing a lot simpler (the animation doesn't @@ -109,6 +111,44 @@ describe('MessagePanel', function() { return events; } + + // make a collection of events with some member events that should be collapsed + // with a MemberEventListSummary + function mkMelsEvents() { + const events = []; + const ts0 = Date.now(); + + let i = 0; + events.push(test_utils.mkMessage({ + event: true, room: "!room:id", user: "@user:id", + ts: ts0 + ++i*1000, + })); + + for (i = 0; i < 10; i++) { + events.push(test_utils.mkMembership({ + event: true, room: "!room:id", user: "@user:id", + target: { + userId: "@user:id", + name: "Bob", + getAvatarUrl: () => { + return "avatar.jpeg"; + }, + }, + ts: ts0 + i*1000, + mship: 'join', + prevMship: 'join', + name: 'A user', + })); + } + + events.push(test_utils.mkMessage({ + event: true, room: "!room:id", user: "@user:id", + ts: ts0 + ++i*1000, + })); + + return events; + } + it('should show the events', function() { const res = TestUtils.renderIntoDocument( , @@ -120,6 +160,23 @@ describe('MessagePanel', function() { expect(tiles.length).toEqual(10); }); + it('should collapse adjacent member events', function() { + const res = TestUtils.renderIntoDocument( + , + ); + + // just check we have the right number of tiles for now + const tiles = TestUtils.scryRenderedComponentsWithType( + res, sdk.getComponent('rooms.EventTile'), + ); + expect(tiles.length).toEqual(2); + + const summaryTiles = TestUtils.scryRenderedComponentsWithType( + res, sdk.getComponent('elements.MemberEventListSummary'), + ); + expect(summaryTiles.length).toEqual(1); + }); + it('should show the read-marker in the right place', function() { const res = TestUtils.renderIntoDocument( , + ); + + const summary = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_EventListSummary'); + + // find the
  • which wraps the read marker + const rm = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_RoomView_myReadMarker_container'); + + expect(rm.previousSibling).toEqual(summary); + }); + it('shows a ghost read-marker when the read-marker moves', function(done) { // fake the clock so that we can test the velocity animation. clock.install(); @@ -191,50 +263,4 @@ describe('MessagePanel', function() { }, 100); }, 100); }); - - it('shows only one ghost when the RM moves twice', function() { - const parentDiv = document.createElement('div'); - - // first render with the RM in one place - let mp = ReactDOM.render( - , parentDiv); - - const tiles = TestUtils.scryRenderedComponentsWithType( - mp, sdk.getComponent('rooms.EventTile')); - const tileContainers = tiles.map(function(t) { - return ReactDOM.findDOMNode(t).parentNode; - }); - - // now move the RM - mp = ReactDOM.render( - , parentDiv); - - // now there should be two RM containers - let found = TestUtils.scryRenderedDOMComponentsWithClass(mp, 'mx_RoomView_myReadMarker_container'); - expect(found.length).toEqual(2); - - // the first should be the ghost - expect(tileContainers.indexOf(found[0].previousSibling)).toEqual(4); - - // the second should be the real RM - expect(tileContainers.indexOf(found[1].previousSibling)).toEqual(6); - - // and move the RM again - mp = ReactDOM.render( - , parentDiv); - - // still two RM containers - found = TestUtils.scryRenderedDOMComponentsWithClass(mp, 'mx_RoomView_myReadMarker_container'); - expect(found.length).toEqual(2); - - // they should have moved - expect(tileContainers.indexOf(found[0].previousSibling)).toEqual(6); - expect(tileContainers.indexOf(found[1].previousSibling)).toEqual(8); - }); });