Skip to content

Fix ScrollView centerContent losing taps and causing jitter on iOS #47591

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

Closed
wants to merge 5 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 13, 2024

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. 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:

[IOS] [FIXED] - Fixed centerContent losing taps and causing jitter

Test Plan:

I'm extracting this from a patch we're applying to Bluesky. 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.

merged.mov

After the fix

Observe the natural iOS behavior.

IMG_8963.MOV

Unfortunately I do not have the capacity to verify this fix in other scenarios outside of our app.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 13, 2024
Not sure if needed, but don't want to regress from facebook#24817
{
if (_scrollView.centerContent &&
!CGSizeEqualToSize(self.contentSize, CGSizeZero) &&
!CGSizeEqualToSize(self.bounds.size, CGSizeZero)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about these defensive checks. Added them to avoid losing the fix from #24817 but I don't have a repro case and I'm not a UIKit expert.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 17, 2024

cc @cipolleschi @NickGerleman @sammy-SC figured one of you might have context on this?

@facebook-github-bot
Copy link
Contributor

@sammy-SC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@javache
Copy link
Member

javache commented Nov 18, 2024

Does this same fix need to be applied to the new architecture? This is Paper-only.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 18, 2024

Ah yes, I guess this is the place?

- (void)setContentOffset:(CGPoint)contentOffset
{
if (_isSetContentOffsetDisabled) {
return;
}
if (_centerContent && !CGSizeEqualToSize(self.contentSize, CGSizeZero)) {
CGSize scrollViewSize = self.bounds.size;
if (self.contentSize.width <= scrollViewSize.width) {
contentOffset.x = -(scrollViewSize.width - self.contentSize.width) / 2.0;
}
if (self.contentSize.height <= scrollViewSize.height) {
contentOffset.y = -(scrollViewSize.height - self.contentSize.height) / 2.0;
}
}
super.contentOffset = CGPointMake(
RCTSanitizeNaNValue(contentOffset.x, @"scrollView.contentOffset.x"),
RCTSanitizeNaNValue(contentOffset.y, @"scrollView.contentOffset.y"));
}

We're not running Fabric yet so I'm not sure I'll be able to port this but I can maybe have a look after we migrate

@sammy-SC
Copy link
Contributor

hey @gaearon,

I was trying to create a repro for the issue this is fixing but without any luck. Is there a minimal repro you have so this can be tested outside of Bluesky app?

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 22, 2024

Yeah I could spend some time on this. Would an Expo repro be OK? The default "reproducer" template suggested in the issue template is incredibly painful.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 22, 2024

Hmm, actually, you should be able to reproduce at least the lack of rubber banding with centerContent without a problem. Is this difficult to reproduce? It never worked.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 22, 2024

Here's a minimal repro.

import {ScrollView, Image, View} from 'react-native'

export default function App() {
  return (
    <View style={{flex: 1}}>
      <ScrollView
        bounces
        pinchGestureEnabled
        centerContent
        maximumZoomScale={10}
        onMomentumScrollBegin={() => {
          console.log('momentum begin')
        }}
        onMomentumScrollEnd={() => {
          console.log('momentum end')
        }}>
        <Image
          source={{
            uri: 'https://i.ytimg.com/vi/S-OgkNgxm3k/sddefault.jpg?v=64db06b8',
          }}
          style={{height: 300}}
        />
      </ScrollView>
    </View>
  )
}

Problem 1: No rubber banding

Steps to reproduce:

  1. Slightly pinch in
  2. Try to pan the image up and down

Behavior before the fix

It does not bounce at all while in a partially zoomed in state (despite my bounces={true}). Also, the shift from a fully zoomed in to a partially zoomed in state presents an abrupt jump.

before.mov

Behavior after the fix

It bounces — no matter how much or little you're zoomed in. The shift from a fully zoomed in to partially zoomed in state gracefully animated with a bounce too.

bounce_after.mov

Problem 2: Erroneous momentum events

Steps to reproduce:

  1. Pinch in and slightly pan (either right after the pinch or together with the pinch)
  2. Repeatedly tap many times faster than once a second

Behavior before the fix

The scroll view keeps emitting onMomentumBegin and onMomentumEnd nonstop for as long as you keep tapping fast enough. To emphasize, there is no actual animation, it just thinks it's animating.

before.mov

Behavior after the fix

There are no spurious momentum events (because there is no actual momentum) on repeated taps.

Verify that if you do pan and release afterwards, it does emit momentum events (but only once)

after.mov

Problem 3: Losing taps

I don't think of this as a distinct problem — probably just a consequence of Problem 2.

I don't understand how to use the React Native built-in gesture system (sorry 😔) so I'm not going to attempt that. However, it's pretty easy to reproduce the issue with React Native Gesture Handler.

import {View, ScrollView, Image} from 'react-native'
import {
  GestureHandlerRootView,
  Gesture,
  GestureDetector,
} from 'react-native-gesture-handler'
export default function App() {
  const tap = Gesture.Tap().onEnd(() => {
    'worklet'
    console.log('tapped!')
  })
  return (
    <GestureHandlerRootView>
      <GestureDetector gesture={tap}>
        <View style={{flex: 1}}>
          <ScrollView
            bounces
            pinchGestureEnabled
            centerContent
            maximumZoomScale={10}
            onMomentumScrollBegin={() => {
              console.log('momentum begin')
            }}
            onMomentumScrollEnd={() => {
              console.log('momentum end')
            }}>
            <Image
              source={{
                uri: 'https://i.ytimg.com/vi/S-OgkNgxm3k/sddefault.jpg?v=64db06b8',
              }}
              style={{height: 300}}
            />
          </ScrollView>
        </View>
      </GestureDetector>
    </GestureHandlerRootView>
  )
}

Steps to reproduce:

  1. Pinch in and slightly pan (either right after the pinch or together with the pinch)
  2. Repeatedly tap many times faster than once a second

Behavior before the fix

The tap gesture doesn't fire (and by the logs, it's clear that the erroneous momentum is interfering).

before.mov

Behavior after the fix

The tap gesture fires as expected — when the scroll view is not busy.

after.mov

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 22, 2024

Pushed a Fabric implementation. Verified it against the repro case above.

@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 25, 2024
@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in fe7e97a.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gaearon in fe7e97a

When will my fix make it into a release? | How to file a pick request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants