Add fabric support for maintainVisibleContentPosition on iOS

[janicduplessis]

Summary

Reland of #35319 with a fix for custom pull to refresh components.

Custom pull to refresh component in fabric will need to conform to the RCTCustomPullToRefreshViewProtocol protocol, this way we know that the view is a pull to refresh and not the content view.

Changelog

Test Plan

This will need to be tested internally in the product the crash happened.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,479,401	-20,101
android	hermes	armeabi-v7a	7,801,773	-12,732
android	hermes	x86	8,956,970	-20,611
android	hermes	x86_64	8,813,928	-20,753
android	jsc	arm64-v8a	9,116,023	-14,455
android	jsc	armeabi-v7a	8,313,096	-7,079
android	jsc	x86	9,168,448	-14,990
android	jsc	x86_64	9,427,211	-15,092

Base commit: caf80d4
Branch: main

[facebook-github-bot]

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

[janicduplessis]

@cortinico Any updates on this?

[cortinico]

@cortinico Any updates on this?

I've passed this over to @jacdebug for merging. Let me check where it got stuck

[jacdebug]

@cortinico Any updates on this?

I've passed this over to @jacdebug for merging. Let me check where it got stuck

@janicduplessis I am getting it tested with internal app possibly land early next week.

[jacdebug]

@cortinico Any updates on this?

I've passed this over to @jacdebug for merging. Let me check where it got stuck

@janicduplessis I am getting it tested with internal app possibly land early next week.

While testing found that app (wilde) is still crashing in local build. I need help on this so passed to @cipolleschi to debug it.

[janicduplessis]

Custom pull to refresh components will need to implement the RCTCustomPullToRefreshViewProtocol protocol, if that is the case already then it might be another issue.

[cipolleschi]

Hi @janicduplessis, as @jacdebug was saying, I take ownership of the PR internally yesterday. I haven't had time to look into it specifically, I hope to be able to do it tomorrow!

[cipolleschi]

Hi @janicduplessis, I took a look at the PR to see why it was crashing the internal infra.

I applied the changes highlighted in the review and now it's building and running fine internally.

Have a look at them and let me know if they make sense to you. For context, internally our Spinner View does not conform to the RCTCustomPullToRefreshViewComponent. I understand why you added that protocol and that check, but I'm not completely sure that it's a good approach, as it will require to mark with that protocol every view used for that purpose... which include all the views in the OSS and all the views in the other apps.

I'm curious to hear your thoughts about this.

[janicduplessis]

I’m fine with removing the assert, if that makes landing this easier. It should work properly since pull to refresh view is added before the content view https://github.com/facebook/react-native/blob/main/Libraries/Components/ScrollView/ScrollView.js#L1828, although relying on that seems a little bit more brittle.

I would still recommend implementing the interface for custom pull to refresh for internal components you know of. I think outside facebook custom pull to refresh views might not really be used as it isn’t documented anywhere.

[cipolleschi]

I think outside facebook custom pull to refresh views might not really be used as it isn’t documented anywhere.

Exactly because of that, wouldn't be better if we find a way that work for both Meta and the outside world?

I'm not super knowledgeable of how this component works, so I may be missing some details, but if Meta is not using the protocol and the rest of the world is not using the protocol as well, why do we actually need to introduce it?

[janicduplessis]

I think the main reason is that fabric scrollview previously didn’t need any knowledge of it’s content view so the protocol wasn’t needed. I would assume that if facebook still has custom pull to refresh component for the old architecture it would implement the RCTCustomRefreshContolProtocol protocol, which is the equivalent of the protocol I added but for old arch.

[cipolleschi]

Thanks for the pointer.. At this point, I have two questions:

1. Why can't we reuse RCTCustomRefreshContolProtocol? I see that this protocol requires some methods, while the RCTCustomPullToRefreshViewComponent doesn't, so we could have RCTCustomRefreshContolProtocol (the old one) that conforms to RCTCustomPullToRefreshViewComponent and everything should work, WDYT?
2. If we keep the two separated, shouldn't we check for both protocols in RCTScrollViewComponentView?

[janicduplessis]

I did consider reusing it, but I didn’t see why it should have any methods as we don’t use them. For example the current fabric pull to refresh component view doesn’t conform to the old RCTCustomRefreshContolProtocol. I think fabric has the concept of ComponentViews, which wraps the actual view so that was another reason for using a different protocol, as the component view might not be the actual refresh control view.

I don’t think we need to check both as it is probably not possible to end up with a old arch view there. Even with the interop view if I remember correctly it is wrapped in a componentview still. Anyway if that case is possible without the assert it would work still.

[facebook-github-bot]

@cipolleschi merged this pull request in 59c4db8.