-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Make bio area scrollable on iOS #2931
Conversation
prevent ghost presses handle refreshes, animations, and clamps handle most cases for cancelling the scroll animation handle animations save point simplify remove unnecessary context readme apply offset on pan find the RCTScrollView send props, add native gesture recognizer get the react tag wrap the profile in context create module
8fe7749
to
4d089f4
Compare
// We can probably uncomment this later, but for now let's just not allow refreshing | ||
// if offset < 0 { | ||
// return offset - (offset * 0.55) | ||
// } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding refreshControl
support to this is pretty janky. I think the move might be to apply a few changes to the actual RCTRefreshControl
instead. There are a few private methods there that we could use to our advantage if we make them public. This is something that I'd rather do in a separate PR though, so I say let's test the scroll behavior in general before applying patches to RN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the consequence in UI if we don't implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not great. i'm going to spend some time to see if i can find a good way in this PR.
not having this will prevent pull to refresh behavior which is not great. the problem is that the underlying RCTRefreshControl does some of its own logic (inside of a private method that we can't access here), so there's an overlap in us calling beginRefreshing here and the underlying method calling it as well.
will give this some thought, because i'd like to ship with refreshControl support since it's a pretty crucial thing to have.
I've gotten a bit closer here to reducing the changes needed on the JS side mainly by understanding the underlying |
The Pull Request introduced fingerprint changes against the base commit: 2bc20b1 Fingerprint diff[{"type":"dir","filePath":"modules/expo-scroll-forwarder/ios","reasons":["expoAutolinkingIos"],"hash":"5622c8a603984fab86f20bddd229d210c1bf7c16"},{"type":"dir","filePath":"patches","reasons":["patchPackage"],"hash":"601b05bfe503376abface0811f958d1290802427"},{"type":"contents","id":"expoAutolinkingConfig:ios","contents":"{\"extraDependencies\":{\"androidMavenRepos\":[],\"iosPods\":{}},\"modules\":[{\"packageName\":\"expo-camera\",\"packageVersion\":\"14.0.6\",\"pods\":[{\"podName\":\"ExpoCamera\",\"podspecDir\":\"node_modules/expo-camera/ios\"}],\"swiftModuleNames\":[\"ExpoCamera\"],\"modules\":[\"CameraViewModule\",\"CameraViewNextModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-clipboard\",\"packageVersion\":\"5.0.1\",\"pods\":[{\"podName\":\"ExpoClipboard\",\"podspecDir\":\"node_modules/expo-clipboard/ios\"}],\"swiftModuleNames\":[\"ExpoClipboard\"],\"modules\":[\"ClipboardModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-dev-client\",\"packageVersion\":\"3.3.11\",\"pods\":[{\"podName\":\"expo-dev-client\",\"podspecDir\":\"node_modules/expo-dev-client/ios\"}],\"swiftModuleNames\":[\"expo_dev_client\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-dev-launcher\",\"packageVersion\":\"3.6.9\",\"pods\":[{\"podName\":\"expo-dev-launcher\",\"podspecDir\":\"node_modules/expo-dev-launcher\"}],\"swiftModuleNames\":[\"EXDevLauncher\"],\"modules\":[\"DevLauncherInternal\",\"DevLauncherAuth\",\"RNCSafeAreaProviderManager\"],\"appDelegateSubscribers\":[\"ExpoDevLauncherAppDelegateSubscriber\"],\"reactDelegateHandlers\":[\"ExpoDevLauncherReactDelegateHandler\"],\"debugOnly\":true},{\"packageName\":\"expo-dev-menu\",\"packageVersion\":\"4.5.8\",\"pods\":[{\"podName\":\"expo-dev-menu\",\"podspecDir\":\"node_modules/expo-dev-menu\"}],\"swiftModuleNames\":[\"EXDevMenu\"],\"modules\":[\"DevMenuModule\",\"DevMenuInternalModule\",\"DevMenuPreferences\",\"RNCSafeAreaProviderManager\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[\"ExpoDevMenuReactDelegateHandler\"],\"debugOnly\":true},{\"packageName\":\"expo-dev-menu-interface\",\"packageVersion\":\"1.7.2\",\"pods\":[{\"podName\":\"expo-dev-menu-interface\",\"podspecDir\":\"node_modules/expo-dev-menu-interface/ios\"}],\"swiftModuleNames\":[\"expo_dev_menu_interface\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-eas-client\",\"packageVersion\":\"0.11.0\",\"pods\":[{\"podName\":\"EASClient\",\"podspecDir\":\"node_modules/expo-eas-client/ios\"}],\"swiftModuleNames\":[\"EASClient\"],\"modules\":[\"EASClientModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-haptics\",\"packageVersion\":\"12.8.1\",\"pods\":[{\"podName\":\"ExpoHaptics\",\"podspecDir\":\"node_modules/expo-haptics/ios\"}],\"swiftModuleNames\":[\"ExpoHaptics\"],\"modules\":[\"HapticsModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-image-loader\",\"packageVersion\":\"4.6.0\",\"pods\":[{\"podName\":\"EXImageLoader\",\"podspecDir\":\"node_modules/expo-image-loader/ios\"}],\"swiftModuleNames\":[\"EXImageLoader\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-image-manipulator\",\"packageVersion\":\"11.8.0\",\"pods\":[{\"podName\":\"ExpoImageManipulator\",\"podspecDir\":\"node_modules/expo-image-manipulator/ios\"}],\"swiftModuleNames\":[\"ExpoImageManipulator\"],\"modules\":[\"ImageManipulatorModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-image-picker\",\"packageVersion\":\"14.7.1\",\"pods\":[{\"podName\":\"ExpoImagePicker\",\"podspecDir\":\"node_modules/expo-image-picker/ios\"}],\"swiftModuleNames\":[\"ExpoImagePicker\"],\"modules\":[\"ImagePickerModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-json-utils\",\"packageVersion\":\"0.12.0\",\"pods\":[{\"podName\":\"EXJSONUtils\",\"podspecDir\":\"node_modules/expo-json-utils/ios\"}],\"swiftModuleNames\":[\"EXJSONUtils\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-linear-gradient\",\"packageVersion\":\"12.7.2\",\"pods\":[{\"podName\":\"ExpoLinearGradient\",\"podspecDir\":\"node_modules/expo-linear-gradient/ios\"}],\"swiftModuleNames\":[\"ExpoLinearGradient\"],\"modules\":[\"LinearGradientModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-manifests\",\"packageVersion\":\"0.13.0\",\"pods\":[{\"podName\":\"EXManifests\",\"podspecDir\":\"node_modules/expo-manifests/ios\"}],\"swiftModuleNames\":[\"EXManifests\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-media-library\",\"packageVersion\":\"15.9.1\",\"pods\":[{\"podName\":\"EXMediaLibrary\",\"podspecDir\":\"node_modules/expo-media-library/ios\"}],\"swiftModuleNames\":[\"EXMediaLibrary\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-notifications\",\"packageVersion\":\"0.27.6\",\"pods\":[{\"podName\":\"EXNotifications\",\"podspecDir\":\"node_modules/expo-notifications/ios\"}],\"swiftModuleNames\":[\"EXNotifications\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-scroll-forwarder\",\"packageVersion\":\"UNVERSIONED\",\"pods\":[{\"podName\":\"ExpoScrollForwarder\",\"podspecDir\":\"modules/expo-scroll-forwarder/ios\"}],\"swiftModuleNames\":[\"ExpoScrollForwarder\"],\"modules\":[\"ExpoScrollForwarderModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-sharing\",\"packageVersion\":\"11.10.0\",\"pods\":[{\"podName\":\"ExpoSharing\",\"podspecDir\":\"node_modules/expo-sharing/ios\"}],\"swiftModuleNames\":[\"ExpoSharing\"],\"modules\":[\"SharingModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-structured-headers\",\"packageVersion\":\"3.7.0\",\"pods\":[{\"podName\":\"EXStructuredHeaders\",\"podspecDir\":\"node_modules/expo-structured-headers/ios\"}],\"swiftModuleNames\":[\"EXStructuredHeaders\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-system-ui\",\"packageVersion\":\"2.9.3\",\"pods\":[{\"podName\":\"ExpoSystemUI\",\"podspecDir\":\"node_modules/expo-system-ui/ios\"}],\"swiftModuleNames\":[\"ExpoSystemUI\"],\"modules\":[\"ExpoSystemUIModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-task-manager\",\"packageVersion\":\"11.7.2\",\"pods\":[{\"podName\":\"EXTaskManager\",\"podspecDir\":\"node_modules/expo-task-manager/ios\"}],\"swiftModuleNames\":[\"EXTaskManager\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-updates-interface\",\"packageVersion\":\"0.15.1\",\"pods\":[{\"podName\":\"EXUpdatesInterface\",\"podspecDir\":\"node_modules/expo-updates-interface/ios\"}],\"swiftModuleNames\":[\"EXUpdatesInterface\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"expo-web-browser\",\"packageVersion\":\"12.8.2\",\"pods\":[{\"podName\":\"ExpoWebBrowser\",\"podspecDir\":\"node_modules/expo-web-browser/ios\"}],\"swiftModuleNames\":[\"ExpoWebBrowser\"],\"modules\":[\"WebBrowserModule\"],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false},{\"packageName\":\"unimodules-app-loader\",\"packageVersion\":\"4.5.0\",\"pods\":[{\"podName\":\"UMAppLoader\",\"podspecDir\":\"node_modules/unimodules-app-loader/ios\"}],\"swiftModuleNames\":[\"UMAppLoader\"],\"modules\":[],\"appDelegateSubscribers\":[],\"reactDelegateHandlers\":[],\"debugOnly\":false}]}","reasons":["expoAutolinkingIos"],"hash":"ac9bbfa866d71fba9d8860789f11fd6134aa51f4"}] Generated by PR labeler 🤖 |
|
@gaearon I feel a lot more comfortable about Do you feel that the |
ced33af
to
271e178
Compare
Ah, missing animation for scrolling back up if the offset is < 0 and > -130 😭 will fix tomorrow. |
allow swipe back gesture clean up types always run animation if the final offset is < 0 separate logic update patch readme get the `RCTRefreshControl` working nicely
cefb21a
to
6e81de7
Compare
Alright, fixed the missing animation as well as fixed interference with the native swipe back gesture. |
5cb1f63
to
542e72f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sick.
i did some testing and encountered a weird choppy state once, but i couldn't figure out how to repro again. it went away after changing tabs there and back, and it's probably no worse than the current behavior. if we see this a lot in prod, we can use the killswitch.
the code looks solid to me.
i tried disabling the effects that set the scroll tag and it seemed to reliably "turn off" the fix for the corresponding tab as i would expect.
* origin/main: (455 commits) Use getSuggestions endpoint behind the gate (#3499) Added `new_profile_scroll_component` to `Gate` type (#3487) Fix useGate lint rule (#3486) Make bio area scrollable on iOS (#2931) Improve Android haptic, offer toggle for haptics in the app (#3482) Search - only enable queries once tab is active (#3471) [Statsig] Mark Testflight as staging tier (#3470) [Statsig] Typecheck gates (#3467) Bump to 1.77 (#3468) Search - extra tabs (#3408) notify slack on production builds (#3461) notify slack on production builds (#3460) 1.76 release preparations (#3459) Update zh-CN translation (#3433) Italian Localization (#3388) [Statsig] Send prev route name (#3456) [Statsig] Instrument feed display (#3455) Small logic cleanups (#3449) Use ALF for the embed consent modal (#3336) Onboarding tweaks (#3447) ...
Why
Our implementation of a header with a tab bar has caused us quite a bit of pain for a while now on iOS. Unfortunately, because of how iOS handles touches, we were unable to properly scroll in the area of a user bio that is pressable - i.e. in the bio text, the followers/following links, or the Edit Profile button.
This PR addresses those issues by adding some gesture detectors to the profile header. Those detectors can then be used to update the underlying
UIScrollView
's content offset and to trigger a refresh of the scroll view's content.In an effort to make this act as closely as possible to a normal
UIScrollView
(i.e. just a<ScrollView>
with a<RefreshControl>
, we needed to make some changes to React Native'sRCTRefreshControl
. Most of the methods and variables on theRCTRefreshControl
are private, so we needed to expose a new method,forwarderBeginRefreshing()
to handle triggering a refresh.I've used the Expo Modules API for this as that will allow us to use this same code with Fabric, rather than have to rewrite or update it whenever we migrate to Fabric.
How
findNodeHandle()
The first and smallest part of the code happens on the JS side. We need to tell our
ExpoScrollForwarderView
whichRCTScrollView
it should be modifying. To do that, we need to get the native tag of the view viafindNodeHandle()
.An initial concern we had with this was having to use
svRef.current.getNativeScrollRef()._nativeTag
. As it turns out, this was a valid concern. A few libraries - including@gorhom/bottom-sheet
- have used this method of obtaining the native view. While this method does work right now with Paper, when migrating to Fabric this method seems to have broken. See https://github.com/discord/react-native-bottom-sheet/pull/11/files.findNodeHandle()
however does work with Fabric.findNodeHandle()
is also used in React Native's ownScrollView
component, forsvRef.current.getScrollableNode()
. https://github.com/facebook/react-native/blob/6014dce04c86fde5c7d4050859fca20007ff1200/packages/react-native/Libraries/Components/ScrollView/ScrollView.js#L845.We use a
useEffect()
to update the native tag on tab changes. From my testing, this has not caused any problems. It's a bit unfortunate that we need to prop drill to get asetState()
down to where we need to call it at, but oh well.RCTRefreshControl
PatchOne additional piece of code that we needed to implement for this to act nicely was a little patch to
RCTRefreshControl
. Most of the methods inRCTRefreshControl
are not exposed, and as a result we couldn't just do this right inside of the newExpoScrollForwarderView
module.The patch adds a new method -
forwarderBeginRefreshing()
toRCTRefreshControl
that mimics the normal behavior of the component. Whenever our scroll view reaches a content offset of -130 (only when we are scrolling from within the header, otherwise we just let theRefreshControl
act normally) we call this new method, mostly mimicing the behavior that we expect.ExpoScrollForwarder
Most of the new logic is inside of this new module. There are a few different pieces here to take note of.
Finding the underlying
RCTScrollView
Whenever we update the
scrollViewTag
prop, we calltryFindScrollView()
. This method will attempt to find anRCTScrollView
with a tag that matches thescrollViewTag
prop. If we do find one, we also add two gesture detectors to the view (see below).cancelGestureRecognizers
Because there is some new code that is responsible for modifying the scroll position of the view - which runs separate of the real
UIScrollView
logic for scroll animations, velocity, etc - we need to ensure that we stop that animation whenever we touch the actualUIScrollView
. For example:UIScrollView
UIScrollView
, we no longer need to run the custom scroll animation to completionThis isn't a super easy task - we need to make sure that our new gesture recognizers do not interfere with the
UIScrollView
'sUIPanGestureRecognizer
. We also need to make sure that a tap within theUIScrollView
does not accidentally trigger a tap on a post or other button. To achieve this, we add aUITapGestureRecognizer
as well as aUILongPressGestureRecognizer
to theUIScrollView
.The former is used only to intercept taps - so that we do not accidentally trigger any navigation events, post likes, etc. whenever we bring the scroll to a halt.
The latter is used to actually cancel the scroll animation. It is a
UILongPressGestureRecognizer
with aminimumPressDuration
of 0.01, which will result in a nearly immediate halt of the previous animation and let theUIScrollView
take over animations on its own. (We can't use aUIPanGestureRecognizer
here because it would interfere with theUIScrollView
'sUIPanGestureRecognizer
)Every time we switch tabs - i.e. switch from "Posts" to "Lists" - we remove the cancel recognizers from the current
UIScrollView
and add them to the new one that we are switching to. This happens inside oftryFindScrollView()
.The main
UIPanGestureRecognizer
The main
UIPanGestureRecognizer
is added to theUIView
- the view that contains our header. This operates in the same way as (or rather is the underlying mechanism behind of) Reanimated'sGesture.Pan()
.We use this recognizer to "simulate" and "forward" the scroll events to the
UIScrollView
. We run a bit of logic to "dampen" the scroll whenever we end up at an offset of < 0, handle refresh logic whenever we reach an offset of -130, and apply a decay "animation" all by using this gesture recognizer's events.Caveats
This operates very closely to the real thing. However, there are a few quirks that I would definitely like to solve, although I believe they would require a more extensive (i.e. a completely custom
UIScrollView
module) to achieve.The decay animation
We would like to be able to use
UIView.animate
(https://developer.apple.com/documentation/uikit/uiview/1622418-animate) to achieve this. Unfortunately, from my testing this interferes with theFlatList
's virtualization. As a result, we instead use a timer - which runs on every frame - to update the scroll position instead. This isn't optimal but it appears to be the best way to achieve what we are looking for. I'll play with this some more in the future to see if a better alternative could come upQuirky animation on the
UIRefreshControl
There's a bit of a strange animation that occurs with the refresh control here. This - from what I can tell - is a result of "hacking" the entire thing to do this programatically. There may be a better alternative here (I suspect that looking more closely at the
RCTScrollView
view might turn up something), but for right now this is a very close replica that I feel comfortable shipping (I was not comfortable shipping the prior iteration I did of this, where the refresh control was extremely janky).Video
RocketSim_Recording_iPhone_15_Pro_6.1_2024-04-06_22.56.11.mp4
Test Plan
We should use this internally for a bit to find any quirks. I am sure there will be some that come up, although I feel confident at this point that a few small iterations should get us extremely close to what we want.
I've tested switching between tabs without issue, including starting each tab at a different scroll position. I've also tested opening multiple windows with profiles.
Additionally, I've done some "attempts" at breaking it. I.e., switch tabs and immediately start scrolling to try and find a delay between the tab switch and the calling of
tryFindScrollView()
. So far I have not been able to turn up anything.And of course, I have done some pretty significant testing of the refresh control, and aside from the above noted quirk, have not seen anything that has caused problems.
Lastly, I've tested the gate by setting the percentage to 100% of internal users and verified the scroll behavior works, then setting it to 0% of internal users and verified that it is disabled.
General Usage
You should have two views: a
ScrollView
(or aFlatList
,VirtualizedList
, etc. that usesRCTScrollView
) and anotherview that you want to detect pan on to apply to the
RCTScrollView
.Next, you need to get the
_nativeTag
of yourScrollView
, like below:Next, wrap the view you're trying to detect gesture on inside of
ScrollForwarder
.