From 86f39ee0eee2e18e86b3e92b2b7d8c0a56da9bdd Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 2 Apr 2020 15:07:50 +0100 Subject: [PATCH] Fix read marker visibility for grouped events The recent "groupers" which extracted out timeline grouping logic forgot to pass through the last event state for read marker computation. This causes the read marker to become visible when e.g. returning to room if it was last placed inside a grouped set of events (currently room creation and membership events). Regressed by https://github.com/matrix-org/matrix-react-sdk/pull/4059 Related to https://github.com/vector-im/riot-web/issues/12338 --- src/components/structures/MessagePanel.js | 21 +- .../structures/MessagePanel-test.js | 306 +++++++++++------- 2 files changed, 209 insertions(+), 118 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 4fd57d95e6..c3a2bdbc59 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -838,14 +838,16 @@ class CreationGrouper { // events that we include in the group but then eject out and place // above the group. this.ejectedEvents = []; - this.readMarker = panel._readMarkerForEvent(createEvent.getId()); + this.readMarker = panel._readMarkerForEvent( + createEvent.getId(), + createEvent === lastShownEvent, + ); } shouldGroup(ev) { const panel = this.panel; const createEvent = this.createEvent; if (!panel._shouldShowEvent(ev)) { - this.readMarker = this.readMarker || panel._readMarkerForEvent(ev.getId()); return true; } if (panel._wantsDateSeparator(this.createEvent, ev.getDate())) { @@ -863,7 +865,10 @@ class CreationGrouper { add(ev) { const panel = this.panel; - this.readMarker = this.readMarker || panel._readMarkerForEvent(ev.getId()); + this.readMarker = this.readMarker || panel._readMarkerForEvent( + ev.getId(), + ev === this.lastShownEvent, + ); if (!panel._shouldShowEvent(ev)) { return; } @@ -950,7 +955,10 @@ class MemberGrouper { constructor(panel, ev, prevEvent, lastShownEvent) { this.panel = panel; - this.readMarker = panel._readMarkerForEvent(ev.getId()); + this.readMarker = panel._readMarkerForEvent( + ev.getId(), + ev === lastShownEvent, + ); this.events = [ev]; this.prevEvent = prevEvent; this.lastShownEvent = lastShownEvent; @@ -971,7 +979,10 @@ class MemberGrouper { const renderText = textForEvent(ev); if (!renderText || renderText.trim().length === 0) return; // quietly ignore } - this.readMarker = this.readMarker || this.panel._readMarkerForEvent(ev.getId()); + this.readMarker = this.readMarker || this.panel._readMarkerForEvent( + ev.getId(), + ev === this.lastShownEvent, + ); this.events.push(ev); } diff --git a/test/components/structures/MessagePanel-test.js b/test/components/structures/MessagePanel-test.js index e6332cf7f8..3ac70bdac9 100644 --- a/test/components/structures/MessagePanel-test.js +++ b/test/components/structures/MessagePanel-test.js @@ -142,128 +142,42 @@ describe('MessagePanel', function() { return events; } - it('should show the events', function() { - const res = TestUtils.renderIntoDocument( - , - ); + // A list of membership events only with nothing else + function mkMelsEventsOnly() { + const events = []; + const ts0 = Date.now(); - // just check we have the right number of tiles for now - const tiles = TestUtils.scryRenderedComponentsWithType( - res, sdk.getComponent('rooms.EventTile')); - expect(tiles.length).toEqual(10); - }); + let i = 0; - it('should collapse adjacent member events', function() { - const res = TestUtils.renderIntoDocument( - , - ); + 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', + })); + } - // 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); + return events; + } - 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 tiles = TestUtils.scryRenderedComponentsWithType( - res, sdk.getComponent('rooms.EventTile')); - - // find the
  • which wraps the read marker - const rm = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_RoomView_myReadMarker_container'); - - // it should follow the
  • which wraps the event tile for event 4 - const eventContainer = ReactDOM.findDOMNode(tiles[4]).parentNode; - expect(rm.previousSibling).toEqual(eventContainer); - }); - - it('should show the read-marker that fall in summarised events after the summary', function() { - const melsEvents = mkMelsEvents(); - 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(); - clock.mockDate(); - - 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; - }); - - // find the
  • which wraps the read marker - const rm = TestUtils.findRenderedDOMComponentWithClass(mp, 'mx_RoomView_myReadMarker_container'); - expect(rm.previousSibling).toEqual(tileContainers[4]); - - // now move the RM - mp = ReactDOM.render( - , parentDiv); - - // now there should be two RM containers - const found = TestUtils.scryRenderedDOMComponentsWithClass(mp, 'mx_RoomView_myReadMarker_container'); - expect(found.length).toEqual(2); - - // the first should be the ghost - expect(found[0].previousSibling).toEqual(tileContainers[4]); - const hr = found[0].children[0]; - - // the second should be the real thing - expect(found[1].previousSibling).toEqual(tileContainers[6]); - - // advance the clock, and then let the browser run an animation frame, - // to let the animation start - clock.tick(1500); - - realSetTimeout(() => { - // then advance it again to let it complete - clock.tick(1000); - realSetTimeout(() => { - // the ghost should now have finished - expect(hr.style.opacity).toEqual('0'); - done(); - }, 100); - }, 100); - }); - - it('should collapse creation events', function() { + // A list of room creation, encryption, and invite events. + function mkCreationEvents() { const mkEvent = test_utils.mkEvent; const mkMembership = test_utils.mkMembership; const roomId = "!someroom"; const alice = "@alice:example.org"; const ts0 = Date.now(); - const events = [ + + return [ mkEvent({ event: true, type: "m.room.create", @@ -341,6 +255,150 @@ describe('MessagePanel', function() { name: 'Bob', }), ]; + } + + function isReadMarkerVisible(rmContainer) { + return rmContainer && rmContainer.children.length > 0; + } + + it('should show the 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(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 insert the read-marker in the right place', function() { + const res = TestUtils.renderIntoDocument( + , + ); + + const tiles = TestUtils.scryRenderedComponentsWithType( + res, sdk.getComponent('rooms.EventTile')); + + // find the
  • which wraps the read marker + const rm = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_RoomView_myReadMarker_container'); + + // it should follow the
  • which wraps the event tile for event 4 + const eventContainer = ReactDOM.findDOMNode(tiles[4]).parentNode; + expect(rm.previousSibling).toEqual(eventContainer); + }); + + it('should show the read-marker that fall in summarised events after the summary', function() { + const melsEvents = mkMelsEvents(); + 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); + + // read marker should be visible given props and not at the last event + expect(isReadMarkerVisible(rm)).toBeTruthy(); + }); + + it('should hide the read-marker at the end of summarised events', function() { + const melsEvents = mkMelsEventsOnly(); + 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); + + // read marker should be hidden given props and at the last event + expect(isReadMarkerVisible(rm)).toBeFalsy(); + }); + + 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(); + clock.mockDate(); + + 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; + }); + + // find the
  • which wraps the read marker + const rm = TestUtils.findRenderedDOMComponentWithClass(mp, 'mx_RoomView_myReadMarker_container'); + expect(rm.previousSibling).toEqual(tileContainers[4]); + + // now move the RM + mp = ReactDOM.render( + , parentDiv); + + // now there should be two RM containers + const found = TestUtils.scryRenderedDOMComponentsWithClass(mp, 'mx_RoomView_myReadMarker_container'); + expect(found.length).toEqual(2); + + // the first should be the ghost + expect(found[0].previousSibling).toEqual(tileContainers[4]); + const hr = found[0].children[0]; + + // the second should be the real thing + expect(found[1].previousSibling).toEqual(tileContainers[6]); + + // advance the clock, and then let the browser run an animation frame, + // to let the animation start + clock.tick(1500); + + realSetTimeout(() => { + // then advance it again to let it complete + clock.tick(1000); + realSetTimeout(() => { + // the ghost should now have finished + expect(hr.style.opacity).toEqual('0'); + done(); + }, 100); + }, 100); + }); + + it('should collapse creation events', function() { + const events = mkCreationEvents(); const res = mount( , ); @@ -363,4 +421,26 @@ describe('MessagePanel', function() { // invite event should be in the event summary expect(summaryEventTiles.length).toEqual(tiles.length - 3); }); + + it('should hide read-marker at the end of creation event summary', function() { + const events = mkCreationEvents(); + const res = mount( + , + ); + + // find the
  • which wraps the read marker + const rm = res.find('.mx_RoomView_myReadMarker_container').getDOMNode(); + + const rows = res.find('.mx_RoomView_MessageList').children(); + expect(rows.length).toEqual(6); + expect(rm.previousSibling).toEqual(rows.at(4).getDOMNode()); + + // read marker should be hidden given props and at the last event + expect(isReadMarkerVisible(rm)).toBeFalsy(); + }); });