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

message scrolling: Fix "Scroll down to view" warning. 

We recently added a feature to warn users that they
may need to scroll down to view messages that they
just sent, but it was broken due to various complexities
in the rendering code path.

Now we compute it a bit more rigorously.

It requires us to pass some info about rendering up
and down the stack, which is why it's kind of a long
commit, but the bulk of the logic is in these JS files:

    * message_list_view.js
    * notifications.js

I choose to pass structs around instead of booleans,
because I anticipate we may eventually add more metadata
about rendering to it, plus bools are just kinda brittle.
(The exceptions are that `_maybe_autoscroll`, which
is at the bottom of the stack, just passes back a simple
boolean, and `notify_local_mixes`, also at the bottom
of the stack, just accepts a simple boolean.)

This errs on the side of warning the user, even if the
new message is partially visible.

Fixes #11138
This commit is contained in:
Steve Howell 2019-01-07 20:00:03 +00:00 committed by Tim Abbott
parent 6f8da1bb27
commit b3594c984a
5 changed files with 73 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
static/js/message_events.js
View File

@ -64,25 +64,28 @@ exports.insert_new_messages = function insert_new_messages(messages, locally_ech
// other lists, so we always update this
message_util.add_new_messages(messages, message_list.all);
var render_info;
if (narrow_state.active()) {
// We do this NOW even though the home view is not active,
// because we want the home view to load fast later.
message_util.add_new_messages(messages, home_msg_list);
if (narrow_state.filter().can_apply_locally()) {
message_util.add_new_messages(messages, message_list.narrowed);
render_info = message_util.add_new_messages(messages, message_list.narrowed);
} else {
// if we cannot apply locally, we have to wait for this callback to happen to notify
maybe_add_narrowed_messages(messages, message_list.narrowed);
}
} else {
// we're in the home view, so update its list
message_util.add_new_messages(messages, home_msg_list);
render_info = message_util.add_new_messages(messages, home_msg_list);
}
if (locally_echoed) {
notifications.notify_local_mixes(messages);
var need_user_to_scroll = render_info && render_info.need_user_to_scroll;
notifications.notify_local_mixes(messages, need_user_to_scroll);
}
activity.process_loaded_messages(messages);

13
static/js/message_list.js
View File

@ -45,6 +45,12 @@ exports.MessageList.prototype = {
var bottom_messages = info.bottom_messages;
var interior_messages = info.interior_messages;
// Currently we only need data back from rendering to
// tell us whether users needs to scroll, which only
// applies for `append_to_view`, but this may change over
// time.
var render_info;
if (interior_messages.length > 0) {
self.view.rerender_the_whole_thing();
return true;
@ -54,7 +60,7 @@ exports.MessageList.prototype = {
}
if (bottom_messages.length > 0) {
self.append_to_view(bottom_messages, opts);
render_info = self.append_to_view(bottom_messages, opts);
}
if (self === exports.narrowed && !self.empty()) {
@ -69,6 +75,8 @@ exports.MessageList.prototype = {
// And also select the newly arrived message.
self.select_id(self.selected_id(), {then_scroll: true, use_closest: true});
}
return render_info;
},
get: function (id) {
@ -283,7 +291,8 @@ exports.MessageList.prototype = {
opts = _.extend({messages_are_new: false}, opts);
this.num_appends += 1;
this.view.append(messages, opts.messages_are_new);
var render_info = this.view.append(messages, opts.messages_are_new);
return render_info;
},
remove_and_rerender: function MessageList_remove_and_rerender(messages) {

56
static/js/message_list_view.js
View File

@ -749,10 +749,18 @@ MessageListView.prototype = {
if (list === current_msg_list && messages_are_new) {
if (started_scrolled_up) {
return;
return {
need_user_to_scroll: true,
};
}
var new_messages_height = self._new_messages_height(new_dom_elements);
self._maybe_autoscroll(new_messages_height);
var need_user_to_scroll = self._maybe_autoscroll(new_messages_height);
if (need_user_to_scroll) {
return {
need_user_to_scroll: true,
};
}
}
},
@ -804,17 +812,19 @@ MessageListView.prototype = {
// If we are near the bottom of our feed (the bottom is visible) and can
// scroll up without moving the pointer out of the viewport, do so, by
// up to the amount taken up by the new message.
//
// returns `true` if we need the user to scroll
var selected_row = this.selected_row();
var last_visible = rows.last_visible();
// Make sure we have a selected row and last visible row. (defensive)
if (!(selected_row && selected_row.length > 0 && last_visible)) {
return;
return false;
}
if (new_messages_height <= 0) {
return;
return false;
}
if (!activity.has_focus) {
@ -825,12 +835,15 @@ MessageListView.prototype = {
// throttled by modern Chrome's aggressive power-saving
// features.
blueslip.log("Suppressing scrolldown due to inactivity");
return;
return false;
}
// do not scroll if there are any active popovers.
if (popovers.any_active()) {
return;
// If a popover is active, then we are pretty sure the
// incoming message is not from the user themselves, so
// we don't need to tell users to scroll down.
return false;
}
var info = message_viewport.message_viewport_info();
@ -844,16 +857,31 @@ MessageListView.prototype = {
// c) scroll amount isn't really tied to size of new messages (bad)
// d) all the bad things about scrolling for users who want messages
// to stay on the screen
var scroll_amount = new_messages_height;
var scroll_amount;
var need_user_to_scroll;
if (scroll_amount > scroll_limit) {
if (new_messages_height <= scroll_limit) {
// This is the happy path where we can just scroll
// automatically, and the user will see the new message.
scroll_amount = new_messages_height;
need_user_to_scroll = false;
} else {
// Sometimes we don't want to scroll the entire height of
// the message, but our callers can give appropriate
// warnings if the message is gonna be offscreen.
// (Even if we are somewhat constrained here, the message may
// still end up being visible, so we do some arithmetic.)
scroll_amount = scroll_limit;
var offset = message_viewport.offset_from_bottom(last_visible);
need_user_to_scroll = offset > scroll_amount;
}
// Ok, we are finally ready to actually scroll.
if (scroll_amount > 0) {
message_viewport.system_initiated_animate_scroll(scroll_amount);
}
return need_user_to_scroll;
},
@ -1030,9 +1058,11 @@ MessageListView.prototype = {
append: function (messages, messages_are_new) {
var cur_window_size = this._render_win_end - this._render_win_start;
var render_info;
if (cur_window_size < this._RENDER_WINDOW_SIZE) {
var slice_to_render = messages.slice(0, this._RENDER_WINDOW_SIZE - cur_window_size);
this.render(slice_to_render, 'bottom', messages_are_new);
render_info = this.render(slice_to_render, 'bottom', messages_are_new);
this._render_win_end += slice_to_render.length;
}
@ -1041,7 +1071,13 @@ MessageListView.prototype = {
// newly received message should trigger a rerender so that
// the new message, which will appear in the viewable area,
// is rendered.
this.maybe_rerender();
var needed_rerender = this.maybe_rerender();
if (needed_rerender) {
render_info = {need_user_to_scroll: true};
}
return render_info;
},
prepend: function (messages) {

8
static/js/message_util.js
View File

@ -16,14 +16,16 @@ function add_messages(messages, msg_list, opts) {
loading.destroy_indicator($('#page_loading_indicator'));
$('#first_run_message').remove();
msg_list.add_messages(messages, opts);
var render_info = msg_list.add_messages(messages, opts);
return render_info;
}
exports.add_old_messages = function (messages, msg_list) {
add_messages(messages, msg_list, {messages_are_new: false});
return add_messages(messages, msg_list, {messages_are_new: false});
};
exports.add_new_messages = function (messages, msg_list) {
add_messages(messages, msg_list, {messages_are_new: true});
return add_messages(messages, msg_list, {messages_are_new: true});
};

19
static/js/notifications.js
View File

@ -564,15 +564,6 @@ function get_message_header(message) {
{recipient: message.display_reply_to});
}
exports.get_echo_not_in_view_reason = function () {
if (message_viewport.bottom_message_visible()) {
// If the message is visible, we do not want to notify user
return;
}
return i18n.t("Sent! Scroll down to view your message.");
};
exports.get_local_notify_mix_reason = function (message) {
var row = current_msg_list.get_row(message.id);
if (row.length > 0) {
@ -595,7 +586,7 @@ exports.get_local_notify_mix_reason = function (message) {
return i18n.t("Sent! Your message is outside your current narrow.");
};
exports.notify_local_mixes = function (messages) {
exports.notify_local_mixes = function (messages, need_user_to_scroll) {
/*
This code should only be called when we are locally echoing
messages. It notifies users that their messages aren't
@ -617,16 +608,16 @@ exports.notify_local_mixes = function (messages) {
var reason = exports.get_local_notify_mix_reason(message);
if (!reason) {
// Check if local_echo is not in view
reason = exports.get_echo_not_in_view_reason();
if (reason) {
if (need_user_to_scroll) {
reason = i18n.t("Sent! Scroll down to view your message.");
exports.notify_above_composebox(reason, "", null, "");
setTimeout(function () {
$('#out-of-view-notification').hide();
}, 3000);
}
// This is more than normal, just continue on.
// This is the HAPPY PATH--for most messages we do nothing
// other than maybe sending the above message.
return;
}