Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scroll to sent message when bottom of the screen is in view #23298

Closed
alya opened this issue Oct 19, 2022 · 7 comments · Fixed by #29398
Closed

Scroll to sent message when bottom of the screen is in view #23298

alya opened this issue Oct 19, 2022 · 7 comments · Fixed by #29398
Labels
area: message feed (scrolling) Scroll behavior, performance, and side-effects (marking as read) help wanted post release Issues to focus attention on after the current major release priority: high UI experiment Design direction to be evaluated

Comments

@alya
Copy link
Contributor

alya commented Oct 19, 2022

At present, when the user sends a message, and the bottom of the message view is on-screen, we scroll to show the sent message if we can do so without moving the blue selection box. As reported on CZO, this can be confusing to users, as they may not see the message they just sent.

Instead, we should scroll to show the newly sent message any time the bottom of the message view is on-screen. The selection box should just move down to the top message on the screen if needed.

We should leave other checks as they currently are, including active.client_is_active and whether popovers are open.

@zulipbot
Copy link
Member

zulipbot commented Oct 19, 2022

Hello @zulip/design members, this issue was labeled with the "UI experiment" label, so you may want to check it out!

@timabbott
Copy link
Member

I believe the logic that is requested to change here is _maybe_autoscroll; in particular this block:

        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;                                                                
            const offset = $message_viewport.offset_from_bottom($last_visible);                          
                                                                                                         
            // For determining whether we need to show the user a "you                                   
            // need to scroll down" notification, the obvious check                                      
            // would be `offset > scroll_amount`, and that is indeed                                     
            // correct with a 1-line message in the compose box.                                         
            // However, the compose box is open with the content of                                      
            // the message just sent when this code runs, and                                            
            // `offset_from_bottom` if an offset from the top of the                                     
            // compose box, which is about to be reset to empty.  So                                     
            // to compute the offset at the time the user might see                                      
            // this notification, we need to adjust by the amount that                                   
            // the current compose is bigger than the empty, open                                        
            // compose box.                                                                              
            const compose_textarea_default_height = 42;                                                  
            const compose_textarea_current_height = $("#compose-textarea").height();                     
            const expected_change =                                                                      
                compose_textarea_current_height - compose_textarea_default_height;                       
            const expected_offset = offset - expected_change;                                            
            need_user_to_scroll = expected_offset > scroll_amount;                                       
        }                                                                                                
                                                                                                         
        // Ok, we are finally ready to actually scroll.                                                  
        if (scroll_amount > 0) {                                                                         
            $message_viewport.system_initiated_animate_scroll(scroll_amount);                            
        }                                                                                                

The caller already does a check (search for started_scrolled_up) to cover the case where the bottom whitespace is not visible.

I think basically the plan would be to skip messing with a scroll_limit, and just scroll a distance based on the height of the new message. It's likely that logic around the compose box height would be desired in both code paths. And then this function:

export function system_initiated_animate_scroll(scroll_amount) {                                         
    message_scroll.suppress_selection_update_on_next_scroll();                                           
    const viewport_offset = scrollTop();                                                                 
    in_stoppable_autoscroll = true;                                                                      
    $message_pane.animate({                                                                              
        scrollTop: viewport_offset + scroll_amount,                                                      
        always() {                                                                                       
            in_stoppable_autoscroll = false;                                                             
        },                                                                                               
    });                                                                                                  
}                                                                                                        

Would likely need the message_scroll.suppress_selection_update_on_next_scroll removed, so that we move the selection when the automated scroll would push it offscreen.

@JoeyPriceless
Copy link
Collaborator

@zulipbot claim

JoeyPriceless added a commit to JoeyPriceless/zulip that referenced this issue Nov 11, 2022
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
@JoeyPriceless
Copy link
Collaborator

@alya @timabbott I've opened a PR that implements the desired behavior at #23539. :)

It pretty much follows Tim's suggested solution.

@zulipbot
Copy link
Member

zulipbot commented Nov 29, 2022

@JoeyPriceless You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

JoeyPriceless added a commit to JoeyPriceless/zulip that referenced this issue Dec 5, 2022
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
@JoeyPriceless
Copy link
Collaborator

@zulipbot claim

@zulipbot
Copy link
Member

zulipbot commented Dec 25, 2022

@JoeyPriceless You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

amanagr pushed a commit to JoeyPriceless/zulip that referenced this issue Mar 2, 2023
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
amanagr pushed a commit to JoeyPriceless/zulip that referenced this issue Mar 2, 2023
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
@alya alya added the post release Issues to focus attention on after the current major release label May 19, 2023
shameondev pushed a commit to JoeyPriceless/zulip that referenced this issue Jun 29, 2023
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
@laurynmm laurynmm added area: message feed (scrolling) Scroll behavior, performance, and side-effects (marking as read) and removed area: message feed (uncategorized) labels Sep 19, 2023
N-Shar-ma pushed a commit to N-Shar-ma/zulip that referenced this issue Mar 21, 2024
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.

Fixes zulip#23298.
N-Shar-ma added a commit to N-Shar-ma/zulip that referenced this issue Mar 21, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message that was just added, updating the selected message if necessary.

Fixes: zulip#23298.
N-Shar-ma added a commit to N-Shar-ma/zulip that referenced this issue Mar 21, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message/s just sent by the current user, updating the selected message
if necessary.

Scroll behaviour for messages sent by other users remains unchanged.

Fixes: zulip#23298.
N-Shar-ma added a commit to N-Shar-ma/zulip that referenced this issue Apr 2, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message/s just sent by the current user, updating the selected message
if necessary.

Scroll behaviour for messages sent by other users remains unchanged.

Fixes: zulip#23298.
timabbott pushed a commit that referenced this issue Apr 2, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message/s just sent by the current user, updating the selected message
if necessary.

Scroll behaviour for messages sent by other users remains unchanged.

Fixes: #23298.
timabbott pushed a commit that referenced this issue Apr 3, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message/s just sent by the current user, updating the selected message
if necessary.

Scroll behaviour for messages sent by other users remains unchanged.

Fixes: #23298.
timabbott pushed a commit to timabbott/zulip that referenced this issue Apr 4, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message/s just sent by the current user, updating the selected message
if necessary.

Scroll behaviour for messages sent by other users remains unchanged.

Fixes: zulip#23298.
timabbott pushed a commit that referenced this issue Apr 11, 2024
Now whenever the bottom of the feed is visible, we always scroll to the
message/s just sent by the current user, updating the selected message
if necessary.

Scroll behaviour for messages sent by other users remains unchanged.

Fixes: #23298.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message feed (scrolling) Scroll behavior, performance, and side-effects (marking as read) help wanted post release Issues to focus attention on after the current major release priority: high UI experiment Design direction to be evaluated
Projects
Status: Done
5 participants