iOS: Fix refreshControl layouting #28236

yogevbd · 2020-03-05T14:36:46Z

Summary

In react-native-navigation we allow the usage of native iOS navigationBar largeTitle which cause the title to "jump" when pulling to refresh.
We found that the layout calculations of the refreshControl element mess up the system behaviour.

Changelog

[iOS] [Fixed] - Fix refreshControl messes up navigationBar largeTitles

Test Plan

Before the fix:

And after:

How it looks like with react-native init app after the fix:

facebook-github-bot · 2020-03-05T14:36:56Z

Hi @yogevbd!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, 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.

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

facebook-github-bot · 2020-03-05T14:54:58Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

analysis-bot · 2020-03-05T14:57:10Z

RNTester.app (iOS): 10715136 bytes

maxencehenneron · 2020-03-13T08:04:33Z

~~Thank you for the PR. However, it seems that the refreshControl property of UIScrollView is only available on iOS 10+.~~

~~see: https://developer.apple.com/documentation/uikit/uiscrollview/2127691-refreshcontrol~~

~~can you wrap the assignation in an @available condition? Also, maybe keep the current code in older versions for retro compatibility?~~

Edit: nevermind, react-native 0.62 targets iOS 10.0. I didn't know about this change, looks good!

My app uses large title (without react-native-navigation) and suffers the same issue.

shergin · 2020-03-22T17:00:46Z

I think we dropped support for iOS 9 in January, so that should be possible to land. I will be oncall next week, so I will try to land it today.

facebook-github-bot

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

shergin

The compilation fails with:

stderr: xplat/js/react-native-github/React/Views/ScrollView/RCTScrollView.m:233:23: error: incompatible pointer types assigning to 'UIRefreshControl * _Nullable' from 'UIView *' [-Werror,-Wincompatible-pointer-types]
self.refreshControl = refreshControl;
^ ~~~~~~~~~~~~~~

yogevbd · 2020-03-24T10:22:03Z

@shergin The build should pass now. Can you please review? Thanks!

…reshControl

analysis-bot · 2020-04-01T13:01:50Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 41fb336

analysis-bot · 2020-04-01T14:21:31Z

Platform	Engine	Arch	Size (bytes)
android	hermes	arm64-v8a	6,765,381
android	hermes	armeabi-v7a	6,433,045
android	hermes	x86	7,153,229
android	hermes	x86_64	7,043,118
android	jsc	arm64-v8a	8,936,764
android	jsc	armeabi-v7a	8,596,806
android	jsc	x86	8,767,718
android	jsc	x86_64	9,343,239

Base commit: 41fb336

guyca · 2020-04-02T10:05:05Z

Only the windows tests suite fails. I'm assuming it's flaky since this PR was rebased yesterday and other builds are passing. Could someone please trigger the failing tests suite? 🙏 @shergin

…reshControl

guyca · 2020-04-20T14:30:59Z

@shergin Tests are green! 💚

facebook-github-bot

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

guyca · 2020-07-26T11:27:01Z

LGTM! 🔥

yogevbd · 2020-07-26T16:04:08Z

@shergin @PeteTheHeat Can you guys please try to merge it again?

simonmitchell · 2020-07-27T09:33:24Z

Whoop whoop! Great work guys, excited to see this merged as it's now patched in two of our projects 😝

facebook-github-bot

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

Evan-Younan · 2020-07-31T08:33:11Z

Thanks a lot for this fix! Looking forward to seeing this merged soon.

chrise86 · 2020-09-23T09:37:46Z

Has this made it into a release yet?

chrise86 · 2020-09-24T09:21:00Z

@simonmitchell @yogevbd tried this out using a patch, and the refresh indicator works great now. I have noticed a couple odd jumps on load sometimes, where the title appears small to start and the content appears under the navbar, but generally much better. However it looks like this has broken stickyHeaderIndices, or at least the content inset calculation for it. Content now hides under the navbar when scrolled.

guyca · 2020-09-25T05:40:43Z

@chrise86
Since it's a bit difficult to reproduce edge cases reported in issues - would you be able to push a minimal reproducible example?

julian-baumann · 2020-10-18T22:25:16Z

What is the current status on this?

Btw @yogevbd how did you manage to apply a transparent background to a large title only with react native navigation? And also remove the border.

yogevbd · 2020-10-19T20:18:23Z

@julian-baumann These options should do with react-native-navigation:

{
  topBar: {
    background: {
      color: 'transparent',
      noBorder: true
    }
  }
}

brmk · 2020-11-26T14:51:32Z

What is the plan for releasing this? It looks abandoned, doesn't it?

habovh · 2020-12-15T14:54:24Z

Well according to this commit details 1b0fb9b it looks like this fix will be part of React Native 0.64 (currently in RC1).

Summary: Fixes #30912 Reverts #31024 which did not fix the issue This fix was removed in #28236, however it caused bug #7976 to resurface, as reported in #30912 Pull Request resolved: #30978 Test Plan: This code had been present for quite some time before being removed in #28236 ## Changelog [Internal] [fixed] - regression with refresh control Reviewed By: p-sun Differential Revision: D26981600 Pulled By: PeteTheHeat fbshipit-source-id: 2560e7dba1cfd6ed41d98f2c9ff4cd07a0e5fa24

pouyarezvani · 2021-05-04T17:49:35Z

Was this fix released? because I'm still having this issue with the large header and FlatList RefreshControl.
If not I will need to Patch-Package it?

dandre-hound · 2021-08-02T18:46:42Z

@yogevbd we are discussing in #31461 how this PR introduces an undesirable visual jump when pulling to refresh on RN list components (scrollview, flatlist, and sectionlist). We are able to reproduce the visual jump on fresh react-native projects with and without navigation headers. After reverting this PR, pulling to refresh is smooth as expected. Do you have any pointers as to why this may be happening, especially since we're able to reproduce without navigation headers? We're really surprised more people haven't mentioned #31461 since it's easily reproducible and it's a very obvious UI issue.

Do you think we're safe to rollback this commit locally if we don't use large navigation headers? Or does this PR fix multiple issues with pulling to refresh? If this PR is only intended to fixe this one issue with pull to refresh on large header titles, we may roll this back locally. Thanks.

yogevbd · 2021-08-03T10:27:57Z

@dandre-hound it is safe to rollback this commit if you don't use navigation large headers. Will work on a fix asap.

dandre-hound · 2021-08-04T19:25:20Z

Sounds great @yogevbd -- we are happy to help test. Thank you for offering to look into this!

This reverts commit 1b0fb9b.

tj-mc · 2024-06-19T04:54:44Z

Is anybody working on a fix for the bug introduced by this PR?
We still face this issue, and are anxious to revert a many-years-old patch 🤔

Edit, sorry for the false alarm. It is actually working fine on 0.73 for us.

iOS: Fix refreshControl layouting

1c97fd9

react-native-bot added Bug Platform: iOS iOS applications. labels Mar 5, 2020

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 Mar 5, 2020

react-native-bot added the Component: RefreshControl label Mar 5, 2020

guyca mentioned this pull request Mar 7, 2020

[v2] Large title animation flicks with pull-to-refresh wix/react-native-navigation#5152

Closed

Merge branch 'master' into fixRefreshControl

78dcce8

facebook-github-bot reviewed Mar 22, 2020

View reviewed changes

shergin suggested changes Mar 23, 2020

View reviewed changes

Fix build

57c5963

yogevbd requested review from shergin and facebook-github-bot March 23, 2020 12:01

remstos mentioned this pull request Mar 25, 2020

Refresh Indicator in FlatList doesn't work correctly with largeTitle wix/react-native-navigation#5815

Open

Merge branch 'master' of github.com:facebook/react-native into fixRef…

df16acd

…reshControl

empty commit

7b5c841

yogevbd added 2 commits April 7, 2020 18:00

Merge branch 'master' of github.com:facebook/react-native into fixRef…

1f5f322

…reshControl

Merge branch 'master' of github.com:facebook/react-native into fixRef…

3a8e839

…reshControl

facebook-github-bot reviewed Apr 27, 2020

View reviewed changes

facebook-github-bot closed this in 702ff6d Apr 27, 2020

Merge branch 'master' into fixRefreshControl

52c019b

facebook-github-bot reviewed Jul 28, 2020

View reviewed changes

facebook-github-bot closed this in 1b0fb9b Aug 5, 2020

kamui545 mentioned this pull request Feb 8, 2021

iOS: Regression in refreshControl on RN 0.64 #30912

Closed

swrobel mentioned this pull request Feb 10, 2021

Restore refresh spinner fix #30978

Closed

krismeld mentioned this pull request Jul 24, 2021

Starting from 0.64.0 RefreshControl causes an unwanted visual jump in the list content when the refreshing prop goes from true to false #31461

Open

maxencehenneron mentioned this pull request Aug 4, 2021

Large title: The navbar gets stuck when using the refresh control. software-mansion/react-native-screens#1034

Closed

gaodeng mentioned this pull request Aug 19, 2021

Upcoming v0.65.1 cherrypicks discussion react-native-community/releases#244

Closed

dandre-hound mentioned this pull request Aug 26, 2021

Upcoming v0.65.2 cherrypicks discussion react-native-community/releases#245

Closed

gaodeng mentioned this pull request Oct 3, 2021

Potential v0.66.1 Patch Release react-native-community/releases#254

Closed

gaodeng added a commit to gaodeng/react-native that referenced this pull request Nov 16, 2021

Revert "iOS: Fix refreshControl layouting (facebook#28236)"

ddd0942

This reverts commit 1b0fb9b.

gaodeng mentioned this pull request Nov 16, 2021

Revert "iOS: Fix refreshControl layouting (#28236)" #32607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS: Fix refreshControl layouting #28236

iOS: Fix refreshControl layouting #28236

yogevbd commented Mar 5, 2020 •

edited

Loading

facebook-github-bot commented Mar 5, 2020

facebook-github-bot commented Mar 5, 2020

analysis-bot commented Mar 5, 2020 •

edited

Loading

maxencehenneron commented Mar 13, 2020 •

edited

Loading

shergin commented Mar 22, 2020

facebook-github-bot left a comment

shergin left a comment

yogevbd commented Mar 24, 2020

analysis-bot commented Apr 1, 2020 •

edited

Loading

analysis-bot commented Apr 1, 2020 •

edited

Loading

guyca commented Apr 2, 2020

guyca commented Apr 20, 2020

facebook-github-bot left a comment

guyca commented Jul 26, 2020

yogevbd commented Jul 26, 2020

simonmitchell commented Jul 27, 2020

facebook-github-bot left a comment

Evan-Younan commented Jul 31, 2020

chrise86 commented Sep 23, 2020

chrise86 commented Sep 24, 2020

guyca commented Sep 25, 2020

julian-baumann commented Oct 18, 2020

yogevbd commented Oct 19, 2020

brmk commented Nov 26, 2020

habovh commented Dec 15, 2020

pouyarezvani commented May 4, 2021

dandre-hound commented Aug 2, 2021

yogevbd commented Aug 3, 2021

dandre-hound commented Aug 4, 2021

tj-mc commented Jun 19, 2024 •

edited

Loading

iOS: Fix refreshControl layouting #28236

iOS: Fix refreshControl layouting #28236

Conversation

yogevbd commented Mar 5, 2020 • edited Loading

Summary

Changelog

Test Plan

Before the fix:

And after:

How it looks like with react-native init app after the fix:

facebook-github-bot commented Mar 5, 2020

facebook-github-bot commented Mar 5, 2020

analysis-bot commented Mar 5, 2020 • edited Loading

maxencehenneron commented Mar 13, 2020 • edited Loading

shergin commented Mar 22, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

shergin left a comment

Choose a reason for hiding this comment

yogevbd commented Mar 24, 2020

analysis-bot commented Apr 1, 2020 • edited Loading

analysis-bot commented Apr 1, 2020 • edited Loading

guyca commented Apr 2, 2020

guyca commented Apr 20, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

guyca commented Jul 26, 2020

yogevbd commented Jul 26, 2020

simonmitchell commented Jul 27, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

Evan-Younan commented Jul 31, 2020

chrise86 commented Sep 23, 2020

chrise86 commented Sep 24, 2020

guyca commented Sep 25, 2020

julian-baumann commented Oct 18, 2020

yogevbd commented Oct 19, 2020

brmk commented Nov 26, 2020

habovh commented Dec 15, 2020

pouyarezvani commented May 4, 2021

dandre-hound commented Aug 2, 2021

yogevbd commented Aug 3, 2021

dandre-hound commented Aug 4, 2021

tj-mc commented Jun 19, 2024 • edited Loading

yogevbd commented Mar 5, 2020 •

edited

Loading

analysis-bot commented Mar 5, 2020 •

edited

Loading

maxencehenneron commented Mar 13, 2020 •

edited

Loading

analysis-bot commented Apr 1, 2020 •

edited

Loading

analysis-bot commented Apr 1, 2020 •

edited

Loading

tj-mc commented Jun 19, 2024 •

edited

Loading