scroller: invert list based on reading direction #3236
Conversation
Fixes LAND-1353. This changes our logic about whether or not to invert the list. Rather than relying on load direction, we make the invert/uninvert decision base on reading direction (whether the user perceives to be scrolling up or down). If scrolling up, we invert. If scrolling down, we "uninvert". This avoids jumpiness associated with scrolling "up" in an uninverted state, or "down" in an inverted state. This also moves a few variables around for easier readability, and moves a few variables into useMemo where it seems appropriate.
patosullivan
requested review from
dnbrwstr,
mrozanski,
arthyn and
latter-bolden
|
Moving to draft while I diagnose a bug when exiting/re-opening thread pane. |
|
There is a jump I get consistently when a thread view is closed. The scroll position in the main conversation jumps to an older point, presumably the oldest that was fetched so far. |
|
I detected something that may be related to the thread open/close issue, or at least has a similar result. On desktop, |
…/reopening threads or clicking 'go to latest'
|
Fixed the issues mariano found afaict, working on a new issue now (click to view not scrolling). |
…orceScrolls on inversion flips
|
Ok, I think we're back in a state where someone other than me should try banging on this to find if there are any remaining issues. |
|
@patosullivan here's a clip of the issue I was trying to describe in standup. Steps to reproduce:
You can sometimes see oscillating behavior when the scroller is fighting your movement, other times it stays in place and doesn't move despite the trackpad input (see first & last pause in the video). Looks like the inversion is correctly updating immediately in the scroller devtools. I think the real world case where this might occur is if you're scanning chat history quickly and you pass a message you actually want to read/focus on. Seemed to present even with a longer, more natural interval between switching directions than what's demoed. Screen.Recording.2024-02-15.at.12.25.25.PM.mov |
…les for this branch
|
I'm having a hard time reproducing the frozen state that @latter-bolden found, might need to pair to see exactly how to trigger that one. I've been looking at how to reduce the jitter on the flip between inverted/uninverted, not coming up with much as of yet. Will keep trying. |
… reading direction change logic into useEffect
|
@latter-bolden I setup a way to throttle the change in scroll direction, which reduces the frequency that a user should see any jumpyness while we invert/un-invert, and may also fix the issue you're seeing. Let me know if you're still seeing that. |
|
Testing the latest locally, on desktop, I've noticed a new issue.
After two momentum / full-stop cycles in oposite directions what I observe is that the third one stops abruptly after a fraction of a second and makes two or three short jumps back and forth. I think letting the momentum stop by itself is the trigger |
patosullivan
deleted the
po/land-1353-invert-list-only-when-appropriate
branch
This reverts the changes made to invert the scroller direction based on reading direction, from #3236 Also adds more logging so we can get some more insight into what is changing and when. Dan and I used the afternoon to try to find what was causing the issue in iOS, couldn't find anything within a reasonable amount of time. So LAND-1353 is now unresolved. We ought to pull in the @tanstack/virtual library to our shared packages so we can get some clearer debugging around what's happening in virtual and maybe modify to our needs. I looked at pulling it in, but the package itself uses workspaces and pnpm, so not super straight forward to pull in directly. We should create a ticket for it IMO.
Fixes LAND-1353.
This changes our logic about whether or not to invert the list. Rather than relying on load direction, we make the invert/uninvert decision base on reading direction (whether the user perceives to be scrolling up or down). If scrolling up, we invert. If scrolling down, we "uninvert". This avoids jumpiness associated with scrolling "up" in an uninverted state, or "down" in an inverted state (see TanStack/virtual#659). We also assume that if a user has scrolled directly to a message they are going to begin reading "down" from that message.
This also moves a few variables around for easier readability, and moves a few variables into useMemo where it seems appropriate.
Tested on local dev moons on livenet.
PR Checklist