From 120ff78516ea77b458368d99031ac7436fbbfbf7 Mon Sep 17 00:00:00 2001
From: Aman Agrawal <amanagr@zulip.com>
Date: Thu, 9 Nov 2023 08:33:30 +0000
Subject: [PATCH] narrow: Fix message row partially visible on narrow.

This will prevent any message we want to select after narrowing
from being offscreen entirely or partially.

Steps to reproduce the bug:
* `./manage.py populate_db -n 3000 --max-topics=2`
* Narrow to a stream and scroll high up.
* Align two recipient bars together with nothing between them.
* Click on the first recipient bar after keeping the selected message
  on the second recipient bar.
* You will see that the selected message is not in view.
---
 web/src/narrow.js                 | 23 ++++++++++++++++++++++-
 web/tests/narrow_activate.test.js | 30 +++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/web/src/narrow.js b/web/src/narrow.js
index 8767b69e84..f73f7269b3 100644
--- a/web/src/narrow.js
+++ b/web/src/narrow.js
@@ -384,10 +384,31 @@ export function activate(raw_terms, opts) {
             // We override target_id in this case, since the user could be
             // having a near: narrow auto-reloaded.
             id_info.target_id = opts.then_select_id;
+            // Position selected row to not scroll off-screen.
             if (opts.then_select_offset === undefined) {
                 const $row = message_lists.current.get_row(opts.then_select_id);
                 if ($row.length > 0) {
-                    opts.then_select_offset = $row.get_offset_to_window().top;
+                    const row_props = $row.get_offset_to_window();
+                    const navbar_height = $("#navbar-fixed-container").height();
+                    // 30px height + 10px top margin.
+                    const compose_box_top = $("#compose").get_offset_to_window().top;
+                    const sticky_header_outer_height = 40;
+                    const min_height_for_message_top_visible =
+                        navbar_height + sticky_header_outer_height;
+
+                    if (
+                        // We want to keep the selected message in the same scroll position after the narrow changes if possible.
+                        // Row top should be below the sticky header.
+                        row_props.top >= min_height_for_message_top_visible &&
+                        // Row top and some part of message should be above the compose box.
+                        row_props.top + 10 <= compose_box_top
+                    ) {
+                        // Use the same offset of row in the new narrow as it is in the current narrow.
+                        opts.then_select_offset = row_props.top;
+                    } else {
+                        // Otherwise, show selected message below the sticky header.
+                        opts.then_select_offset = min_height_for_message_top_visible;
+                    }
                 }
             }
         }
diff --git a/web/tests/narrow_activate.test.js b/web/tests/narrow_activate.test.js
index c1a25dc484..1a52daf00f 100644
--- a/web/tests/narrow_activate.test.js
+++ b/web/tests/narrow_activate.test.js
@@ -186,6 +186,9 @@ run_test("basics", ({override}) => {
         last: () => ({id: 1100}),
     };
 
+    $("#navbar-fixed-container").set_height(40);
+    $("#compose").get_offset_to_window = () => ({top: 200});
+
     message_fetch.load_messages_for_narrow = (opts) => {
         // Only validates the anchor and set of fields
         assert.deepEqual(opts, {
@@ -202,7 +205,9 @@ run_test("basics", ({override}) => {
     });
 
     assert.equal(message_lists.current.selected_id, selected_id);
-    assert.equal(message_lists.current.view.offset, 25);
+    // 25 was the offset of the selected message but it is low for the
+    // message top to be visible, so we use set offset to navbar height + header height.
+    assert.equal(message_lists.current.view.offset, 80);
     assert.equal(narrow_state.narrowed_to_pms(), false);
 
     helper.assert_events([
@@ -233,4 +238,27 @@ run_test("basics", ({override}) => {
     });
 
     assert.equal(narrow_state.narrowed_to_pms(), true);
+
+    message_lists.current.selected_id = () => -1;
+    // Row offset is between navbar and compose, so we keep it in the same position.
+    row.get_offset_to_window = () => ({top: 100, bottom: 150});
+    message_lists.current.get_row = () => row;
+
+    narrow.activate(terms, {
+        then_select_id: selected_id,
+    });
+
+    assert.equal(message_lists.current.view.offset, 100);
+
+    message_lists.current.selected_id = () => -1;
+    // Row is below navbar and row bottom is below compose but since the message is
+    // visible enough, we don't scroll the message to a new position.
+    row.get_offset_to_window = () => ({top: 150, bottom: 250});
+    message_lists.current.get_row = () => row;
+
+    narrow.activate(terms, {
+        then_select_id: selected_id,
+    });
+
+    assert.equal(message_lists.current.view.offset, 150);
 });