search

android / nowinandroid

A fully functional Android app built entirely with Kotlin and Jetpack Compose
Apache License 2.0
16.64k stars 3.01k forks source link

[Bug]: Scrollbar is hardly stable #1526

Open Jaehwa-Noh opened 3 months ago

Jaehwa-Noh commented 3 months ago

Is there an existing issue for this?

Is there a StackOverflow question about this issue?

What happened?

Scrollbar is hardly stable. When I drag down and drag up, you can see the scrollbar position jump other place. Screen_recording_20240703_165416.webm

https://github.com/android/nowinandroid/assets/48680511/16aa3e05-39e2-4b5e-95e8-ee409ce21809

Relevant logcat output

No response

Code of Conduct

SimonMarquis commented 3 months ago

Interesting, is it only present on large screen devices, or also on regular phone width?

Jaehwa-Noh commented 3 months ago

@SimonMarquis I'd not tested it on regular and tablet yet. What I used a device in the video was Resizable API34 emulator with Foldable size.

anhtuannd commented 2 months ago

I think it happens due to there is one spacer at the end of stagger list. https://github.com/android/nowinandroid/blob/50b13ecb21138b76e14ff0b2f4cf1dcfda9d0f43/feature/foryou/src/main/kotlin/com/google/samples/apps/nowinandroid/feature/foryou/ForYouScreen.kt#L200 Since ScrollBar (thumb position and thumb size) is calculated based on first visible items and percentage of visible items, not by real position, and all other items are long layout, it causes miscalculating when you scroll to end of the list. https://github.com/android/nowinandroid/blob/50b13ecb21138b76e14ff0b2f4cf1dcfda9d0f43/core/designsystem/src/main/kotlin/com/google/samples/apps/nowinandroid/core/designsystem/component/scrollbar/ScrollbarExt.kt#L204

rodrigodiasferreira commented 2 months ago

Hi @anhtuannd , @Jaehwa-Noh , @SimonMarquis , I've done some investigation about this issue, the reason for this behavior is indeed the itemsVisible and the Spacer as commented by @anhtuannd . When it is close to the end, and in one of the lanes there is no additional news resource, then the StaggeredGridLayoutInfo.visibleItemsInfo list the Spacer, which is way down, which makes the calculation logic of the items visible broke, since the Spacer is out of the viewport. And it happens also for some news resource that are before the viewport and the StaggeredGridLayoutInfo.visibleItemsInfo also list them. In order to make it smoother this behavior, I added some safeguard to do not consider the size of such items that are outside the viewport. That fixed the issue, you guys may review and test, when you got some time. If there is any improvement that you see, please let me know. Here it is the PR that fixes this issue: https://github.com/android/nowinandroid/pull/1553

Since, I think it is weird the StaggeredGridLayoutInfo.visibleItemsInfo list the items that are out of the viewport, as the name says, it should list the visible items (if it is out of the viewport, it is not visible), so I opened an issue to compose team to take a look into this, and if they agree it is an issue, so they can be aware and fix, when they have manpower for that: https://issuetracker.google.com/issues/353143657

There is still one improvement to be done (but I think it is another issue, may I open a new one, or you guys prefer to be handled on this one?). I will investigate: it is the scenario when some visibleItemsInfo is not visible anymore, the function: interpolateFirstItemIndex is not having a continuous index continuation, so one may see still some small jumps. I will check, if I can came up with some equation that may keep it smooth on those scenarios, and improve even more the scroll bar for multi lane scenario.

@anhtuannd , about the improvement that I've done on the PR: https://github.com/android/nowinandroid/pull/1550, actually this fix, that I've done, it makes sense to use the safeguard of not using the items out of the viewport for the types: LazyListState, LazyGridState, LazyStaggeredGridState, as you can see on the PR, it was fixed on the common function: itemVisibilityPercentage. So, the improvement is still valid. I will add this comment on that PR too, to answer to you. Thanks for raising this concern.

rodrigodiasferreira commented 2 months ago

Hi @anhtuannd , @Jaehwa-Noh , @SimonMarquis , I was able to fix the other part of the issue related to the interpolated index. This change, I wasn't able to make it independent PR, as it uses the genericScrollbarState function that I created before. I had to make the fix on top of the refactoring:

@anhtuannd , this one also, did not require to separate the function: genericScrollbarState, so I think the refactoring is still a good improvement to reduce logic and code repetition.

On my tests, it looks pretty smooth now. If you find any problems, please let me know.

Thanks.

rodrigodiasferreira commented 1 month ago

Hi @anhtuannd , @Jaehwa-Noh , @SimonMarquis, Just giving an update about the issue that I opened to compose team: https://issuetracker.google.com/issues/353143657, they just fixed. As soon as it is available on some version, will update the PR in order to update the library, and remove my fix related to the visibleItems, probably it won't be necessary anymore.