floating_recipient_bar: Replace with sticky header. #23900

amanagr · 2022-12-19T08:32:18Z

Continuation of #9910
discussion: https://chat.zulip.org/#narrow/stream/101-design/topic/UI.20redesign.3A.20stream.2Ftopic.20header.20bar
blocker for #23782

Please read bullets in #9910 and #16762 by @andersk to check for bugs that needed to be fixed in those PRs.

There should be no visual changes other than a new possible state of the UI in the below screenshot:

amanagr · 2022-12-19T14:04:30Z

In message-basics.ts, this part of the test is failing for some reason, instead of clicking on stream header, it clicks on the search box.

    console.log("Narrowing by clicking stream");
    await page.click(`#zhome [title='Narrow to stream "Verona"']`);
    await expect_verona_stream(page);

andersk · 2022-12-23T20:10:07Z

static/js/message_list_view.js

+        /* To get to the sticky header ASAP, we take help from how far has user scrolled. */
+        if ($message_viewport.scrollTop() > $message_viewport.height() / 2) {
+            iterable_headers.reverse();
+        }
+        let sticky_header;
+        for (const header of iterable_headers) {
+            if (is_sticky(header)) {
+                sticky_header = header;
+                break;
+            }
+        }


This should use a binary search (_.sortedIndexBy).

Or, better, can we find the message row first with document.elementsFromPoint, then find the sticky header from the message?

document.elementsFromPoint is very risky to use since there are a lot of UI state we would have to take into account.

I don't understand how we can use _.sortedIndexBy or binary search here. I fear we would end up doing more work in common cases since the user is mostly looking at the latest unread message, in which case, the sticky header is just is 0-5 message headers away.

“Risky” in what way? You’re already using document.elementsFromPoint below, and you’re already doing a complicated computation with UI state in is_sticky above. If that’s risky, I don’t see this being any worse.

If there are 400 messages visible, binary search does 8 or 9 tests, while your linear search does up to 200. The potential gain if linear search happens to do 0–5 tests is small, while the potential loss is large.

I tried implementing it using document.elementsFromPoint, seems to be working fine, it would be great if you can check too!

We should definitely profile this bit of code while doing scrolling around (ideally including things like using the Mac trackpad acceleration in "all messages"), renarrowing, etc., once we've settled on how we think it's cleanest to do it.

I haven't read this carefully, but one risk with whatever we're doing here is that removing the sticky class at the start of this function and then trying to read what is in a given location might trigger an intermediate repaint or something that could be slow?

I haven't read this carefully, but one risk with whatever we're doing here is that removing the sticky class at the start of this function and then trying to read what is in a given location might trigger an intermediate repaint or something that could be slow?

Since we are not changing the position of any element by removing the sticky_header class, it most likely will repaint the date part of the message row if it is still visible in the narrow which is harmless. I will do a profile to confirm though.

&.sticky_header { .recipient_row_date { display: block; } }

I removed the method which used document.elementsFromPoint and went back to my original method and applied binary search to it as recommended by @andersk. This made some of the comments below not relevant anymore.

andersk · 2022-12-23T20:11:20Z

static/js/message_list_view.js

+            /* If user at the top of the scroll container, there is no sticky
+               header, so we just set date of the header to the first visible message. */
+            sticky_header = $headers[0];
+            $message_row = $($headers[0].nextElementSibling);


$headers.next()?

It is either $($headers[0]).next() or $($headers[0].nextElementSibling); of which the second one seems more readable, so I went with it.

$headers.first().next() then?

andersk · 2022-12-23T20:19:43Z

static/js/message_list_view.js

+        const rendered_date = timerender.render_date(time, undefined, today)[0].outerHTML;
+        $(sticky_header).find(".recipient_row_date").html(rendered_date);


I know this code was copied, but avoid the .outerHTML and .html() round trip—just .append() the jQuery object directly.

Okay, directly used html since we needed to replace the content and I didn't want to use .empty().append().

andersk · 2022-12-23T20:28:38Z

static/js/message_viewport.js

-    // The message_header is NOT considered to be part of the visible
+    // Sticky message_header is considered to be part of the visible


The inversion of this comment is wrong. You’re still adding the sticky header height to visible_top, which is subtracted out of visible_height, so it’s not considered part of the visible message pane.

Yeah, thanks for noticing.

andersk · 2022-12-23T21:05:14Z

static/styles/zulip.css

+        box-shadow: inset 0 1px 0 2px hsl(215, 47%, 50%),
+            inset 0 0 0 2px hsl(215, 47%, 50%), -1px -1px 0 0 hsl(215, 47%, 50%),
+            1px 1px 0 0 hsl(215, 47%, 50%), -1px 1px 0 0 hsl(215, 47%, 50%),
+            1px -1px 0 0 hsl(215, 47%, 50%);


One would think it shouldn’t take six shadows to draw a rectangle. We can at least replace the last four non-inset shadows (-1px -1px 0 0, 1px 1px 0 0, -1px 1px 0 0, 1px -1px 0 0) with one (0 0 0 1px); this applies to the original rule too.

(And, just noting, we’ll need to remember to remove this in #23531.)

Yeah, updated!

Where is this code to be removed (wasn't sure because this comment is on an outdated diff)?

Working on #23531 right now :)

It is in the commit css: Combine box-shadows for selected message.

Ah okay great :) So I've already addressed it. Thanks!

amanagr · 2023-01-05T07:07:28Z

@andersk thanks for the review. Updated the PR based on your comments.

alya · 2023-01-05T19:41:26Z

@andersk Any more feedback from your side, or does it look good to you now?

amanagr · 2023-01-06T13:17:19Z

@andersk updated based on your last review!

static/styles/zulip.css

timabbott · 2023-01-11T00:44:31Z

static/styles/zulip.css

+       compensate for that. */
+    .messagebox-content {
+        box-shadow: inset 0 1px 0 2px hsl(215, 47%, 50%),
+            inset 0 0 0 2px hsl(215, 47%, 50%), 0 0 0 1px hsl(215, 47%, 50%);


Should this HSL color be extracted as a variable?

And does this block need a dark theme analog?

Extracting HSL color as variable doesn't make sense here since there is no prior of doing so here.
We don't need to do anything for the dark theme since the same color is used there too.

static/js/message_list_view.js

timabbott · 2023-01-11T00:55:25Z

static/js/message_list_view.js

+            * Date separator
+            * Z logo at the top of message feed
+
+            If we encounter either loading indicator or date separator, we set sticky header to the next recipient row. */


Is there a way to write this algorithm in a way that doesn't depend on what other elements might be present there?

No longer relevant.

static/js/message_list_view.js

timabbott · 2023-01-11T00:58:26Z

static/js/message_list_view.js

+        let $message_row;
+        if (date_row.length) {
+            /* There is always a message header present next to a date_row. */
+            const recipient_row = date_row[0].nextElementSibling;


I don't like hardcoding assumptions about what elements we might have in the DOM -- could we instead do something like .nextAll(".recipient_row").first()?

An alternative option, which might be faster, might be to just assert that we got a .recipient_row in a way that'll throw an exception in development? That will probably mean if we violate this invariant in a future feature, we'll notice.

No longer relevant.

static/js/message_list_view.js

timabbott · 2023-01-11T01:03:15Z

static/js/message_list_view.js

+            if (!message_row.length) {
+                /* If there is no message row under the header, it means it is not sticky yet,
+                so we just get the message next to the header. */
+                $message_row = $sticky_header.next();


Similarly, should this be .nextAll(".message_row").first()? I think that does the same thing, but is more explicit and robust in the case we add some small/invisible div in between at some point.

No longer relevant / applied the fix elsewhere.

timabbott · 2023-01-11T01:04:21Z

static/js/message_list_view.js

+            window.innerWidth / 2,
+            visible_top + header_height - 1,
+        );
+        let $sticky_header = $(


Should this be called first_fully_visible_message_header?

Yes, but that is a long name, so added it as a comment.

timabbott · 2023-01-11T01:05:24Z

static/js/message_list_view.js

+        if (!rows_length) {
+            /* No headers are present */
+            return;
+        }


Would this be more cleanly written as having $headers = $table.find(".message_header"); happen early, doing $headers.first() as it'll be used, and checking if that exists or not?

(Just thinking about not mixing different data sources in a single calculation, given this doesn't access _rows anywhere else)

No longer relevant.

timabbott · 2023-01-11T01:10:43Z

static/js/message_viewport.js

+    const $sticky_header = $(".sticky_header");
+    if ($sticky_header.length) {
+        res.visible_top += $sticky_header.safeOuterHeight();
+    }


Just to make sure I understand, this calculation and the below one in this file are both the same as before -- the difference is just that previously, there would always be a .floating_recipient element that was possibly not visible and thus had 0 height, and now there sometimes is no such element?

timabbott · 2023-01-11T01:13:55Z

Generally, this looks very promising, and I'll be very glad for us to get to delete the various code this deletes. I posted a bunch of comments above.

I was able to find a scroll position where no date is displayed; so that's probably a bug.

I also at one point got a JS exception pop up while scrolling around "All messages",

amanagr · 2023-01-13T07:34:39Z

@timabbott updated! The bugs you faced should no longer be reproduced too!

static/js/message_list_view.js

amanagr · 2023-01-16T03:28:59Z

Can you explain what changes resolved the comments that are no longer relevant. (I've not tried to read the updated PR in full yet)

I changed most of the code in update_sticky_recipient_headers and since the code on which you reviewed are no longer present in the PR, they became irrelevant.

timabbott · 2023-01-17T19:41:30Z

@alya @andersk can you do another review on this? The algorithm in message_list_view.js is rewritten since the previous version.

alya · 2023-01-19T23:05:49Z

I didn't see any glitches when playing around with this for a bit.

timabbott · 2023-02-01T01:30:23Z

Test-deployed this on chat.zulip.org. I'm really excited about trying to get this integrated.

Since `message_viewport` library is not jQuery object, it should not have `$` prefix before it.

There are no visual changes since we are replacing 4 single borders with a single box.

In addition to the benefit of using variables, this change makes it more noticeable that the header has a different height on smaller widths.

amanagr · 2023-02-06T16:53:14Z

Pushed to resolve conflicts.

timabbott · 2023-02-08T00:27:09Z

Merged, thanks @amanagr! Hopefully we don't have further bugs to track down, but even if we do, I think they're likely to be minor enough that we can fix forward from here. I'm very glad to see that ancient code deleted, even if ideally we'd be able to make something work for the dates that would let it be entirely CSS :).

In zulip#23900, the floating recipient bar was replaced with a sticky recipient bar. The logic in `id_for_recipient_row` was not updated for that change, so we update that function to just get the first message in the group and return that message ID.

In #23900, the floating recipient bar was replaced with a sticky recipient bar. The logic in `id_for_recipient_row` was not updated for that change, so we update that function to just get the first message in the group and return that message ID.

In zulip#23900, the floating recipient bar was replaced with a sticky recipient bar. The logic in `id_for_recipient_row` was not updated for that change, so we update that function to just get the first message in the group and return that message ID.

zulipbot added the size: XL label Dec 19, 2022

This was referenced Dec 19, 2022

wip - FRB: Replace with pos:sticky attribute. #16762

Closed

WIP: Improve floating recipient bar behavior during scrolling using position: sticky #9910

Closed

amanagr force-pushed the floating_recipient_bar branch from 4e86329 to 1878d33 Compare December 19, 2022 14:14

andersk reviewed Dec 23, 2022

View reviewed changes

amanagr force-pushed the floating_recipient_bar branch from 1878d33 to f76c10b Compare January 5, 2023 07:06

amanagr force-pushed the floating_recipient_bar branch from f76c10b to 17fea8e Compare January 6, 2023 13:15

timabbott reviewed Jan 11, 2023

View reviewed changes

static/styles/zulip.css Show resolved Hide resolved

timabbott reviewed Jan 11, 2023

View reviewed changes

static/styles/zulip.css Outdated Show resolved Hide resolved

timabbott reviewed Jan 11, 2023

View reviewed changes

static/js/message_list_view.js Outdated Show resolved Hide resolved

timabbott reviewed Jan 11, 2023

View reviewed changes

static/js/message_list_view.js Outdated Show resolved Hide resolved

timabbott reviewed Jan 11, 2023

View reviewed changes

static/js/message_list_view.js Show resolved Hide resolved

timabbott reviewed Jan 11, 2023

View reviewed changes

static/js/message_list_view.js Show resolved Hide resolved

timabbott reviewed Jan 11, 2023

View reviewed changes

zulipbot added the has conflicts label Jan 11, 2023

amanagr force-pushed the floating_recipient_bar branch from 17fea8e to 57490c3 Compare January 13, 2023 07:34

zulipbot removed the has conflicts label Jan 13, 2023

timabbott reviewed Jan 15, 2023

View reviewed changes

static/js/message_list_view.js Outdated Show resolved Hide resolved

timabbott reviewed Jan 15, 2023

View reviewed changes

static/js/message_list_view.js Show resolved Hide resolved

timabbott reviewed Jan 15, 2023

View reviewed changes

static/js/message_list_view.js Show resolved Hide resolved

timabbott reviewed Jan 15, 2023

View reviewed changes

static/js/message_list_view.js Show resolved Hide resolved

amanagr force-pushed the floating_recipient_bar branch from 23d9236 to faaf0ca Compare January 16, 2023 03:55

timabbott added the product review Added by maintainers when a PR needs product review. label Jan 17, 2023

alya removed the product review Added by maintainers when a PR needs product review. label Jan 19, 2023

timabbott added the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label Jan 20, 2023

amanagr added 4 commits February 6, 2023 14:06

message_list_view: Remove wrong $ prefix in message_viewport variable.

d5e4a46

Since `message_viewport` library is not jQuery object, it should not have `$` prefix before it.

css: Combine box-shadows for selected message.

ef20006

There are no visual changes since we are replacing 4 single borders with a single box.

css: Use variables for changed header height in smaller widths.

9512ea9

In addition to the benefit of using variables, this change makes it more noticeable that the header has a different height on smaller widths.

floating_recipient_bar: Replace with sticky header.

c1202f5

amanagr force-pushed the floating_recipient_bar branch from faaf0ca to c1202f5 Compare February 6, 2023 16:53

timabbott merged commit 4dfe3d3 into zulip:main Feb 8, 2023

amanagr deleted the floating_recipient_bar branch February 8, 2023 10:40

alya mentioned this pull request Feb 24, 2023

Recipient header bar slow to update #21639

Closed

laurynmm mentioned this pull request Oct 3, 2023

recipient-row: Remove logic for floating recipient bar. #27008

Merged

12 tasks

		const rendered_date = timerender.render_date(time, undefined, today)[0].outerHTML;
		$(sticky_header).find(".recipient_row_date").html(rendered_date);

		// The message_header is NOT considered to be part of the visible
		// Sticky message_header is considered to be part of the visible

floating_recipient_bar: Replace with sticky header. #23900

floating_recipient_bar: Replace with sticky header. #23900

Conversation

amanagr commented Dec 19, 2022

amanagr commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanagr Jan 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanagr commented Jan 5, 2023

alya commented Jan 5, 2023

amanagr commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanagr Jan 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Jan 11, 2023

amanagr commented Jan 13, 2023 • edited

amanagr commented Jan 16, 2023

timabbott commented Jan 17, 2023

alya commented Jan 19, 2023

timabbott commented Feb 1, 2023

amanagr commented Feb 6, 2023

timabbott commented Feb 8, 2023

amanagr Jan 13, 2023 •

edited

amanagr Jan 13, 2023 •

edited

amanagr commented Jan 13, 2023 •

edited