Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Tim Abbott 2019-02-25 09:59:44 -08:00
parent 93f9082071
commit cc8021a742
2 changed files with 27 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
frontend_tests/node_tests/message_list_view.js
View File

@ -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.

25
static/js/message_list_view.js
View File

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