From cc8021a742491a04519544bda571206e0e23a842 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 25 Feb 2019 09:59:44 -0800 Subject: [PATCH] message_list: Fix focus issues when editing last message. Previously, if you were in the process of editing the last message in a narrow and a new message came in, we'd rerender that second-to-last message, causing your editing widget to lose focus (and thus the next few keys you typed to be interpreted as keyboard shortcuts, which had a good chance of resulting in your navigating somewhere random). This rerendering was essentially unnecessary; the only change to state going into the rendering process was the next_is_same_sender CSS class being toggled on the messagebox in the message. So, at most, we should have been just toggling that CSS class (and this commit makes us do precisely that). It seems like we could further improve this code by just removing the next_is_same_sender CSS class entirely and removing this block, but I'm leaving that for follow-up work. Fixes #11656. --- .../node_tests/message_list_view.js | 26 +++++++++---------- static/js/message_list_view.js | 25 ++++++++++-------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/frontend_tests/node_tests/message_list_view.js b/frontend_tests/node_tests/message_list_view.js index 9eca200bf1..37994e621c 100644 --- a/frontend_tests/node_tests/message_list_view.js +++ b/frontend_tests/node_tests/message_list_view.js @@ -126,7 +126,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_append_message_same_subject() { @@ -151,7 +151,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert_message_list_equal(result.append_messages, [message2]); - assert_message_list_equal(result.rerender_messages, [message1]); + assert_message_list_equal(result.rerender_messages_next_same_sender, [message1]); }()); (function test_append_message_different_subject() { @@ -177,7 +177,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_append_message_different_subject_and_days() { @@ -203,7 +203,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); assert.equal( message_group2.group_date_divider_html, '900000000 - 1000000'); @@ -229,7 +229,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, [message2]); - assert.deepEqual(result.rerender_messages, [message1]); + assert.deepEqual(result.rerender_messages_next_same_sender, [message1]); assert(list._message_groups[0].message_containers[1].want_date_divider); }()); @@ -256,7 +256,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_append_message_same_subject_me_message() { @@ -282,7 +282,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert.deepEqual(result.rerender_groups, []); assert_message_list_equal(result.append_messages, [message2]); - assert_message_list_equal(result.rerender_messages, [message1]); + assert_message_list_equal(result.rerender_messages_next_same_sender, [message1]); }()); @@ -309,7 +309,7 @@ run_test('merge_message_groups', () => { assert_message_groups_list_equal(result.rerender_groups, [build_message_group([message2, message1])]); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_prepend_message_different_subject() { @@ -334,7 +334,7 @@ run_test('merge_message_groups', () => { assert_message_groups_list_equal(result.prepend_groups, [message_group2]); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_prepend_message_different_subject_and_day() { @@ -364,7 +364,7 @@ run_test('merge_message_groups', () => { assert_message_groups_list_equal(result.prepend_groups, [message_group2]); assert.deepEqual(result.rerender_groups, [message_group1]); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_prepend_message_different_day() { @@ -393,7 +393,7 @@ run_test('merge_message_groups', () => { assert.deepEqual(result.prepend_groups, []); assert_message_groups_list_equal(result.rerender_groups, [message_group2]); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); (function test_prepend_message_historical() { @@ -419,12 +419,12 @@ run_test('merge_message_groups', () => { assert_message_groups_list_equal(result.prepend_groups, [message_group2]); assert.deepEqual(result.rerender_groups, []); assert.deepEqual(result.append_messages, []); - assert.deepEqual(result.rerender_messages, []); + assert.deepEqual(result.rerender_messages_next_same_sender, []); }()); }); -// TODO: Add a test suite for rerender_messages() that includes cases +// TODO: Add a test suite for rerender_messages_next_same_sender() that includes cases // where new messages added via local echo have a different date from // the older messages. diff --git a/static/js/message_list_view.js b/static/js/message_list_view.js index dfc78f771f..192b09a193 100644 --- a/static/js/message_list_view.js +++ b/static/js/message_list_view.js @@ -388,7 +388,7 @@ MessageListView.prototype = { prepend_groups: [], rerender_groups: [], append_messages: [], - rerender_messages: [], + rerender_messages_next_same_sender: [], }; var first_group; var second_group; @@ -447,7 +447,7 @@ MessageListView.prototype = { } else { if (was_joined) { // rerender the last message - message_actions.rerender_messages.push(prev_msg_container); + message_actions.rerender_messages_next_same_sender.push(prev_msg_container); message_actions.append_messages = _.first(new_message_groups).message_containers; new_message_groups = _.rest(new_message_groups); } else if (first_group !== undefined && second_group !== undefined) { @@ -704,15 +704,18 @@ MessageListView.prototype = { }); } - // Rerender message rows - if (message_actions.rerender_messages.length > 0) { - _.each(message_actions.rerender_messages, function (message_container) { - var old_row = self.get_row(message_container.msg.id); - var row = $(self._get_message_template(message_container)); - self._post_process(row); - old_row.replaceWith(row); - condense.condense_and_collapse(row); - list.reselect_selected_id(); + // Update the rendering for message rows which used to be last + // and now know whether the following message has the same + // sender. + // + // It is likely the case that we can just remove the block + // entirely, since it appears the next_is_same_sender CSS + // class doesn't do anything. + if (message_actions.rerender_messages_next_same_sender.length > 0) { + _.each(message_actions.rerender_messages_next_same_sender, function (message_container) { + var row = self.get_row(message_container.msg.id); + $(row).find("div.messagebox").toggleClass("next_is_same_sender", + message_container.next_is_same_sender); }); }