From 7d4ec1f93bc3b56d6eb205a4a0fc7b579f064a38 Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@zulip.com>
Date: Wed, 31 Jan 2024 09:50:51 -0800
Subject: [PATCH] narrow: Open compose box before rendering main message feed.

As discussed in the new comments, we had a bug where the
system-initiated animated scroll that happens when the compose box
opens as a result of narrowing would race with the internal
rerendering that occurs when the message_fetch request asking the
server for additional data returns.

The correct fix for this is just to open the compose box, if we're
going to do so, before setting the user's scroll position in the
narrowing/rendering process.

This ends up being a UI improvement (in that the compose box is
available for typing a bit earlier) as well as avoiding both the risk
of this race as well as the bad UX of adjusting the user's scroll
position multiple times as part of entering the view.

This does not address an as-yet-unknown bug wherein the animated
scroll that occurs when opening the compose box, when racing with a
background rerender, results in a bogus ending scroll position, though
it's easy to see how that might occur given that rerendering does
clear the DOM briefly.
---
 web/src/compose_actions.js        | 23 +++++++++++++++++------
 web/src/narrow.js                 |  8 +++++++-
 web/tests/narrow_activate.test.js |  2 +-
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/web/src/compose_actions.js b/web/src/compose_actions.js
index 526a666ee3..a4eabbf578 100644
--- a/web/src/compose_actions.js
+++ b/web/src/compose_actions.js
@@ -90,11 +90,11 @@ function clear_box() {
     $(".compose_control_button_container:has(.add-poll)").removeClass("disabled-on-hover");
 }
 
-export function autosize_message_content() {
+export function autosize_message_content(opts) {
     if (!compose_ui.is_full_size()) {
         autosize($("textarea#compose-textarea"), {
             callback() {
-                maybe_scroll_up_selected_message();
+                maybe_scroll_up_selected_message(opts);
             },
         });
     }
@@ -112,7 +112,7 @@ export function complete_starting_tasks(msg_type, opts) {
     // by compose.start() for now.  Having this as a separate function
     // makes testing a bit easier.
 
-    maybe_scroll_up_selected_message();
+    maybe_scroll_up_selected_message(opts);
     compose_fade.start_compose(msg_type);
     if (msg_type === "stream") {
         stream_bar.decorate(
@@ -125,7 +125,11 @@ export function complete_starting_tasks(msg_type, opts) {
     compose_recipient.update_narrow_to_recipient_visibility();
 }
 
-export function maybe_scroll_up_selected_message() {
+export function maybe_scroll_up_selected_message(opts) {
+    if (!opts.skip_scrolling_selected_message) {
+        return;
+    }
+
     // If the compose box is obscuring the currently selected message,
     // scroll up until the message is no longer occluded.
     if (message_lists.current.selected_id() === -1) {
@@ -194,7 +198,7 @@ export function start(msg_type, opts) {
     }
 
     popovers.hide_all();
-    autosize_message_content();
+    autosize_message_content(opts);
 
     if (reload_state.is_in_progress()) {
         return;
@@ -456,7 +460,14 @@ export function on_narrow(opts) {
                 return;
             }
         }
-        start("private");
+
+        // Open the compose box, passing the option to skip attempting
+        // an animated adjustment to scroll position, which is useless
+        // because we are called before the narrowing process has set
+        // the view's scroll position. recenter_view is responsible
+        // for taking the open compose box into account when placing
+        // the selecting message.
+        start("private", {skip_scrolling_selected_message: true});
         return;
     }
 
diff --git a/web/src/narrow.js b/web/src/narrow.js
index f73f7269b3..3a8b574e4a 100644
--- a/web/src/narrow.js
+++ b/web/src/narrow.js
@@ -508,6 +508,13 @@ export function activate(raw_terms, opts) {
             });
         }
 
+        // Important: We need to consider opening the compose box
+        // before calling update_selection, so that the logic in
+        // recenter_view for positioning the currently selected
+        // message can take into account the space consumed by the
+        // open compose box.
+        compose_actions.on_narrow(opts);
+
         if (select_immediately) {
             update_selection({
                 id_info,
@@ -525,7 +532,6 @@ export function activate(raw_terms, opts) {
 
         handle_post_view_change(msg_list);
 
-        compose_actions.on_narrow(opts);
 
         unread_ui.update_unread_banner();
 
diff --git a/web/tests/narrow_activate.test.js b/web/tests/narrow_activate.test.js
index 1a52daf00f..ac24eb1d94 100644
--- a/web/tests/narrow_activate.test.js
+++ b/web/tests/narrow_activate.test.js
@@ -215,6 +215,7 @@ run_test("basics", ({override}) => {
         [message_feed_loading, "hide_indicators"],
         [message_lists, "save_pre_narrow_offset_for_reload"],
         [compose_banner, "clear_message_sent_banners"],
+        [compose_actions, "on_narrow"],
         [unread_ops, "process_visible"],
         [narrow_history, "save_narrow_state_and_flush"],
         [message_viewport, "stop_auto_scrolling"],
@@ -226,7 +227,6 @@ run_test("basics", ({override}) => {
         [narrow_title, "update_narrow_title"],
         [left_sidebar_navigation_area, "handle_narrow_activated"],
         [stream_list, "handle_narrow_activated"],
-        [compose_actions, "on_narrow"],
         [compose_recipient, "handle_middle_pane_transition"],
     ]);