Restore refresh spinner fix

[swrobel]

Fixes #30912
Reverts #31024 which did not fix the issue

Summary

This fix was removed in #28236, however it caused bug #7976 to resurface, as reported in #30912

Test Plan

This code had been present for quite some time before being removed in #28236

Changelog

[Internal] [fixed] - regression with refresh control

[analysis-bot]

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

Base commit: 81c895f

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,908,425	+2
android	hermes	armeabi-v7a	8,407,440	-4
android	hermes	x86	9,399,038	-8
android	hermes	x86_64	9,342,854	-18
android	jsc	arm64-v8a	10,642,333	+5
android	jsc	armeabi-v7a	10,124,448	-5
android	jsc	x86	10,694,449	-3
android	jsc	x86_64	11,278,525	+1

Base commit: b15f8a3

[swrobel]

@yogevbd maybe you could weigh in since you removed this code?

[yogevbd]

@swrobel Looks good to me

[swrobel]

@grabbou I really hate to be annoying here, but this seems like a pretty critical regression to fix before the 0.64 final release. Would you mind taking a look?

[swrobel]

@PeteTheHeat maybe you're the right person to have merge this, since you merged #28236

[PeteTheHeat]

I'm good merging this, but I'd like to understand why the bug resurfaced. It seems like comment was incorrect, updating to use the iOS10 refreshControl UIScrollView prop did not fix the issue?

[janicduplessis]

Ok I checked the original issue and it seems to fix the same thing as #31024, not sure why I thought at the time the api change for refreshControl in scrollview would fix the issue :S. Can you check that the fix in #31024 resolve the problem and this patch is not needed anymore? I think it is a better fix and is less arcane than setting a random background color.

[swrobel]

@janicduplessis yes, that does seem to fix the issue and seems like a cleaner solution to me! Oddly, it doesn't seem to be in 0.64-rc.4, despite having been merged over a week before that was cut.

[janicduplessis]

It needs to be cherry-picked in the release branch, as it has been cut a while back before this commit. I'll get it done.

[swrobel]

Actually, @janicduplessis unfortunately I noticed that I'm seeing unexpected spinners again with your fix, whereas I wasn't with the old one. It's definitely less frequent than without your patch, but it still happens 😞

[janicduplessis]

Ok, in that case can you reopen this PR, remove my fix from #31024 and update the comment?

[swrobel]

Ok, in that case can you reopen this PR, remove my fix from #31024 and update the comment?

I think I got everything you requested

[janicduplessis]

Can you remove the code from #31024? You might need to rebase this to see it as it was merged recently.

[swrobel]

Can you remove the code from #31024? You might need to rebase this to see it as it was merged recently.

That should do it...

[janicduplessis]

I don't see it in the diff, it should be removing these lines: https://github.com/facebook/react-native/blob/master/React/Views/RefreshControl/RCTRefreshControl.m#L52-L62

[swrobel]

@janicduplessis you want my PR to revert your PR's changes? I guess that wasn't clear to me if that's what you were asking for before...

[janicduplessis]

Yes, sorry for the confusion. My fix isn't needed anymore if we merge this so might as well remove it.

[swrobel]

@janicduplessis done! Not sure if react-native-community/releases#214 needs updating, but it seems @grabbou thinks yours should be cherry-picked and not mine (actually, I suppose both might need to be cherry-picked since my depends on yours being there so I can revert it?)

[janicduplessis]

Yea probably want to cherry pick both. LGTM cc @PeteTheHeat

[facebook-github-bot]

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

[facebook-github-bot]

@PeteTheHeat merged this pull request in 0afba0e.