message_view: Update autoscroll on message sent. #23539
Conversation
zulipbot
added
size: XL
area: message feed (uncategorized)
priority: high
UI experiment
|
Hello @zulip/design, @zulip/server-message-view members, this pull request was labeled with the "area: message view", "UI experiment" labels, so you may want to check it out! |
static/js/message_list_view.js
Outdated
| // Ok, we are finally ready to actually scroll. | ||
| // Find out how close we are to the bottom of the feed. The +1 is to make sure we get | ||
| // consistent behavior when composing a message right after an autoscroll. | ||
| const scroll_amount = $message_viewport.offset_from_bottom($last_visible, true) + 1; | ||
| if (scroll_amount > 0) { |
There was a problem hiding this comment.
Will this always be true? Or can $message_viewport.offset_from_bottom be negative?
There was a problem hiding this comment.
As specified in $message_viewport.offset_from_bottom's comment, the return value is positive if the last row is hidden. If we scroll all the way to the bottom past the last row, the return value would be negative and it would not be necessary to scroll.
There was a problem hiding this comment.
OK, so this is a check on whether the previous last message was visible or not?
Also, why is that the correct amount to scroll, then? Our previous logic based the scroll amount on the height of the new message; what's the thinking behind this calculation being correct?
There was a problem hiding this comment.
So the previous behavior had a case where it did a partial scroll if the height of the new message was smaller than the scroll_limit, with the scroll_limit being the distance between the selected message and the top of the screen.
Since we no longer care about pushing the selected message off screen, the logic can be simplified. This calculation is sufficient since the height of the new message is included in the offset_from_bottom calculation. As long as the bottom of the last message will be visible when the compose box is empty, we want to scroll to the bottom of the feed.
static/js/message_list_view.js
Outdated
|
|
||
| // Ok, we are finally ready to actually scroll. | ||
| // Find out how close we are to the bottom of the feed. The +1 is to make sure we get | ||
| // consistent behavior when composing a message right after an autoscroll. |
There was a problem hiding this comment.
Can you explain specifically what happens if we don't have this +1?
There was a problem hiding this comment.
Lets say we post a message that triggers the autoscroll. The view would then scroll down to the bottom of the feed. If we then instantly write another message, offset_from_bottom would return 0 since we were just moved to the bottom. The scroll would then not trigger on the second message and our newest message would be hidden from us.
I realize now that a neater solution would be to remove the +1 and use >= on L1017...
There was a problem hiding this comment.
Yeah, that sounds good -- can you make that tweak?
There was a problem hiding this comment.
The tweak has been made. However I'm having a slightly different off-by-one problem.
After running system_initiated_animate_scroll with the value from offset_from_bottom, value returned from calling offset_from_bottom again is consistently off by about 0.1 to 1 px.
In order to address this I changed message_viewport.js's is_scrolled_up method to have an error margin of 1 px.
This seems to work well but I'm not quite sure why the scroll method doesn't actually go all the way down. Do you have any thoughts on this?
timabbott
added
the
product review
JoeyPriceless
force-pushed
the
scroll_to_sent_message
branch
from
49fb9f5 to
1c66aa0
Compare
|
@timabbott Thanks for the review and sorry for the late response on my part. I've updated the commit to address your comments. I also removed the return value of _maybe_autoscroll since it is no longer necessary. |
|
Sorry this got lost! There was no comment tagging me, so I was not aware that my review was the next step on this PR. |
|
It's a bit hard to figure out how to test, but I was able to generate an example where the feed scrolled correctly when I sent a message, and the blue box moved down. |
|
Anyway, I think we could try this on CZO. This issue was reported again today, and I'd love to see an improvement here. I'm wondering if we'll actually want to be more aggressive with scrolling, and do so any time the bottom message in the feed is at least partially in view, rather than requiring the bottom of the feed to be visible. But we can start with this. @timabbott tagging for another look from you / CZO deployment if/when it's ready. |
alya
added
chat.zulip.org review
|
@JoeyPriceless Would you be up for getting this PR rebased? |
amanagr
force-pushed
the
scroll_to_sent_message
branch
from
1c66aa0 to
d75c606
Compare
amanagr
force-pushed
the
scroll_to_sent_message
branch
from
d75c606 to
41ead8b
Compare
|
Is there any hope of getting this PR merged? This is a much needed feature. |
alya
added
the
post release
|
@timabbott Let's get this tested soon after the release. The current behavior has been regularly bugging me as well. |
|
some folks on our team are also hoping for this feature. as you mentioned, @alya, you wanted this to get tested "soon after the release". I'm assuming you mean release 7.0, which as of this post is 1 month old; furthermore there's a 7.1 release 2 weeks old, now, which apparently still doesn't have this feature in it, going by the "open" status. I understand product dev, and QA in particular, is a big process. Just wondering if this is still something you're eyeing as part of that process or if it got kinda forgotten again. |
Updates the autoscroll behavior when sending a message. Will now autoscroll to the bottom of the message list if bottom of the view is visible, moving the message selection to the message at the top of the view. Concerns message_list_view.js and message_viewport.js. Fixes zulip#23298
shameondev
force-pushed
the
scroll_to_sent_message
branch
from
41ead8b to
433db9a
Compare
|
Hey @timabbott, I've added this PR to your "integration review" list. If I'm getting it right, we'd like to give it some time to settle in CZO after the release. |
shameondev
added
the
integration review
timabbott
force-pushed
the
main
branch
2 times, most recently
from
4ec3636 to
88b200c
Compare
|
Heads up @JoeyPriceless, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
looking forward to this issue being fixed — we have just started using Zulip as a Slack alternative and are all finding that you have to click scroll after sending a message really difficult to get past 🤞 |
alya
added
completion candidate
|
Closing as this effort was finally integrated via #29398; thanks for the work on this @JoeyPriceless! |
Updates the autoscroll behavior when sending a message. Will now autoscroll to the bottom of the message list if bottom of the view is visible, moving the message selection to the message at the top of the view.
Concerns message_list_view.js and message_viewport.js.
Fixes #23298
Design considerations
When designing the implementation, I had to move the code that reduces the scrolling depending on how much the compose box shrinks after being cleared. This is because it was now needed to determine whether the user had scrolled up at the beginning of the render function while taking into account the size of the written message. I was unsure whether to add an argument to is_scrolled_up() and offets_from_bottom(). Ultimately I decided to add arguments since this is the only context in which these functions are used. Please tell me if you'd like to me to handle this differently.
Screen capture:
