Skip to content

Control scroll position on new and newly-fetched messages #83

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

Open
gnprice opened this issue Apr 21, 2023 · 2 comments · May be fixed by #1288
Open

Control scroll position on new and newly-fetched messages #83

gnprice opened this issue Apr 21, 2023 · 2 comments · May be fixed by #1288
Labels
a-msglist The message-list screen, except what's label:a-content

Comments

@gnprice
Copy link
Member

gnprice commented Apr 21, 2023

Currently if you're looking at the message list and then a new message comes in, you'll see the messages on the screen jump. This is exactly the right thing (*) if you're scrolled to the bottom — the new message appears at the bottom, and the existing messages all move up to make room. But if you're scrolled further up, then we should keep steady on the screen the messages you're looking at.

Instead, the current behavior is that you see all the messages jump up.

Fundamentally what's happening is that we use a (variant of a) ListView, which models the contents as a sequence of items indexed 0, 1, 2, 3, etc.; and then we pass reverse: true so that item 0 is shown at the bottom, and interpret item index i as "the i'th message in the list, counting zero-based from the end". So if we're looking around item 37 (the message 37 slots from the end), and then a new message arrives, then as far as the ListView knows we should still be looking around item 37 — but that's now the message that was item 36 a moment ago.

A good 90% solution here would be to build the message list out of two list slivers facing back to back, rather than one list sliver facing upward. This is a standard Flutter solution for having a list that grows in both directions; see example in the CustomScrollView doc.

(The list sliver class in that example is SliverList, which corresponds to a basic ListView. We'd use our SliverStickyHeaderList, which corresponds to our StickyHeaderListView.)

(*) Well, possibly it'd be better to have some kind of sliding animation rather than a sudden jump. But that'd be its own issue. This issue is about the new state of things that would be the endpoint of any animation.


Notes on possible followups:

That two-sliver solution would completely solve the problem for adding more messages (or removing messages) at the very top or bottom, which covers (a) message events and (b) fetching messages on scroll, #78. We'd still have some jumps on (c) deleting messages, or moving them away, when they're not the very oldest or latest; (d) moving messages into the middle of the list; (e) likely on some cases of editing messages so that their height changes; (f) similarly on a message going from no reactions to some reactions or back, or other changes that affect its displayed height.

For (c) and (d), I think the Flutter API has the hooks we'd need (we'd make our own implementation of RenderSliverBoxChildManager), but fundamentally it's a subtle problem; in particular we'd need a richer data structure for the list of messages than a List or Queue.

For (e) and (f), and for that matter as a possible alternative solution for (c) and (d) or even (a) and (b), I believe we could use a ScrollController. When something changes, we'd calculate what the right scroll position is in the new state of things, and call jumpTo to go there. Or perhaps we'd override ScrollController.createScrollPosition? The example of PageController, mentioned in that doc, sounds potentially promising as a model.

@gnprice gnprice added the m-beta label May 26, 2023
@gnprice gnprice added this to the Beta milestone May 27, 2023
@gnprice gnprice added a-msglist The message-list screen, except what's label:a-content and removed m-beta labels May 27, 2023
@gnprice gnprice modified the milestones: Beta 1, Beta 2 Nov 8, 2023
@gnprice gnprice modified the milestones: Beta 2, Beta 3 Nov 22, 2023
@gnprice gnprice modified the milestones: B2: Summer 2024, Launch May 9, 2024
@gnprice
Copy link
Member Author

gnprice commented Jul 31, 2024

This 90% solution:

A good 90% solution here would be to build the message list out of two list slivers facing back to back, rather than one list sliver facing upward.

will happen as part of #82. So this issue tracks going further than that.

@gnprice gnprice modified the milestones: Launch, Post-launch Jul 31, 2024
@gnprice gnprice added upstream Would benefit from work in Flutter or another upstream and removed upstream Would benefit from work in Flutter or another upstream labels Nov 22, 2024
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e linked a pull request Jan 19, 2025 that will close this issue
@E-m-i-n-e-n-c-e
Copy link
Contributor

Started working on this.

https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Control.20scroll.20position.20on.20new.20and.20newly-fetched.20messages

20250119-1825-45.3281596.mp4
20250119-1831-50.5506127.mp4

gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 8, 2025
This is NFC for the behavior at initial fetch.  But thereafter, with
this change, messages never move between slivers, and new messages
go into the bottom sliver.

I believe the main user-visible consequence of this change is that if
the user is scrolled up in history and then a new message comes in,
the new message will no longer cause all the messages to shift upward.
This is the "90% solution" to zulip#83.

On the other hand, if the user is scrolled all the way to the end,
then they still remain that way when a new message comes in --
there's specific logic to ensure that in MessageListScrollPosition,
and an existing test in test/widgets/message_list_test.dart verifies
it end to end.

The main motivation for this change is that it brings us closer to
having a `fetchNewer` method, and therefore to being able to have the
message list start out in the middle of history.

This change also allows us to revert a portion of fca651b, where
a test had had to be weakened slightly because messages began to
get moved between slivers.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 13, 2025
This is NFC for the behavior at initial fetch.  But thereafter, with
this change, messages never move between slivers, and new messages
go into the bottom sliver.

I believe the main user-visible consequence of this change is that if
the user is scrolled up in history and then a new message comes in,
the new message will no longer cause all the messages to shift upward.
This is the "90% solution" to zulip#83.

On the other hand, if the user is scrolled all the way to the end,
then they still remain that way when a new message comes in --
there's specific logic to ensure that in MessageListScrollPosition,
and an existing test in test/widgets/message_list_test.dart verifies
it end to end.

The main motivation for this change is that it brings us closer to
having a `fetchNewer` method, and therefore to being able to have the
message list start out in the middle of history.

This change also allows us to revert a portion of fca651b, where
a test had had to be weakened slightly because messages began to
get moved between slivers.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 16, 2025
This is NFC for the behavior at initial fetch.  But thereafter, with
this change, messages never move between slivers, and new messages
go into the bottom sliver.

I believe the main user-visible consequence of this change is that if
the user is scrolled up in history and then a new message comes in,
the new message will no longer cause all the messages to shift upward.
This is the "90% solution" to zulip#83.

On the other hand, if the user is scrolled all the way to the end,
then they still remain that way when a new message comes in --
there's specific logic to ensure that in MessageListScrollPosition,
and an existing test in test/widgets/message_list_test.dart verifies
it end to end.

The main motivation for this change is that it brings us closer to
having a `fetchNewer` method, and therefore to being able to have the
message list start out in the middle of history.

This change also allows us to revert a portion of fca651b, where
a test had had to be weakened slightly because messages began to
get moved between slivers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-msglist The message-list screen, except what's label:a-content
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants