Fix new arch refresh control not shown when refreshing on mount

[High5Apps]

Summary:

• In the old architecture on iOS, it was possible to begin refreshing a RefreshControl backed by RCTRefreshControl.m on mount (see old arch rn-tester video).
• In the new architecture on iOS, when attempting to refresh on mount, the RefreshControl backed by RCTPullToRefreshViewComponentView.mm is not shown. Additionally, recycled RefreshControls are missing their titles (see new arch rn-tester video).
• This change fixes the new arch issues mentioned above, so that the new arch behaves the same as the old arch did (see new arch rn-tester video with fix).
• This change also updates the rn-tester RefreshControlExample to include a refresh-on-mount to prevent future regressions

Changelog:

[IOS] [FIXED] - Fix new arch RefreshControl wasn't shown when refreshing on mount
[IOS] [FIXED] - Fix new arch recycled RefreshControl was missing its title
[INTERNAL] [CHANGED] - Update rn-tester RefreshControlExample to refresh on mount

Test Plan:

• I created a video of the baseline old arch behavior by overriding new_arch_enabled to return false
• I created a video of the broken new arch behavior by overriding new_arch_enabled to return true
• I applied the changes in this PR and created a video that confirms that the new arch with fix now behaves the same as the old arch
• I also used patch-package to verify that the changes in this PR work as expected in my production React Native app, running react-native@0.77.0, with the new arch now enabled by default. Note that this app uses react-navigation and react-native-screens, which I expected might complicate the fix. The changes in this PR continue to work as expected, even with the complications of those popular libraries.

Details:

• All of my changes were inspired by the many years of fixes that are included in RCTRefreshControl.m, which haven't yet been included in the new RCTPullToRefreshViewComponentView.mm
  • My _isBeforeInitialLayout is inspired by the old _isInitialRender
  • My layoutSubviews is inspired by the old layoutSubviews
  • My beginRefreshingProgrammatically is inspired by the old beginRefreshingProgrammatically
• I have tried to comment all of the unexpected changes. Some of these comments are taken directly from the old RCTRefreshControl.m
• In general, it seems like UIRefreshControl ignores many style updates, and even calls to beginRefreshing, until it's been added to the view hierarchy. That's why the layoutSubviews hack was needed in the old arch, and is similarly needed in this PR.
  • Here's a Stack Overflow answer that hints at this, by saying that beingRefreshing needed to be called in a ViewController's viewWillAppear lifecycle callback
  • Here's a different Stack Overflow answer that shows that viewWillLayoutSubviews (and by extension, all of the subviews' layoutSubviews) is indeed called after viewWillAppear
• Since _refreshControl effectively ignores updates until the first layoutSubviews call, I needed to buffer all calls to updateProps:oldProps: into my new _initialProps before finally re-calling updateProps:oldProps: with _initialProps against PullToRefreshViewShadowNode::defaultSharedProps in layoutSubviews
• Additionally, I needed to move the if block regarding refreshing from the top of updateProps:oldProps: to the bottom of it, because _refreshControl ignores updates that are made after the call to beginRefreshing

Known issues:

• There is one small difference between the old arch and the new arch with this PR. The old arch programmatically animates the pull down content offset, whereas this PR does not. See [scrollView setContentOffset:offset] in the PR code.
  • I tried animating it with the block below, and this did work in the rn-tester app. However, it caused the fix to not work when used with the extremely popular react-navigation and react-native-screens libraries. In that case, the RefreshControl exhibited the same buggy behavior of not being shown when refreshing on mount.

  [UIView animateWithDuration:0.25 delay:0 options:UIViewAnimationOptionBeginFromCurrentState animations:^{
      [_scrollViewComponentView.scrollView setContentOffset:offset];
  } completion:^(BOOL finished) {
      [_refreshControl beginRefreshing];
  }];

  • I thought it would be better to fix this for a larger number of real-world scenarios without animation than it would be to fix it for just the rn-tester app with animation.

[facebook-github-bot]

Hi @High5Apps!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[High5Apps]

Fixes #35779 for new architecture

[koreanddinghwan]

i've tried this pr on RN 0.76.7, and it worked.

[cipolleschi]

Sorry, for some reason Github didn't attach my review when I wrote it.

This approach does not really follow the React mental model, where the UI should only be a function of the data it receives. In this case we are adding some imperative commands to perform different operations between the first render and subsequent renders.

I wonder if there is a problem somewhere else in this component or in Fabric that should be fixed, instead....

[High5Apps]

Thanks for the review! As suggested, I was able to completely remove _initialProps in favor of _props without affecting the fix. I tested that the revised fix continues to work in rn-tester as well as my app, which contains complications such as react-navigation and react-native-screens.

Regarding:

"This approach does not really follow the React mental model, where the UI should only be a function of the data it receives. In this case we are adding some imperative commands to perform different operations between the first render and subsequent renders.

I wonder if there is a problem somewhere else in this component or in Fabric that should be fixed, instead...."

My best guess is that the underlying issues that cause this file to not really follow the React mental model are issues with the iOS UIRefreshControl itself. Specifically, as mentioned in my Details section above, it seems like the UIRefreshControl just ignores many style changes and calls to beginRefreshing unless very specific conditions are met. None of those conditions are documented in the UIRefreshControl documentation. However, it seems like these same limiting conditions affected the non-Fabric RCTRefreshControl.m too. Because as I mentioned in the Details section of my initial comment above:

• My _isBeforeInitialLayout is inspired by the old _isInitialRender
• My layoutSubviews is inspired by the old layoutSubviews
• My beginRefreshingProgrammatically is inspired by the old beginRefreshingProgrammatically

All of those same departures from the expected React metal model were also needed in the non-Fabric component, indicating to me that the root cause is the underlying buggy behavior of the UIRefreshControl itself.

[facebook-github-bot]

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

[facebook-github-bot]

@cipolleschi merged this pull request in e3d607f.

[react-native-bot]

This pull request was successfully merged by @High5Apps in e3d607f

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