-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[0.57.0] Horizontal scroll view + pagingEnabled / snapToInterval / snapToOffsets #21116
Comments
This comment has been minimized.
This comment has been minimized.
was ok on 0.57.0-rc.3, broke in rc.4 |
@nadavmos are you able to pinpoint which commit may be responsible for this? We also have a few issues related to *Lists and they seem to be related to these commits, but not sure if they are the reason why OP is having issues: Could you test a patch/fork that reverse them and let me know if the issue disappear @mjmasn ? |
I can help shed some light, yes. I fixed the swipe-scrolling algorithm when I added snapToOffsets. Previously, it was using smoothScrollTo() for everything. There were two main issues:
As a result, swiping even a little bit sideways would sometimes scroll you all the way to the left/right or sometimes barely scroll you at all. It was very inconsistent and provided a poor UX. It also did not match iOS at all, which did not exhibit any such problems. With the changes, the algorithm is now using the standard fling() function, but limiting the offset's minimum and maximum value to both be the value it should snap to. This forces Android to use the same scrolling algorithm it would use if the snapping didn't exist. NOTE: This behavior should not match ViewPagerAndroid. A horizontal ScrollView is not meant to scroll only one snap at a time. It's meant to snap to the nearest point to where the ScrollView would scroll if there was no snapping involved. This is currently the case on both iOS and Android. |
@olegbl thanks for shedding light on this, it creates a major difference between iOS and Android where in iOS when horizontal paging is enabled no matter how hard (or soft) you fling, you'll only be able to swipe one page at a time. the current behaviour of the scrollview on android is if the paging is totally ignored and its possible to manually scroll to a position between pages without being snapped to page |
@olegbl do you have access to an iPhone with Facebook's F8 app installed? That uses
Our problem is that even if this prop did what it claims to, we need it to apply to snapToInterval / snapToOffsets as well. i.e. only ever scroll 1 page per swipe. This seemed to be the behaviour on RN 0.56 - with snapToInterval we could have views where we showed multiple 'pages' on tablet, but scrolling would still just scroll 1 'interval' at a time, and to scroll faster you just swipe repeatedly like with ViewPagerAndroid. We can't use VPA because we need to show multiple pages at a time depending on device screen width (and we're still waiting on a fix for adding/removing pages without re-rendering the whole VPA). I'll do some more testing with iOS later when I have access to a Macbook but there is definitely a huge difference between iOS and android now, and a huge change from 0.56 -> 0.57 for android. |
So it looks like we can get a not very smooth, poor man's version of the 1-page-at-a-time behaviour on Android by setting |
I'm having the same issue. Any recommendations on how to make Android swipe only one page at a time? |
Ah, you're right! It looks like on iOS, the paging algorithm is indeed slightly different from the snapping algorithm and limits transition to one page at a time. We should do that on Android as well (currently, pagingEnabled acts like snapToInterval={WIDTH_OF_SCROLLVIEW} on Android). This seems to be the major distinction that I was missing. I'll get the changes in this week. Edit: got a fix up, should be in soon-ish. I've been going over the rest of the scenarios (snapToInterval/snapToOffsets horizontal/vertical) and they feel the same across platform but pagingEnabled does end up being different.
I don't understand this one. If either pagingEnabled is set to true or snapToInterval/snapToOffsets are set, Android will always snap to a page boundary for me.
Could you elaborate on this point a bit? I don't think I understand. As per the docs, snapToInterval just completely overrides pagingEnabled.
That is not the case in the native implementation on either platform. I would think we'd want to keep the native algorithm rather than overriding it (plus the smoothScrollTo algorithm exhibited issues like #20155). Edit: for snapping just one page at a time smoothScrollTo does feel a lot better (so I'll do that for pagingEnabled). |
@olegbl thanks for looking into this one, much appreciated. Re "Could you elaborate on this point a bit?" Sure, hopefully this attempt is a bit clearer 😁 ... The current RN 0.56 behaviour is that if we turn on pagingEnabled and snapToIncrement, swiping scrolls the view one increment at a time. Whether that's intentional or not I'm unsure but it's changed in 0.57 and the 0.56 behaviour is something we'd definitely like to keep. The use case is basically having a set of pages where on a phone you'd see 1 page at a time, centered, with the slight edge of the prev/next page visible (hence can't just use pagingEnabled, because 'page' width != screen width). Then on tablet we'd hide the first/last 'spacer' pages and set a max width on the other pages, so you might see 3 or 4 pages across the screen, but we'd still want swiping to only move one increment at a time. Something roughly like this (would actually be 1 view that changes some of the props/styles based on screen width, I've just split it for clarity): // For screen width <= 320
const {width} = Dimensions.get('window');
return (
<ScrollView horizontal pagingEnabled snapToInterval={width - 20}>
<View style={{width: 10}} /> // For centring the other pages
<View style={{width: width - 20}}><Text>Page 1</Text></View>
<View style={{width: width - 20}}><Text>Page 2</Text></View>
<View style={{width: width - 20}}><Text>Page 3</Text></View>
<View style={{width: 10}} />
</ScrollView>
); // For screen width > 320
const {width} = Dimensions.get('window');
return (
<ScrollView horizontal pagingEnabled snapToInterval={300}>
<View style={{width: 300}}><Text>Page 1</Text></View>
<View style={{width: 300}}><Text>Page 2</Text></View>
<View style={{width: 300}}><Text>Page 3</Text></View>
</ScrollView>
); |
Interesting! Are you able to get this behavior on iOS as well? When I try your first snippet, I can scroll multiple pages at a time with one swipe on iOS (where the algorithm did not change). When I was working on this, I noticed that pagingEnabled worked differently on iOS vs Android when combined with snapToInterval.
I can get the desired behavior relatively easily on iOS with just some JS code, but it's trickier on Android because apparently Is this roughly what you are looking for? https://imgur.com/KaADzmq If so, maybe I can just fix the overflow issue (#14416 should be possible to fix now that Android has overflow support). Edit: I was able to make overflow: visible work on Android. Submitted a patch for that as well. |
@olegbl hmm if that's the case for iOS I guess Android should match it for now, and this would be more of a feature request. Like I said it may be possible if Yep that gif is pretty much exactly what we're trying to do, nice solution too with the overflow 👍 Thanks :) So it sounds like you've fixed or are fixing most of this already then? Do you have links to any PRs or are they FB internal at the moment? Just so we can track :) Thanks again! |
Thanks, @olegbl! Do you have any time estimates on when the fix will be available in the repository? |
Great, thanks!
They're internal right now. I'll update this thread when they get turned into GitHub commits after being reviewed by the React Native team.
Sorry, no. It depends entirely on how quickly these changes get reviewed. They should only take a few hours after that to get to GitHub. |
Summary: ScrollView always clipping children on Android even when `overflow: visible` was specified. This change just omits the clipping during draw if `overflow: visible` is passed in. The default is not changed (from hidden to visible) in case there is a performance impact. Android now matches iOS in behavior. This helps with issue #21116 and resolves #14416. Reviewed By: shergin Differential Revision: D9952096 fbshipit-source-id: 367afe33ee7ff0fdc5aa1074d239883aa289888a
The rewrite also broke EDIT: seems to be an Android P problem instead #19434 |
@rikur Could you elaborate please?
Perhaps you're running into #19434 ? |
@olegbl thanks for the pointer, I was wrong assuming your changes caused the jerky behaviour. |
Where patches will land? Would be nice to have it in 0.57.2, or how make them land in patch version of RN? |
@chrusart there is a dedicated repo for Changelog & keeping up with the releases: https://github.com/react-native-community/react-native-releases We are trying to get a 0.57.2 out today. |
The scrolling can indeed feel pretty janky on Android when It certainly feels a lot better than the previous implementation (in HorizontalScrollView) though since it at least tries to maintain the velocity of the swipe. Do make sure you are using
Yup, see #21116 (comment)
You'll need to provide a repro. I have not seen this and haven't heard of anyone else encountering it either. |
related to this, please open a new issue for it too 👍 |
Here's a video showing a horizontally scrolling FlatList on Android Pie with RN 57. The scrolling slows down as soon as my finger leaves the screen. Also shown is me unable to scroll all the way to the last item https://www.dropbox.com/s/llu4uzs1v6efkr1/20181008_144241_edited.mp4 If I remove the Here is a video showing that same component on RN 56: https://www.dropbox.com/s/mrzhwp53dbs1syo/20181008_145346_edited.mp4 |
You'd have to provide the code for the video (the repro) and start a new issue for it :) It very much looks like you're not using I think I know what's happening with the last item in your video - I'll see if I can put up a fix. |
@olegbl Sorry, I thought this issue encompassed the problem shown in my video as it relates to the snapToInterval being used. We are in fact using I'll make a new issue if necessary, my bad. |
No worries! The issue in this thread was about pagingEnabled allowing you to scroll more than one page at a time on Android. I did repro ScrollVIew not snapping to end on Android when width != multiple of snapToInterval. Putting up a fix for that :) For the scrolling speed - I don't think there's anything more I can do there. It's better than it was before the change (see #20155 for an example) but it's still not great. Unfortunately I don't think there's much more to be done without rewriting Android's OverScroller.java. |
@olegbl I provided a snack with repro in my second comment about it, the same code also repro not scrolling to last item, just remove padding stuff and it uses decelaration 'fast', i guess this is the same as @ds8k would provide for his video. Quick workaround would be to use smoothScroll... instead fling... when interval or offsets are used, btw. pagingEnabled is actually special case of interval (offsets) when interval == screen_width (i'm aware you know it guys) and it looks like making interval and offsets work should make 'automagically' pagingEnabled work as expected. Will make new issue for UX @kelset as @olegbl will patch not scrolling to last item logic. |
Correct. I spent a long time trying to get scrolling feeling as close to the non-snap version as possible and don't have any ideas for how to make it better.
That doesn't really work. smoothScroll always scrolls over the course of 250ms which means that if you're scrolling any further away than about a screen, it feels way too fast.
That's only a part of it. The other difference is that pagingEnabled only scrolls one page at a time, which snapToInterval allows you to scroll through multiple snap offsets in one gesture. P.S. I put up a fix for the snap-to-end issue for Android. It should make it over to GitHub within a couple of weeks. |
But this is how it looks like on iOS and it seems it will anyway scroll few items if scrolled fast with interval set. few smaller items feels very ok'ish in 250ms and here I disagree that it doesn't work.
Well, yes, then it means that pagingEnabled shouldn't be overridden in js and if:
otherwise pagingEnabled looses it's meaning if overridden in js w/ interval/offset. Nevertheless fling... and smoothScroll... are actually ending at the same item (i guess), only that fling do it in constant speed, smoothScroll.. takes under consideration initial velocity and do it in 250ms, for few small items it was really good experience. |
Can confirm this is fixed in 0.57.2 |
@alexandrius Original issue is fixed if this is what you referring to. Probably we should move to new issue: |
Weeks? Well, not scrolling to last item is a major issue here, will try fix it sooner than that. |
No, it's not. iOS uses an acceleration curve that's much more similar to Android's fling() than smoothScroll(). It's just a much better algorithm and doesn't suffer from velocity desyncs when changing target position. I highly recommend you go in, make the change for Android and compare the feel side by side for both long and short lists - smoothScrollTo feels very different from pagingEnabled={false}.
This was never the behavior on iOS, hence I didn't try to mimic it on Android - though I don't really see any problem with you implementing it.
Sort of. Before the changes, the place where the view ended up was completely wrong (it just used velocity as a flat offset in pixels). With the changes, you could easily swap smoothScrollTo() in for fling() and indeed have them end up at the same place.
Yes, and a terrible one for many items. Perhaps there's a threshold somewhere in there where one could switch between the two, not sure.
It's just waiting for a review from the RN team. It might happen way faster but I've had to wait weeks in the past, hence the estimate. You can try doing a PR (the fix it trivial - just |
Well, maybe I should test more on long lists then (more items on screen width) it just feels bad for short ones. @ds8k video shows how it scrolls on small number of items on screen, but it scrolls many items at once, and it is exactly what I'm talking about. Maybe it doesn't look the same in iOS but it looked more similar before (for short lists at least) pagingEnabled for scrolling item width I never said it's in iOS, it's just would be if we implement it like this. |
@chrusart if you're interested I actually experimented with adding a |
Will check that out @mjmasn ! |
I've put videos in #21643 how it looked in 0.56.1 on iOS and Android and 0.57.2 in Android to compare. |
Summary: The end-of-scrollable-range offset was not clipped before the nearestOffset calculation causing the ScrollView to not snap to the end of the view when the width of the ScrollView was not an exact multiple of snapToInterval. Addresses #21116 (comment) Reviewed By: yungsters Differential Revision: D10248545 fbshipit-source-id: 2bdc94ea0a9d9f063769f2c5da4c33d4872b1db2
Summary: The end-of-scrollable-range offset was not clipped before the nearestOffset calculation causing the ScrollView to not snap to the end of the view when the width of the ScrollView was not an exact multiple of snapToInterval. Addresses #21116 (comment) Reviewed By: yungsters Differential Revision: D10248545 fbshipit-source-id: 2bdc94ea0a9d9f063769f2c5da4c33d4872b1db2
Summary: ScrollView always clipping children on Android even when `overflow: visible` was specified. This change just omits the clipping during draw if `overflow: visible` is passed in. The default is not changed (from hidden to visible) in case there is a performance impact. Android now matches iOS in behavior. This helps with issue facebook#21116 and resolves facebook#14416. Reviewed By: shergin Differential Revision: D9952096 fbshipit-source-id: 367afe33ee7ff0fdc5aa1074d239883aa289888a
Summary: The snapToOffsets changes improved the flinging algorithm for snapToInterval/snapToOffsets but actually broke it for pagingEnabled because it's meant to only scroll one page at a time. First, I just brough back the old algorithm, but noticed that it has a bunch of issues (e.g. facebook#20155). So, I tried to improve the algorithm to make sure it uses the proper target offset prediction using the same physics model that Android uses for it's normal scrolling but still be limited to one page scrolls. This resolves facebook#21116. Reviewed By: shergin Differential Revision: D9945017 fbshipit-source-id: be7d4dfd1140f4c4d32bad93a03812dc80286069
the thread is too long, and i didnt get the point how to solve this, in Android i have to replace FlatList/ ScrollView with ViewPagerAndroid ? since the flatlist in IOS with pagingEnabled is still hard to swipe, while on IOS really easy |
Yeap there's no momentum when |
Summary: ScrollView always clipping children on Android even when `overflow: visible` was specified. This change just omits the clipping during draw if `overflow: visible` is passed in. The default is not changed (from hidden to visible) in case there is a performance impact. Android now matches iOS in behavior. This helps with issue facebook#21116 and resolves facebook#14416. Reviewed By: shergin Differential Revision: D9952096 fbshipit-source-id: 367afe33ee7ff0fdc5aa1074d239883aa289888a
Summary: The snapToOffsets changes improved the flinging algorithm for snapToInterval/snapToOffsets but actually broke it for pagingEnabled because it's meant to only scroll one page at a time. First, I just brough back the old algorithm, but noticed that it has a bunch of issues (e.g. facebook#20155). So, I tried to improve the algorithm to make sure it uses the proper target offset prediction using the same physics model that Android uses for it's normal scrolling but still be limited to one page scrolls. This resolves facebook#21116. Reviewed By: shergin Differential Revision: D9945017 fbshipit-source-id: be7d4dfd1140f4c4d32bad93a03812dc80286069
Summary: The end-of-scrollable-range offset was not clipped before the nearestOffset calculation causing the ScrollView to not snap to the end of the view when the width of the ScrollView was not an exact multiple of snapToInterval. Addresses facebook#21116 (comment) Reviewed By: yungsters Differential Revision: D10248545 fbshipit-source-id: 2bdc94ea0a9d9f063769f2c5da4c33d4872b1db2
Might be related to Horizontal FlatList pagination does not work properly on Android #21096 but that issue isn't very clear on what the problem is, and refers to
FlatList
, notScrollView
.Environment
Run
react-native info
in your terminal and paste its contents here.Description
This is a major regression from 0.56.
The changes to paging and snapping in horizontal scroll views appear to have totally broken the UX in android.
Previously swiping worked pretty much like ViewPagerAndroid and swiped a single page at a time if you swiped at least 50% of the screen width (not sure of exact figure), and the speed of the animation roughly matched the speed of the swipe. It was possible to rapidly swipe through a set of pages with quick swipes.
Now the slightest touch scrolls 1 or more pages (usually 2 for a 'normal' swipe) with a really slow animation and it's not possible to swipe through pages rapidly (swiping quickly just makes the animation slow down even more).
It also looks like you can scroll fast if swiping "back" (i.e. finger moving left to right on the screen) but not in a controlled way.
The new snapToOffsets is also broken.
Reproducible Demo
Let us know how to reproduce the issue. Include a code sample, share a project, or share an app that reproduces the issue using https://snack.expo.io/. Please follow the guidelines for providing a MCVE: https://stackoverflow.com/help/mcve
I created identical repos for 0.56.1 and 0.57.0 with toggles to turn pagingEnabled, snapToInterval, snapToOffsets on/off. Doesn't seem to be any combination that works in 0.57.0.
https://github.com/mjmasn/OldSwipe (RN 0.56.1 behaviour)
https://github.com/mjmasn/NewSwipe (RN 0.57.0 behaviour)
For each repo:
yarn start
andreact-native run-android
.BTW for NewSwipe the app seems to crash in native code when toggling snapToOffsets off 🤔
The text was updated successfully, but these errors were encountered: