Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fabric support for maintainVisibleContentPosition on iOS #35319

Closed

Conversation

janicduplessis
Copy link
Contributor

Summary

This adds support for the maintainVisibleContentPosition in iOS fabric. This was previously only implemented in the old renderer. The implementation is very similar to what we currently have.

Changelog

[iOS] [Added] - Add fabric support for maintainVisibleContentPosition on iOS

Test Plan

Test in RN tester example.

Screen.Recording.2022-11-12.at.11.35.24.mov

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Nov 12, 2022
@analysis-bot
Copy link

analysis-bot commented Nov 12, 2022

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

Base commit: 1452a55
Branch: main

@analysis-bot
Copy link

analysis-bot commented Nov 12, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,477,030 +2,682
android hermes armeabi-v7a 7,799,004 +3,009
android hermes x86 8,953,779 +3,542
android hermes x86_64 8,811,339 +3,449
android jsc arm64-v8a 9,659,519 +2,700
android jsc armeabi-v7a 8,395,096 +3,042
android jsc x86 9,723,996 +3,567
android jsc x86_64 10,200,564 +3,481

Base commit: 62fa6d9
Branch: main

@pull-bot
Copy link

PR build artifact for 0f5cca2 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 0f5cca2 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Nov 22, 2022
@janicduplessis
Copy link
Contributor Author

@jacdebug Any updates on this?

@pull-bot
Copy link

pull-bot commented Dec 2, 2022

PR build artifact for 63f4a3d is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

pull-bot commented Dec 2, 2022

PR build artifact for 63f4a3d is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@jacdebug
Copy link
Contributor

jacdebug commented Dec 6, 2022

@jacdebug Any updates on this?

Sorry for delay, there are some errors on our CI, can you have a looks please? @janicduplessis

image

image

@janicduplessis janicduplessis force-pushed the @janic/mvcp-ios-fabric branch 3 times, most recently from 8ad850d to 95b4a85 Compare January 27, 2023 05:23
@janicduplessis
Copy link
Contributor Author

@jacdebug Could you try importing it again? Should be fixed now.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@jacdebug
Copy link
Contributor

jacdebug commented Feb 1, 2023

This still have the error in CI:

xplat/js/react-native-github/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm:274:56: error: no matching function for call to 'toString'
    scrollView.snapToAlignment = RCTNSStringFromString(toString(newScrollViewProps.snapToAlignment));
                                                       ^~~~~~~~
stderr: xplat/js/react-native-github/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm:274:56: error: no matching function for call to 'toString'
    scrollView.snapToAlignment = RCTNSStringFromString(toString(newScrollViewProps.snapToAlignment));
                                                       ^~~~~~~~
stderr: xplat/js/react-native-github/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm:274:56: error: no matching function for call to 'toString'
    scrollView.snapToAlignment = RCTNSStringFromString(toString(newScrollViewProps.snapToAlignment));

@janicduplessis
Copy link
Contributor Author

Ok I figured out how to repro the build error, happens only when REACT_NATIVE_DEBUG is not defined. Should be good now.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@jacdebug jacdebug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixes, can you rebase and address some final comments.

@janicduplessis
Copy link
Contributor Author

@jacdebug done!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 7, 2023
@facebook-github-bot
Copy link
Contributor

@jacdebug merged this pull request in ecf967a.

@janicduplessis janicduplessis deleted the @janic/mvcp-ios-fabric branch February 7, 2023 12:03
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 9d98e2c.

@cortinico
Copy link
Contributor

Hey @janicduplessis, just a small heads up that we had to revert this PR overnight as it was making one of our product instacrash with:

[tid:main][] <level:mustfix> QuickReport generated: _contentView == nil; RCTScrollView may only contain a single subview.

as it was the assert you introduced. Can you resubmit this and adapt the logic?

@janicduplessis
Copy link
Contributor Author

@cortinico Does this product use a custom pull to refresh component? I see that the paper implementation of scrollview has some logic to handle that, but didn’t see it for fabric.

@sammy-SC
Copy link
Contributor

sammy-SC commented Feb 8, 2023

yes @janicduplessis it uses custom pull to refresh component.

@janicduplessis
Copy link
Contributor Author

Ok this is the problem, I think we can use a protocol to identify the pull to refresh component view instead, like we do for paper.

@janicduplessis
Copy link
Contributor Author

I posted a new PR with a way to handle custom pull to refresh components #36095

facebook-github-bot pushed a commit that referenced this pull request Mar 7, 2023
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

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[IOS] [ADDED] - Add fabric support for maintainVisibleContentPosition on iOS

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Pull Request resolved: #36095

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

Take a local build of Wilde open Marketplace.

Reviewed By: jacdebug

Differential Revision: D43128163

Pulled By: cipolleschi

fbshipit-source-id: 6cf8ddff92aeb446072a3d847434e21b9e38af61
facebook-github-bot pushed a commit that referenced this pull request Apr 27, 2023
Summary:
`maintainVisibleContentPosition` is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in the example from this diff. When using a large page size it will always push visible content outside of the list "window" which will cause currently visible views to be unmounted so the implementation of `maintainVisibleContentPosition` can't adjust the content inset since the visible views no longer exist.

The first illustration shows the working case, when the new content doesn't push visible content outside the window. The red box represents the window, all views outside the box are not mounted, which means the native implementation of `maintainVisibleContentPosition`  has no way to know it exists. In that case the first visible view is #2, after new content is added #2 is still inside the window so there's not problem adjusting content offset to maintain position. As you can see Step 1 and 3 result in the same position for all initial views.

The second illustation shows the broken case, when new content is added and pushes the first visible view outside the window. As you can see in step 2 the view #2 is no longer rendered so there's no way to maintain its position.

#### Illustration 1

![image](https://user-images.githubusercontent.com/2677334/163263472-eaf7342a-9b94-4c49-9a34-17bf8ef4ffb9.png)

#### Illustration 2

![image](https://user-images.githubusercontent.com/2677334/163263528-a8172341-137e-417e-a0c7-929d1e4e6791.png)

To fix `maintainVisibleContentPosition` when using `VirtualizedList` we need to make sure the visible items stay rendered when new items are added at the start of the list.

In order to do that we need to do the following:

- Detect new items that will cause content to be adjusted
- Add cells to render mask so that previously visible cells stay rendered
- Ignore certain updates while scroll metrics are invalid

### Detect new items that will cause content to be adjusted

The goal here is to know that scroll position will be updated natively by the `maintainVisibleContentPosition` implementation. The problem is that the native code uses layout heuristics which are not easily available to JS to do so. In order to approximate the native heuristic we can assume that if new items are added at the start of the list, it will cause `maintainVisibleContentPosition` to be triggered. This simplifies JS logic a lot as we don't have to track visible items. In the worst case if for some reason our JS heuristic is wrong, it will cause extra cells to be rendered until the next scroll event, or content position will not be maintained (what happens all the time currently). I think this is a good compromise between complexity and accuracy.

We need to find how many items have been added before the first one. To do that we save the key of the first item in state `firstItemKey`. When data changes we can find the index of `firstItemKey` in the new data and that will be the amount we need to adjust the window state by.

Note that this means that keys need to be stable, and using index won't work.

### Add cells to render mask so that previously visible cells stay rendered

Once we have the adjusted number we can save this in a new state value `maintainVisibleContentPositionAdjustment` and add the adjusted cells to the render mask.

This state is then cleared when we receive updated scroll metrics, once the native implementation is done adding the new items and adjusting the content offset.

This value is also only set when `maintainVisibleContentPosition` is set so this makes sure this maintains the currently behavior when that prop is not set.

### Ignore certain updates while scroll metrics are invalid

While the `maintainVisibleContentPositionAdjustment` state is set we know that the current scroll metrics are invalid since they will be updated in the native `ScrollView` implementation. In that case we want to prevent certain code from running.

One example is `onStartReached` that will be called incorrectly while we are waiting for updated scroll metrics.

## Changelog

[General] [Fixed] - Fix VirtualizedList with maintainVisibleContentPosition

Pull Request resolved: #35993

Test Plan:
Added bidirectional paging to RN tester FlatList example. Note that for this to work RN tester need to be run using old architecture on iOS, to use new architecture it requires #35319

Using debug mode we can see that virtualization is still working properly, and content position is being maintained.

https://user-images.githubusercontent.com/2677334/163294404-e2eeae5b-e079-4dba-8664-ad280c171ae6.mov

Reviewed By: yungsters

Differential Revision: D45294060

Pulled By: NickGerleman

fbshipit-source-id: 8e5228318886aa75da6ae397f74d1801d40295e8
bang9 pushed a commit to bang9/react-native that referenced this pull request May 4, 2023
Summary:
`maintainVisibleContentPosition` is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in the example from this diff. When using a large page size it will always push visible content outside of the list "window" which will cause currently visible views to be unmounted so the implementation of `maintainVisibleContentPosition` can't adjust the content inset since the visible views no longer exist.

The first illustration shows the working case, when the new content doesn't push visible content outside the window. The red box represents the window, all views outside the box are not mounted, which means the native implementation of `maintainVisibleContentPosition`  has no way to know it exists. In that case the first visible view is facebook#2, after new content is added facebook#2 is still inside the window so there's not problem adjusting content offset to maintain position. As you can see Step 1 and 3 result in the same position for all initial views.

The second illustation shows the broken case, when new content is added and pushes the first visible view outside the window. As you can see in step 2 the view facebook#2 is no longer rendered so there's no way to maintain its position.

![image](https://user-images.githubusercontent.com/2677334/163263472-eaf7342a-9b94-4c49-9a34-17bf8ef4ffb9.png)

![image](https://user-images.githubusercontent.com/2677334/163263528-a8172341-137e-417e-a0c7-929d1e4e6791.png)

To fix `maintainVisibleContentPosition` when using `VirtualizedList` we need to make sure the visible items stay rendered when new items are added at the start of the list.

In order to do that we need to do the following:

- Detect new items that will cause content to be adjusted
- Add cells to render mask so that previously visible cells stay rendered
- Ignore certain updates while scroll metrics are invalid

The goal here is to know that scroll position will be updated natively by the `maintainVisibleContentPosition` implementation. The problem is that the native code uses layout heuristics which are not easily available to JS to do so. In order to approximate the native heuristic we can assume that if new items are added at the start of the list, it will cause `maintainVisibleContentPosition` to be triggered. This simplifies JS logic a lot as we don't have to track visible items. In the worst case if for some reason our JS heuristic is wrong, it will cause extra cells to be rendered until the next scroll event, or content position will not be maintained (what happens all the time currently). I think this is a good compromise between complexity and accuracy.

We need to find how many items have been added before the first one. To do that we save the key of the first item in state `firstItemKey`. When data changes we can find the index of `firstItemKey` in the new data and that will be the amount we need to adjust the window state by.

Note that this means that keys need to be stable, and using index won't work.

Once we have the adjusted number we can save this in a new state value `maintainVisibleContentPositionAdjustment` and add the adjusted cells to the render mask.

This state is then cleared when we receive updated scroll metrics, once the native implementation is done adding the new items and adjusting the content offset.

This value is also only set when `maintainVisibleContentPosition` is set so this makes sure this maintains the currently behavior when that prop is not set.

While the `maintainVisibleContentPositionAdjustment` state is set we know that the current scroll metrics are invalid since they will be updated in the native `ScrollView` implementation. In that case we want to prevent certain code from running.

One example is `onStartReached` that will be called incorrectly while we are waiting for updated scroll metrics.

[General] [Fixed] - Fix VirtualizedList with maintainVisibleContentPosition

Pull Request resolved: facebook#35993

Test Plan:
Added bidirectional paging to RN tester FlatList example. Note that for this to work RN tester need to be run using old architecture on iOS, to use new architecture it requires facebook#35319

Using debug mode we can see that virtualization is still working properly, and content position is being maintained.

https://user-images.githubusercontent.com/2677334/163294404-e2eeae5b-e079-4dba-8664-ad280c171ae6.mov

Reviewed By: yungsters

Differential Revision: D45294060

Pulled By: NickGerleman

fbshipit-source-id: 8e5228318886aa75da6ae397f74d1801d40295e8
bang9 pushed a commit to bang9/react-native that referenced this pull request May 4, 2023
Summary:
`maintainVisibleContentPosition` is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in the example from this diff. When using a large page size it will always push visible content outside of the list "window" which will cause currently visible views to be unmounted so the implementation of `maintainVisibleContentPosition` can't adjust the content inset since the visible views no longer exist.

The first illustration shows the working case, when the new content doesn't push visible content outside the window. The red box represents the window, all views outside the box are not mounted, which means the native implementation of `maintainVisibleContentPosition`  has no way to know it exists. In that case the first visible view is facebook#2, after new content is added facebook#2 is still inside the window so there's not problem adjusting content offset to maintain position. As you can see Step 1 and 3 result in the same position for all initial views.

The second illustation shows the broken case, when new content is added and pushes the first visible view outside the window. As you can see in step 2 the view facebook#2 is no longer rendered so there's no way to maintain its position.

#### Illustration 1

![image](https://user-images.githubusercontent.com/2677334/163263472-eaf7342a-9b94-4c49-9a34-17bf8ef4ffb9.png)

#### Illustration 2

![image](https://user-images.githubusercontent.com/2677334/163263528-a8172341-137e-417e-a0c7-929d1e4e6791.png)

To fix `maintainVisibleContentPosition` when using `VirtualizedList` we need to make sure the visible items stay rendered when new items are added at the start of the list.

In order to do that we need to do the following:

- Detect new items that will cause content to be adjusted
- Add cells to render mask so that previously visible cells stay rendered
- Ignore certain updates while scroll metrics are invalid

### Detect new items that will cause content to be adjusted

The goal here is to know that scroll position will be updated natively by the `maintainVisibleContentPosition` implementation. The problem is that the native code uses layout heuristics which are not easily available to JS to do so. In order to approximate the native heuristic we can assume that if new items are added at the start of the list, it will cause `maintainVisibleContentPosition` to be triggered. This simplifies JS logic a lot as we don't have to track visible items. In the worst case if for some reason our JS heuristic is wrong, it will cause extra cells to be rendered until the next scroll event, or content position will not be maintained (what happens all the time currently). I think this is a good compromise between complexity and accuracy.

We need to find how many items have been added before the first one. To do that we save the key of the first item in state `firstItemKey`. When data changes we can find the index of `firstItemKey` in the new data and that will be the amount we need to adjust the window state by.

Note that this means that keys need to be stable, and using index won't work.

### Add cells to render mask so that previously visible cells stay rendered

Once we have the adjusted number we can save this in a new state value `maintainVisibleContentPositionAdjustment` and add the adjusted cells to the render mask.

This state is then cleared when we receive updated scroll metrics, once the native implementation is done adding the new items and adjusting the content offset.

This value is also only set when `maintainVisibleContentPosition` is set so this makes sure this maintains the currently behavior when that prop is not set.

### Ignore certain updates while scroll metrics are invalid

While the `maintainVisibleContentPositionAdjustment` state is set we know that the current scroll metrics are invalid since they will be updated in the native `ScrollView` implementation. In that case we want to prevent certain code from running.

One example is `onStartReached` that will be called incorrectly while we are waiting for updated scroll metrics.

## Changelog

[General] [Fixed] - Fix VirtualizedList with maintainVisibleContentPosition

Pull Request resolved: facebook#35993

Test Plan:
Added bidirectional paging to RN tester FlatList example. Note that for this to work RN tester need to be run using old architecture on iOS, to use new architecture it requires facebook#35319

Using debug mode we can see that virtualization is still working properly, and content position is being maintained.

https://user-images.githubusercontent.com/2677334/163294404-e2eeae5b-e079-4dba-8664-ad280c171ae6.mov

Reviewed By: yungsters

Differential Revision: D45294060

Pulled By: NickGerleman

fbshipit-source-id: 8e5228318886aa75da6ae397f74d1801d40295e8
jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
Summary:
`maintainVisibleContentPosition` is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in the example from this diff. When using a large page size it will always push visible content outside of the list "window" which will cause currently visible views to be unmounted so the implementation of `maintainVisibleContentPosition` can't adjust the content inset since the visible views no longer exist.

The first illustration shows the working case, when the new content doesn't push visible content outside the window. The red box represents the window, all views outside the box are not mounted, which means the native implementation of `maintainVisibleContentPosition`  has no way to know it exists. In that case the first visible view is facebook#2, after new content is added facebook#2 is still inside the window so there's not problem adjusting content offset to maintain position. As you can see Step 1 and 3 result in the same position for all initial views.

The second illustation shows the broken case, when new content is added and pushes the first visible view outside the window. As you can see in step 2 the view facebook#2 is no longer rendered so there's no way to maintain its position.

#### Illustration 1

![image](https://user-images.githubusercontent.com/2677334/163263472-eaf7342a-9b94-4c49-9a34-17bf8ef4ffb9.png)

#### Illustration 2

![image](https://user-images.githubusercontent.com/2677334/163263528-a8172341-137e-417e-a0c7-929d1e4e6791.png)

To fix `maintainVisibleContentPosition` when using `VirtualizedList` we need to make sure the visible items stay rendered when new items are added at the start of the list.

In order to do that we need to do the following:

- Detect new items that will cause content to be adjusted
- Add cells to render mask so that previously visible cells stay rendered
- Ignore certain updates while scroll metrics are invalid

### Detect new items that will cause content to be adjusted

The goal here is to know that scroll position will be updated natively by the `maintainVisibleContentPosition` implementation. The problem is that the native code uses layout heuristics which are not easily available to JS to do so. In order to approximate the native heuristic we can assume that if new items are added at the start of the list, it will cause `maintainVisibleContentPosition` to be triggered. This simplifies JS logic a lot as we don't have to track visible items. In the worst case if for some reason our JS heuristic is wrong, it will cause extra cells to be rendered until the next scroll event, or content position will not be maintained (what happens all the time currently). I think this is a good compromise between complexity and accuracy.

We need to find how many items have been added before the first one. To do that we save the key of the first item in state `firstItemKey`. When data changes we can find the index of `firstItemKey` in the new data and that will be the amount we need to adjust the window state by.

Note that this means that keys need to be stable, and using index won't work.

### Add cells to render mask so that previously visible cells stay rendered

Once we have the adjusted number we can save this in a new state value `maintainVisibleContentPositionAdjustment` and add the adjusted cells to the render mask.

This state is then cleared when we receive updated scroll metrics, once the native implementation is done adding the new items and adjusting the content offset.

This value is also only set when `maintainVisibleContentPosition` is set so this makes sure this maintains the currently behavior when that prop is not set.

### Ignore certain updates while scroll metrics are invalid

While the `maintainVisibleContentPositionAdjustment` state is set we know that the current scroll metrics are invalid since they will be updated in the native `ScrollView` implementation. In that case we want to prevent certain code from running.

One example is `onStartReached` that will be called incorrectly while we are waiting for updated scroll metrics.

## Changelog

[General] [Fixed] - Fix VirtualizedList with maintainVisibleContentPosition

Pull Request resolved: facebook#35993

Test Plan:
Added bidirectional paging to RN tester FlatList example. Note that for this to work RN tester need to be run using old architecture on iOS, to use new architecture it requires facebook#35319

Using debug mode we can see that virtualization is still working properly, and content position is being maintained.

https://user-images.githubusercontent.com/2677334/163294404-e2eeae5b-e079-4dba-8664-ad280c171ae6.mov

Reviewed By: yungsters

Differential Revision: D45294060

Pulled By: NickGerleman

fbshipit-source-id: 8e5228318886aa75da6ae397f74d1801d40295e8
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…k#35319)

Summary:
This adds support for the `maintainVisibleContentPosition` in iOS fabric. This was previously only implemented in the old renderer. The implementation is very similar to what we currently have.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[iOS] [Added] - Add fabric support for maintainVisibleContentPosition on iOS

Pull Request resolved: facebook#35319

Test Plan:
Test in RN tester example.

https://user-images.githubusercontent.com/2677334/201484543-f7944e34-6cb7-48d6-aa28-e2a7ccdfa666.mov

Reviewed By: sammy-SC

Differential Revision: D41273822

Pulled By: jacdebug

fbshipit-source-id: 7900898f28280ff01619a4af609d2a37437c7240
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…k#36095)

Summary:
Reland of facebook#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

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[IOS] [ADDED] - Add fabric support for maintainVisibleContentPosition on iOS

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Pull Request resolved: facebook#36095

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

Take a local build of Wilde open Marketplace.

Reviewed By: jacdebug

Differential Revision: D43128163

Pulled By: cipolleschi

fbshipit-source-id: 6cf8ddff92aeb446072a3d847434e21b9e38af61
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
`maintainVisibleContentPosition` is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in the example from this diff. When using a large page size it will always push visible content outside of the list "window" which will cause currently visible views to be unmounted so the implementation of `maintainVisibleContentPosition` can't adjust the content inset since the visible views no longer exist.

The first illustration shows the working case, when the new content doesn't push visible content outside the window. The red box represents the window, all views outside the box are not mounted, which means the native implementation of `maintainVisibleContentPosition`  has no way to know it exists. In that case the first visible view is facebook#2, after new content is added facebook#2 is still inside the window so there's not problem adjusting content offset to maintain position. As you can see Step 1 and 3 result in the same position for all initial views.

The second illustation shows the broken case, when new content is added and pushes the first visible view outside the window. As you can see in step 2 the view facebook#2 is no longer rendered so there's no way to maintain its position.

#### Illustration 1

![image](https://user-images.githubusercontent.com/2677334/163263472-eaf7342a-9b94-4c49-9a34-17bf8ef4ffb9.png)

#### Illustration 2

![image](https://user-images.githubusercontent.com/2677334/163263528-a8172341-137e-417e-a0c7-929d1e4e6791.png)

To fix `maintainVisibleContentPosition` when using `VirtualizedList` we need to make sure the visible items stay rendered when new items are added at the start of the list.

In order to do that we need to do the following:

- Detect new items that will cause content to be adjusted
- Add cells to render mask so that previously visible cells stay rendered
- Ignore certain updates while scroll metrics are invalid

### Detect new items that will cause content to be adjusted

The goal here is to know that scroll position will be updated natively by the `maintainVisibleContentPosition` implementation. The problem is that the native code uses layout heuristics which are not easily available to JS to do so. In order to approximate the native heuristic we can assume that if new items are added at the start of the list, it will cause `maintainVisibleContentPosition` to be triggered. This simplifies JS logic a lot as we don't have to track visible items. In the worst case if for some reason our JS heuristic is wrong, it will cause extra cells to be rendered until the next scroll event, or content position will not be maintained (what happens all the time currently). I think this is a good compromise between complexity and accuracy.

We need to find how many items have been added before the first one. To do that we save the key of the first item in state `firstItemKey`. When data changes we can find the index of `firstItemKey` in the new data and that will be the amount we need to adjust the window state by.

Note that this means that keys need to be stable, and using index won't work.

### Add cells to render mask so that previously visible cells stay rendered

Once we have the adjusted number we can save this in a new state value `maintainVisibleContentPositionAdjustment` and add the adjusted cells to the render mask.

This state is then cleared when we receive updated scroll metrics, once the native implementation is done adding the new items and adjusting the content offset.

This value is also only set when `maintainVisibleContentPosition` is set so this makes sure this maintains the currently behavior when that prop is not set.

### Ignore certain updates while scroll metrics are invalid

While the `maintainVisibleContentPositionAdjustment` state is set we know that the current scroll metrics are invalid since they will be updated in the native `ScrollView` implementation. In that case we want to prevent certain code from running.

One example is `onStartReached` that will be called incorrectly while we are waiting for updated scroll metrics.

## Changelog

[General] [Fixed] - Fix VirtualizedList with maintainVisibleContentPosition

Pull Request resolved: facebook#35993

Test Plan:
Added bidirectional paging to RN tester FlatList example. Note that for this to work RN tester need to be run using old architecture on iOS, to use new architecture it requires facebook#35319

Using debug mode we can see that virtualization is still working properly, and content position is being maintained.

https://user-images.githubusercontent.com/2677334/163294404-e2eeae5b-e079-4dba-8664-ad280c171ae6.mov

Reviewed By: yungsters

Differential Revision: D45294060

Pulled By: NickGerleman

fbshipit-source-id: 8e5228318886aa75da6ae397f74d1801d40295e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. Platform: iOS iOS applications. Reverted Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants