fix RefreshControl not showing up #14259

vonovak · 2017-05-30T18:59:39Z

Motivation

There is a bug in the ios implementation of RefreshControl, described in #14056 and #13554. Essentially, it is not possible to programmatically show the refreshControl, because its self.frame.size.height starts with a default value of 60, but as the refreshControl hides, the value approaches zero (0.5). The next time we want to show the control, it will not be visible because the scrollView will only move by 0.5 which is not enough for the refreshControl to show up.

Test plan

I have built a small demo app to debug the issue. I have tested that the refreshControl works in different situations (start with scrolling, start not scrolling, trigger scroll by pull down gesture repeatedly and also programatically via the prop.)

I have noticed that at the time when endRefreshing is called, the self.frame dimensions are correct. The next time beginRefreshing is called the height is completely off though - it is somehow modified in the meantime. That's why I have added another static variable which preserves the value. Let me know what you think of this approach.
I have also modified the way the offset is set and the refreshControl gets displayed and it doesn't seem to affect the behavior on ios 9 and 10 emulators. In fact, it looks even better because the refreshControl is displayed without delay.

janicduplessis · 2017-06-06T07:55:21Z

React/Views/RCTRefreshControl.m

@@ -18,12 +18,17 @@ @implementation RCTRefreshControl {
  UIColor *_titleColor;
 }

+static unsigned short defaultHeight;


Let's just use an instance var instead

I purposely made it static so that the value (which is the same for all instances of refreshControl) is only stored once. Having an instance variable means more memory consumption which in this case is not necessary. Are you sure to change it to per-instance var?

Yes I think it is better even if it will be the same value for each instance. RefreshControl is not a component that has many instances so the memory usage is negligible. Also small detail but maybe initialHeight would be a better name.

janicduplessis · 2017-06-06T07:55:33Z

React/Views/RCTRefreshControl.m

@@ -52,18 +57,9 @@ - (void)beginRefreshing
 {
  // When using begin refreshing we need to adjust the ScrollView content offset manually.
  UIScrollView *scrollView = (UIScrollView *)self.superview;
-  CGPoint offset = {scrollView.contentOffset.x, scrollView.contentOffset.y - self.frame.size.height};
-
-  // `beginRefreshing` must be called after the animation is done. This is why it is impossible


Are you sure this is not needed anymore?

In fact, it looks even better because the refreshControl is displayed without delay.

Do you mean there is no more animation or the animation is just faster / better now? This code was written on iOS 8 or 9 so it's possible it is not needed anymore and I think it's reasonable to remove it.

before, the scrollView would move down with animation and after that the refreshControl would show up. I used the refreshControl to indicate fetching in progress. If the fetching was really fast, you'd only see the scrollView move down and then up, without the refreshControl becoming visible.
Now the scrollView moves down with animation and the refreshControl is shown even before the animation is done, so the refreshControl is visible even when the fetching is really fast.
I have only tested this on emulator. Will test it on a device as well and let you know.

janicduplessis · 2017-06-06T08:31:13Z

React/Views/RCTRefreshControl.m

-                     } completion:^(__unused BOOL finished) {
-                       [super beginRefreshing];
-                     }];
+  CGPoint offset = {scrollView.contentOffset.x, scrollView.contentOffset.y - RCTRefreshControl.defaultHeight};


Could you also add a small comment that explains why we can't use self.frame.size.height here.

when we use self.frame.size.height, it has a issue when we touch and hold to pull at list but just scroll to a part of loading indicator show on screen and then we release it, the height not correct -> beginRefresh will wrong, indicator will not show in nexttime

shergin · 2017-06-06T18:29:09Z

@vonovak To be honest, this whole solution looks a bit... hacky. Could you please describe how it works and why this is correct solution?

rename defualtHeight to _initialHeight, fix facebook#10747

vonovak · 2017-06-06T18:56:10Z

@shergin the problem is roughly this: let's consider a ScrollView that has RefreshControl (RC) in it. You control the RC via the refreshing prop.

1 . Say you pass true to refreshing. The RC will show up. (via the beginRefreshing method in the module). When you put a breakpoint in beginRefreshing, po self.frame.size.height will evaluate to 60.

2 . Then you pass false as the refreshing prop. The RC will hide (endRefreshing method in the module). In endRefreshing, po self.frame.size.height will evaluate to 60.

3 . you pass true to refreshing. The RC will show not up. When you put a breakpoint in beginRefreshing, po self.frame.size.height will evaluate to 0.5.

Of course the question is why, and if there is someone who knows where the root cause is, it would be best to fix it there, but I wasn't able to tell and thus implemented this solution, which is more of a workaround. (maybe it's because UIKit manages the size internally and there isn't anything to fix?)

vonovak · 2017-06-06T18:58:25Z

the PR now fixes also #10747 via the updated endRefreshing method

shergin · 2017-06-06T19:38:23Z

@vonovak Yes, I understand the problem. I just don't believe in the solution.
I am afraid, but we can not merge workarounds when we do not understand what and why is really happening.
So, the main question: Why RC get wrong size. I assume, the parent view, ScrollView, should take care of it.

janicduplessis · 2017-06-06T19:58:35Z

I know we had to add some hacks to RefreshControl on iOS because of some weird UIKit behaviors. When I wrote this UIRefreshControl was more or less supported with UIScrollView and we just added the UIRefreshControl as a child to the UIScrollView and it kind of worked (after a few hacks). With iOS10 there is now an "official" way to use UIRefreshControl with UIScrollView with the refreshControl prop. Maybe using this could fix some of the weird behaviors we have and we could avoid adding more hacks.

vonovak · 2017-06-06T20:19:03Z

that would certainly be nice, but if I understand it correctly, that requires iOS 10 while RN supports >= 8.0... While this solution is a hack, it imho isn't all that horrible. Unfortunately I won't be able to put more time into finding a more elegant solution and I'm not even an iOS developer so I'm not familiar with UIKit. If anyone wants to take this over, feel free to do so.

mikelambert · 2017-06-14T12:01:38Z

Thanks for the workaround @vonovak . Are you using this in your production app? Are there any side-effects to your workaround I should be aware of, if I use this in my app too? In particular, curious why you also changed the call to [super beginRefreshing/endRefreshing], instead of doing just the offset changes? I'm unclear what the effects of that are.

I agree that a proper solution is better for the RN codebase than this hacky workaround. But I also don't have time/experience to fix this properly, and am looking to get a shippable-worthy app again this month after going 0.39->0.45.

vonovak · 2017-06-14T12:15:47Z

@mikelambert to quote on of my comments on the old diff:

(about beingRefreshing): before, the scrollView would move down with animation and after that the refreshControl would show up. I used the refreshControl to indicate fetching in progress. If the fetching was really fast, you'd only see the scrollView move down and then up, without the refreshControl becoming visible.
Now the scrollView moves down with animation and the refreshControl is shown even before the animation is done, so the refreshControl is visible even when the fetching is really fast.

as for endRefreshing, I changed the y offset to 0 which fixes #10747 , the other changes regarding animations in endRefreshing should have no effect imo. I am not using this in production, but would be great if you could give it a try and let us know how it works for you... I don't think there should be a problem with the implementation.

willdawsonme · 2017-08-10T05:54:55Z

I've tried looking into this for a while. After UIRefreshControl's endRefreshing is called, the RefreshControl's height is animated to 0, which is why the frame height ends up being incorrect. I've not been able to find any iOS examples of UIRefreshControl that account for this behaviour though; they all rely on the frame height to offset the content view.

It seems to me like we can't rely on the frame height if it's not going to be static.

facebook-github-bot · 2017-09-05T17:02:28Z

@vonovak I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

vonovak · 2017-09-06T09:49:51Z

while this solution is a workaround, it's not so bad imo. closing this since it likely won't land.

Update RCTRefreshControl.m

772aa30

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 30, 2017

shergin self-requested a review May 31, 2017 07:02

shergin added the Platform: iOS iOS applications. label May 31, 2017

janicduplessis reviewed Jun 6, 2017

View reviewed changes

address review remarks

0e2c037

rename defualtHeight to _initialHeight, fix facebook#10747

vonovak closed this Sep 6, 2017

This was referenced Oct 14, 2017

FlatList data changed but refresh indicator not shows automatically #15892

Closed

RefreshControl offset not functioning correctly for iOS. #14056

Closed

vonovak mentioned this pull request Jan 27, 2018

Flatlist does not show refresh activity programmably after a user refresh #17734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix RefreshControl not showing up #14259

fix RefreshControl not showing up #14259

vonovak commented May 30, 2017

janicduplessis Jun 6, 2017

vonovak Jun 6, 2017

janicduplessis Jun 6, 2017

janicduplessis Jun 6, 2017

janicduplessis Jun 6, 2017

vonovak Jun 6, 2017

janicduplessis Jun 6, 2017

ThangLuuQuoc Nov 14, 2018

shergin commented Jun 6, 2017

vonovak commented Jun 6, 2017 •

edited

Loading

vonovak commented Jun 6, 2017

shergin commented Jun 6, 2017

janicduplessis commented Jun 6, 2017

vonovak commented Jun 6, 2017 •

edited

Loading

mikelambert commented Jun 14, 2017

vonovak commented Jun 14, 2017 •

edited

Loading

willdawsonme commented Aug 10, 2017

facebook-github-bot commented Sep 5, 2017

vonovak commented Sep 6, 2017

fix RefreshControl not showing up #14259

fix RefreshControl not showing up #14259

Conversation

vonovak commented May 30, 2017

Motivation

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shergin commented Jun 6, 2017

vonovak commented Jun 6, 2017 • edited Loading

vonovak commented Jun 6, 2017

shergin commented Jun 6, 2017

janicduplessis commented Jun 6, 2017

vonovak commented Jun 6, 2017 • edited Loading

mikelambert commented Jun 14, 2017

vonovak commented Jun 14, 2017 • edited Loading

willdawsonme commented Aug 10, 2017

facebook-github-bot commented Sep 5, 2017

vonovak commented Sep 6, 2017

vonovak commented Jun 6, 2017 •

edited

Loading

vonovak commented Jun 6, 2017 •

edited

Loading

vonovak commented Jun 14, 2017 •

edited

Loading