Merge pull request #4340 from matrix-org/jryans/read-marker-groupers

Fix read marker visibility for grouped events

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);
     }
 

test/components/structures/MessagePanel-test.js

@@ -142,128 +142,42 @@ describe('MessagePanel', function() {
         return events;
     }
 
-    it('should show the events', function() {
+    // A list of membership events only with nothing else
-        const res = TestUtils.renderIntoDocument(
+    function mkMelsEventsOnly() {
-                <WrappedMessagePanel className="cls" events={events} />,
+        const events = [];
-        );
+        const ts0 = Date.now();
 
-        // just check we have the right number of tiles for now
+        let i = 0;
-        const tiles = TestUtils.scryRenderedComponentsWithType(
-            res, sdk.getComponent('rooms.EventTile'));
-        expect(tiles.length).toEqual(10);
-    });
 
-    it('should collapse adjacent member events', function() {
+        for (i = 0; i < 10; i++) {
-        const res = TestUtils.renderIntoDocument(
+            events.push(test_utils.mkMembership({
-            <WrappedMessagePanel className="cls" events={mkMelsEvents()} />,
+                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
+        return events;
-        const tiles = TestUtils.scryRenderedComponentsWithType(
+    }
-            res, sdk.getComponent('rooms.EventTile'),
-        );
-        expect(tiles.length).toEqual(2);
 
-        const summaryTiles = TestUtils.scryRenderedComponentsWithType(
+    // A list of room creation, encryption, and invite events.
-            res, sdk.getComponent('elements.MemberEventListSummary'),
+    function mkCreationEvents() {
-        );
-        expect(summaryTiles.length).toEqual(1);
-    });
-
-    it('should show the read-marker in the right place', function() {
-        const res = TestUtils.renderIntoDocument(
-                <WrappedMessagePanel className="cls" events={events} readMarkerEventId={events[4].getId()}
-                    readMarkerVisible={true} />,
-        );
-
-        const tiles = TestUtils.scryRenderedComponentsWithType(
-            res, sdk.getComponent('rooms.EventTile'));
-
-        // find the <li> which wraps the read marker
-        const rm = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_RoomView_myReadMarker_container');
-
-        // it should follow the <li> 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(
-                <WrappedMessagePanel className="cls" events={melsEvents} readMarkerEventId={melsEvents[4].getId()}
-                    readMarkerVisible={true} />,
-        );
-
-        const summary = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_EventListSummary');
-
-        // find the <li> 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(
-                <WrappedMessagePanel className="cls" events={events} readMarkerEventId={events[4].getId()}
-                    readMarkerVisible={true}
-                />, parentDiv);
-
-        const tiles = TestUtils.scryRenderedComponentsWithType(
-            mp, sdk.getComponent('rooms.EventTile'));
-        const tileContainers = tiles.map(function(t) {
-            return ReactDOM.findDOMNode(t).parentNode;
-        });
-
-        // find the <li> 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(
-                <WrappedMessagePanel className="cls" events={events} readMarkerEventId={events[6].getId()}
-                    readMarkerVisible={true}
-                />, 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 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(
+                <WrappedMessagePanel className="cls" events={events} />,
+        );
+
+        // 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(
+            <WrappedMessagePanel className="cls" events={mkMelsEvents()} />,
+        );
+
+        // 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(
+                <WrappedMessagePanel className="cls" events={events} readMarkerEventId={events[4].getId()}
+                    readMarkerVisible={true} />,
+        );
+
+        const tiles = TestUtils.scryRenderedComponentsWithType(
+            res, sdk.getComponent('rooms.EventTile'));
+
+        // find the <li> which wraps the read marker
+        const rm = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_RoomView_myReadMarker_container');
+
+        // it should follow the <li> 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(
+                <WrappedMessagePanel className="cls" events={melsEvents} readMarkerEventId={melsEvents[4].getId()}
+                    readMarkerVisible={true} />,
+        );
+
+        const summary = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_EventListSummary');
+
+        // find the <li> 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(
+                <WrappedMessagePanel className="cls" events={melsEvents} readMarkerEventId={melsEvents[9].getId()}
+                    readMarkerVisible={true} />,
+        );
+
+        const summary = TestUtils.findRenderedDOMComponentWithClass(res, 'mx_EventListSummary');
+
+        // find the <li> 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(
+                <WrappedMessagePanel className="cls" events={events} readMarkerEventId={events[4].getId()}
+                    readMarkerVisible={true}
+                />, parentDiv);
+
+        const tiles = TestUtils.scryRenderedComponentsWithType(
+            mp, sdk.getComponent('rooms.EventTile'));
+        const tileContainers = tiles.map(function(t) {
+            return ReactDOM.findDOMNode(t).parentNode;
+        });
+
+        // find the <li> 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(
+                <WrappedMessagePanel className="cls" events={events} readMarkerEventId={events[6].getId()}
+                    readMarkerVisible={true}
+                />, 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(
             <WrappedMessagePanel className="cls" events={events} />,
         );
@@ -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(
+            <WrappedMessagePanel
+                className="cls"
+                events={events}
+                readMarkerEventId={events[5].getId()}
+                readMarkerVisible={true}
+            />,
+        );
+
+        // find the <li> 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();
+    });
 });