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

For narrows where we select the first unread message, on activation we have to select it. And on our initial selection was also `force_rerender` the message list which causes the message to get reselected. When `MessageList.select_id` is called for the first time, `previously_selected_id` is -1. However, because the rerender function call is before the event trigger, `select_id` is called again this time with `previously_selected_id` have the same id as the to be selected message and this selection event gets triggered before our first time's selection. This above case only occurs only for narrow activation or when a message is selected again by clicking on it (message is already read for this case). For the All Messages view we have similar logic where, since opts does not contain `force_rerender` we pass `mark_read` as false (which was previously unused) which skips the "message_selected.zulip" event in `message_scroll.js`. Fixes #10886.
zulip · Sep 13, 2020 · 4b4e683 · 4b4e683
1 parent 29e6595
commit 4b4e683
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 2 deletions.
diff --git a/frontend_tests/node_tests/message_list.js b/frontend_tests/node_tests/message_list.js
@@ -70,9 +70,46 @@ run_test("basics", () => {
 
     assert.deepEqual(list.all_messages(), messages);
 
-    global.$.Event = function (ev) {
+    let message_select_events = [];
+    global.$.Event = function (ev, opts) {
         assert.equal(ev, "message_selected.zulip");
+        message_select_events.push(opts);
     };
+
+    assert.equal(list.selected_id(), -1);
+    global.with_stub((stub) => {
+        list.view.clear_table = 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
@@ -169,8 +169,12 @@ exports.initialize = function () {
         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) {

diff --git a/static/js/narrow.js b/static/js/narrow.js
@@ -821,6 +821,7 @@ exports.deactivate = function () {
             then_scroll: true,
             use_closest: true,
             empty_ok: true,
+            mark_read: false,
         };
 
         // We fall back to the closest selected id, if the user has removed a