Skip to content
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

Fix new arch refresh control not shown when refreshing on mount #49240

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

High5Apps
Copy link

@High5Apps High5Apps commented Feb 6, 2025

Summary:

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 [email protected], 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
  • 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
Copy link
Contributor

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 [email protected]. Thanks!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 6, 2025
@High5Apps
Copy link
Author

Fixes #35779 for new architecture

High5Apps added a commit to High5Apps/organize-rn that referenced this pull request Feb 8, 2025
- Perform changes recommended by the Upgrade Helper
  - Didn't update gradle-wrapper.jar since it is a binary file
  - Didn't manually remove all of the RnDiffAppTests/OrganizeTests references. I only kept the ones that were created by removing OrganizeTests.m and OrganizeTests Info.plist
  - Converted AppDelegate.mm to AppDelegate.swift
    - https://github.com/guardianproject/orbot-apple/blob/57dae6f/Orbot/AppDelegate.swift#L29
    - https://github.com/haqq-network/haqq-wallet/blob/c6687e3/ios/AppDelegate.swift#L67-L73
  - https://react-native-community.github.io/upgrade-helper/?from=0.75.3&to=0.77.0&package=app.getorganize.organize&name=Organize
- Update local dev machine node from v23.3.0 to v23.6.1 and npm from 10.9.0 to 10.9.2 to fix a warning that happened when running `npm run` commands
  - npm/cli#7857 (comment)
- Use align-deps to update and align dependencies
- Fix "2 vulnerabilities (1 moderate, 1 high)" with `npm audit fix`
- Update react-native-vision-camera from 4.5.3 to 4.6.3 to fix a build error
  - mrousavy/react-native-vision-camera#3263
- Fixed another react-native-vision-camera Android build error
  - Opened PR: mrousavy/react-native-vision-camera#3394
  - Added devDependency on [email protected] to bring this fix in locally until the PR is merged
    - https://github.com/ds300/patch-package
- Upgraded react-native-modal-datetime-picker from 17.1.0 to 18.0.0 to fix a default props warning
  - mmazzarolo/react-native-modal-datetime-picker#755
  - https://github.com/mmazzarolo/react-native-modal-datetime-picker/releases/tag/v18.0.0
- Fix Android shadows looked bad by migrating from elevation to boxShadow, which was newly added in 77
  - https://reactnative.dev/docs/view-style-props#boxshadow
  - https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow
- Update react-native-pager-view from 6.4.1 to 6.7.0 to fix an android crash when navigating to FlaggedContent, then hitting the back button
  - callstack/react-native-pager-view#944
  - https://github.com/callstack/react-native-pager-view/releases/tag/v6.6.1
- Fix CountdownClockBorder flicker on touch down by migrating from react native's built-in Animated to react-native-reanimated
  - This was caused by react native 77 using the new architecture by default
  - Add dependency on [email protected]
    - https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/getting-started/
    - Previously removed in 7b0b38a
- Fix iOS RefreshControl not shown on mount
  - This was caused by react native 77 using the new architecture by default
  - Created PR on react-native: facebook/react-native#49240
  - Used patch-package to bring this in locally until the PR is merged
- Fix failing jest tests by removing jestSetupMockReactNavigation.ts
  - According to the docs, the mock is only needed when using DrawerNavigator or (non-native) StackNavigator
    - https://reactnavigation.org/docs/testing/#mocking-native-modules
- Fix ListEmptyComponent briefly shown even though first page was non-empty
  - This was caused by react native 77 using the new architecture by default
  - This affected useModels, usePrependedModels, and useLeadItems because they used useEffect when they should have used useMemo, causing a render delay between when ready was true and when models were non-empty
    - As a result of the change from useEffect to useMemo in Models, I had to remove the isEqual check. This caused useModel consumers to have more renders than previously, since previously the isEqual check debounced many re-renders, e.g. when fetching data from the backend updated the cache, but didn't affect any of the ids watched by the specific instance of useModels
      - I updated useModelCache, which aleviated some of these unnecessary re-renders, but the scenario mentioned above still causes re-renders
      - The only place these new renders caused real issues was with OrgGraph. The useModelCache change fixed a re-render on pull-to-refresh of the OrgGraph. However, I specifically needed to debounce officers in VisGraphData or else selecting a node would trigger an OrgGraph re-render
- Update react-native-screens from 4.5.0 to 4.6.0 to fix an issue where HeaderButton onPress was ignored on Android devices
  - react-navigation/react-navigation#12274
  - software-mansion/react-native-screens#2219 (comment)
- Add an override to rnx-kit config, since it expected react-native-screens to be 4.5.0 instead of 4.6.0 for react native 77
  - https://microsoft.github.io/rnx-kit/docs/tools/align-deps#presets
  - https://microsoft.github.io/rnx-kit/docs/architecture/dependency-management#extensions
  - https://github.com/microsoft/rnx-kit/tree/main/packages/align-deps#configure
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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants