[Lightbox] Patch RCTScrollView to fix centerContent #6298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arcalinea
temporarily deployed
to
scrollview-center - social-app PR #6298
Render
Destroyed
|
gaearon
force-pushed
the
scrollview-center
branch
from
3d78164 to
875accd
Compare
arcalinea
temporarily deployed
to
scrollview-center - social-app PR #6298
Render
Destroyed
gaearon
force-pushed
the
scrollview-center
branch
from
875accd to
2ef697f
Compare
gaearon
requested review from
pfrazee,
estrattonbailey,
mozzius and
haileyok
gaearon
changed the title
gaearon
force-pushed
the
scrollview-center
branch
from
2ef697f to
0677505
Compare
haileyok
approved these changes
Nov 13, 2024
gaearon
force-pushed
the
scrollview-center
branch
from
0677505 to
b2c0fb3
Compare
arcalinea
temporarily deployed
to
scrollview-center - social-app PR #6298
Render
Destroyed
gaearon
commented
Nov 13, 2024
| @@ -9,7 +9,7 @@ index caa5540..c5d4e67 100644 | |||
| - type != nil && [type length] > 0 ? type : @"application/octet-stream", | |||
| + ![type isEqual:[NSNull null]] && [type length] > 0 ? type : @"application/octet-stream", | |||
| [data base64EncodedStringWithOptions:0]]; | |||
|
|
|||
There was a problem hiding this comment.
i just regenerated the patch. don't know what's up with these. i suspect someone might have edited it by hand before and stripped whitespace in the editor
|
thats crazy submission |
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Nov 25, 2024
…47591) Summary: The React Native `<ScrollView>` has a peculiar `centerContent` prop. It solves a common need — keeping the image "centered" while it's not fully zoomed in (but then allowing full panning after it's sufficiently zoomed in). This prop sort of works but it has a few wonky behaviors: - If you start tapping immediately after pinch (and don't stop), the taps will not be recognized until a second after you stop tapping. I suspect this is because the existing `centerContent` implementation hijacks the `contentOffset` setter, but the calling UIKit code _does not know_ it's being hijacked, and so the calling UIKit code _thinks_ it needs to do a momentum animation. This (invisible) momentum animation causes the scroll view to keep eating the tap touches. - While you're zooming in, once you cross the threshold where `contentOffset` hijacking stops adjusting values, there will be a sudden visual jump during the pinch. This is because the "real" `contentOffset` tracks the accumulated translation from the pinch gesture, and once it gets taken into account with no "correction", the new offset snaps into place. - While not sufficiently pinched in, the vertical axis is completely rigid. It does not have the natural rubber banding. The solution to all of these issues is described [here](https://petersteinberger.com/blog/2013/how-to-center-uiscrollview/). Instead of hijacking `contentOffset`, it is more reliable to track zooming, child view, and frame changes, and adjust `contentInsets` instead. This solves all three issues: - UIKit isn't confused by the content offset changing from under it so it doesn't mistrigger a faux momentum animation. - There is no sudden jump because it's the insets that are being adjusted. - Rubber banding just works. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [IOS] [FIXED] - Fixed centerContent losing taps and causing jitter Pull Request resolved: #47591 Test Plan: I'm extracting this from [a patch we're applying to Bluesky](https://github.com/bluesky-social/social-app/blob/2ef697fe3d7dec198544ed6834553f33b95790b3/patches/react-native%2B0.74.1.patch). I'll be honest — I have not tested this in isolation, and it likely requires some testing to get merged in. I do not, unfortuntately, have the capacity to do it myself so this is more of a "throw over the wall" kind of patch. Maybe it will be helpful to somebody else. I've tested these in our real open source app (bluesky-social/social-app#6298). You can reproduce it in any of the lightboxes in the feed or the profile. ### Before the fix Observe the failing tap gestures, sudden jump while pinching, lack of rubber banding. https://github.com/user-attachments/assets/c9883201-c9f0-4782-9b80-8e0a9f77c47c ### After the fix Observe the natural iOS behavior. https://github.com/user-attachments/assets/c025e1df-6963-40ba-9e28-d48bfa5e631d Unfortunately I do not have the capacity to verify this fix in other scenarios outside of our app. Reviewed By: sammy-SC Differential Revision: D66093472 Pulled By: javache fbshipit-source-id: 064f0415b8093ff55cb51bdebab2a46ee97f8fa9
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See facebook/react-native#47591.
Before the fix
Observe the failing tap gestures, sudden jump while pinching, lack of rubber banding.
merged.mov
After the fix
Observe the natural iOS behavior.
IMG_8963.MOV