message view: Don't mark selected message as read when narrowing.

On activating a narrow we always mark the first message it selects as read, which is usually the first unread message. This behaviour is unwanted. The flow is as follows: * On activating the narrow we call the `MessageList.select_id` method along with a `force_rerender` param. Here event = { id: X, previously_selected_id: -1, ... } where X = the first unread message id to be selected. * Due to this `force_rerender` field, we first call the `rerender` method and then trigger our custom event handler (`message_selected.zulip`) which is responsible for marking message as read. * However, due to this call path: `this.select_id` > `this.rerender` > `this.rerender_view` > `this.redo_selection` > `this.select_id`, the `message_selected.zulip` event gets triggered with event as = { id: X, previously_selected_id: X, ... } * Thus the second selection event gets triggered before the first one having the current and previous id equal to "X". This above `id = previously_selected_id` consition only occurs for narrow activation (due to the `force_rerender` flag) or when a message is selected again by clicking on it (message is already read in this case). For the All Messages view we have similar logic where, we pass `mark_read` as false (which was previously unused) which skips the "message_selected.zulip" event in `message_scroll.js`. Work towards #10886.
zulip · May 20, 2021 · c61886b · c61886b
1 parent 6edd78a
commit c61886b
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 2 deletions.
diff --git a/frontend_tests/node_tests/message_list.js b/frontend_tests/node_tests/message_list.js
@@ -73,9 +73,45 @@ run_test("basics", (override) => {
 
     assert.deepEqual(list.all_messages(), messages);
 
-    override($, "Event", (ev) => {
+    let message_select_events = [];
+    override($, "Event", (ev, opts) => {
         assert.equal(ev, "message_selected.zulip");
+        message_select_events.push(opts);
     });
+
+    assert.equal(list.selected_id(), -1);
+    const stub = make_stub();
+    list.view.rerender_preserving_scrolltop = stub.f;
+    // When narrow initially renders in narrow.update_selection.
+    list.select_id(60, {
+        use_closest: true,
+        force_rerender: true,
+    });
+
+    const expected_message_select_events = [
+        {
+            id: 60,
+            previously_selected_id: 60,
+        },
+        {
+            id: 60,
+            previously_selected_id: -1,
+        },
+    ];
+    const actual_message_select_events = message_select_events.map((e) => ({
+        id: e.id,
+        previously_selected_id: e.previously_selected_id,
+    }));
+
+    // The render function is called which inturn calls selected_id,
+    // before the original message selection event takes place.
+    // Thus the event which contains the updated data is actually
+    // called first, instead of the original selected_id function call.
+    assert.equal(list.selected_id(), 60);
+    assert.equal(message_select_events.length, 2);
+    assert.deepEqual(actual_message_select_events, expected_message_select_events);
+    message_select_events = [];
+
     list.select_id(50);
 
     assert.equal(list.selected_id(), 50);

diff --git a/static/js/message_scroll.js b/static/js/message_scroll.js
@@ -188,8 +188,12 @@ export function initialize() {
         if (event.id === -1) {
             return;
         }
+        if (!event.mark_read || event.id === event.previously_selected_id) {
+            // Do not mark selected message read on initial narrow load.
+            return;
+        }
 
-        if (event.mark_read && event.previously_selected_id !== -1) {
+        if (event.previously_selected_id !== -1) {
             // Mark messages between old pointer and new pointer as read
             let messages;
             if (event.id < event.previously_selected_id) {