Fixes ActivityIndicatorIOS doesn't hide initially

[redmar]

I used the reproducible steps as described in origin bug ticket #7987 as test plan.
This has the same contents as PR #8130 but then against master per @janicduplessis request.

[ghost]

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 up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[ide]

Can you explain the root cause of the bug?

[ghost]

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

[redmar]

Can you explain the root cause of the bug?

No not really 😞 , I've tried to trace diffs that relate to the ActivityIndicator but couldn't find the real origin.
I think @janicduplessis comment and idea how to fix in #7987 could be a pointer but am not deep enough into the core of iOS react native to give any sensible reason why the bug occurred.
Still curious about the root cause.

[janicduplessis]

I know that calling stopRefreshing too early doesn't work, it's probably a but in UIKit, not sure exactly at what point does it start working but calling if after the first layout does the trick. My only concern is that I think it used to work at some point so I wonder what changed to break this.

[janicduplessis]

@redmar Do you know if it is broken since a specific version or RN or did it never actually work?

[redmar]

@janicduplessis ok so this took me some time but I did a manual bisect of all the commits between 0.26 (good) and 0.27.0-rc (bad). I can pinpoint the bug to one specific commit now, even to 3 lines in that commit that enable it:

The commit where the bug is introduced is: fe5c0d2

and the 'offending' lines that enable the visibility of the ActivityIndicator while it should be hidden are:

react-native/React/Modules/RCTUIManager.m

Lines 643 to 645 in fe5c0d2

	if (view.isHidden != isHidden) {
	view.hidden = isHidden;
	}

when i comment those lines out everything is working fine.

[janicduplessis]

Great job finding out the commit that broke this!

@rigdern Any idea on the best way to fix this?

[ide]

@redmar thanks for tracking that down. I think what is happening is that when the ActivityIndicator is stopped (which is its default state), it also internally sets self.hidden = NO unless you tell it not to. But this behavior is not modeled in React nor the shadow views so we end up doing:

actualActivityIndicator.hidden = shadowView.hidden // YES

These are three possible solutions:

1. Extend UIActivityIndicatorView and override setHidden to check hidesWhenStopped and isAnimating. This would make it so that we could never display a stopped ActivityIndicator, which on one hand is not very useful, but on the other, it is kind of lame to take away that flexibility unless we have to.
2. Make shadowView.isHidden (

   react-native/React/Modules/RCTUIManager.m

   Line 571 in fe5c0d2

   [areHidden addObject:@(shadowView.isHidden)];

   ) a tri-state NSValue boolean: YES, NO, nil. Set view.hidden = isHidden only when isHidden != nil. This does add complexity (boo) and maybe perf overhead (boo, but need to profile).
3. Break the implicit hidesWhenStopped behavior and make it so that if you want to hide the ActivityIndicator, you need to explicitly write <ActivityIndicator hidden /> in React.

Maybe there are more ideas but those are the ones I have.

[redmar]

@ide I think I got your first point working but without the catch of not being able to never display a stopped ActivityIndicator. See added commit.

Tested this on the test case and it seems to work 😄

edit: whoopsie a bit too fast, forgot to add the old animating custom view property code. Now back.

[janicduplessis]

@redmar What if hidesWhenStopped is set to false, does it still work?

[redmar]

@janicduplessis

hidesWhenStopped={false} animating={false} => activity indicator initially shown but not animating
hidesWhenStopped={false} animating={true}  => activity indicator initially shown and animating
hidesWhenStopped={true} animating={true}  => activity indicator initially shown and animating
hidesWhenStopped={true} animating={false}  => activity indicator initially not shown (bug case, now working as expected)

AFAIK it all works as expected in these cases.

Current patched behaviour is now that when the 'offending' code (seen here

react-native/React/Modules/RCTUIManager.m

Lines 643 to 645 in fe5c0d2

	if (view.isHidden != isHidden) {
	view.hidden = isHidden;
	}

) wants to setHidden=NO on the activityindicator view because the shadowview didn't get the memo to be hidden (as @ide described) it checks again if it should be hidden or not. It has logic to watch for when hidesWhenStopped=TRUE and animating=FALSE it should be hidden no matter what. That's why it's now hidden in the bug case (expected behaviour).

All other interaction with the control is delegated to super so should still work as intended.

[janicduplessis]

@redmar @ide What about we set the hidden value in ActivityIndicator.js?

Something like

hidden={this.props.hidesWhenStopped && !this.props.animating}

https://github.com/facebook/react-native/blob/master/Libraries/Components/ActivityIndicator/ActivityIndicator.js#L84

[redmar]

@janicduplessis have you tried this already? For me this doesn't seem to work, but maybe i'm doing something wrong. (used master, reproduced existing bug, and then applied your suggestion but i don't see any changes while i can confirm the code was really changed).

a possible reason for wanting to use the current patch (which now is only a couple of lines) is that it's an iOS bug only AFAIK so the fix also belongs to the iOS layer/space. But I'm pretty new to this repo so I'm not up to date yet with any policies about where to apply fixes or priority between JS or obj-c space. 😃

[janicduplessis]

Let's move this to it's own file for consistency with other views / view managers.

[janicduplessis]

Ok let's do it this way. It doesn't fix the root issue of showing a view that is hidden initialy but I think this is so specific that activityindicator is the only control that this can cause an issue so fixing it here is fine.

[redmar]

@janicduplessis committed changes based on your comments, hopefully i've done it all in the correct way. (changing and seeing the diff from those .pbxproj files always feels weird)

[janicduplessis]

Thanks @redmar! Sorry for the delay, looks good, lets ship this.

@facebook-github-bot shipit

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[JohnDotOwl]

How do I get this fix?
i'm on
"dependencies": {
"react": "15.1.0",
"react-native": "^0.28.0"
}

[redmar]

It's not yet in v0.29.0-rc.0 AFAIK so you have to wait on the next rc but you can use it now by using master temporarily in your package.json with the following:

"dependencies": {
  "react": "~15.2.0-rc.1",
  "react-native": "https://github.com/facebook/react-native.git"
}

Don't forget to run an react-native upgrade after doing npm install after changing your package.json 😄

[janicduplessis]

It should be in the final 0.29 release but it is not in the current 0.29rc.