Closed roryabraham closed 1 year ago
Hmmm, I guess that makes sense (though I'm not sure what use-cases there might be to pass in the customRangeExtractor
via props). But it seems in line with the way other features have been implemented in the VirtualizedList
component, so that sounds good to me.
I think we need to agree on the defaultRangeExtractor
and how it will be implemented in VirtualizedList
though, as that's a substantial piece of the solution.
I've done a round of investigation on this issue and here are my findings. There are actually 2 separate issues:
autoscrollToTopThreshold
calculation is wrong. This is pretty easy to fix and can be reproduced in the simple scrollview example. The problem is that the y value is set before adjusting content offset so it doesn't need to be adjusted with the delta. Here's the diff with the fix: https://github.com/facebook/react-native/compare/main...janicduplessis:patch-14.
maintainVisibleContentPosition
is broken when using virtualization and the new content pushes visible content outside its "window". This can be reproduced in this diff https://github.com/Expensify/react-native/pull/7/files#diff-e30adf9ac66888db4fe1a9c256590a37677724935458dae6b7a61b99331e292eR46-R48. When using a large PAGE_SIZE (which will cause a lot of items to be added) 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.
The third illustration shows the potential fix, we need to avoid adjusting the window when maintainVisibleContentPosition
this way the first visible view is not unmounted and content position can be maintained. I think this would be possible to implement by making VirtualizedList
aware of maintainVisibleContentPosition
and avoid updating its window when new content is added.
though I'm not sure what use-cases there might be to pass in the customRangeExtractor via props
Probably not too much indeed, as flatlist is quite customizable and allows passing custom props like overscan, windowsize etc.
I believe last function called customRangeExtractor
defined at https://github.com/Expensify/App/issues/7925#issuecomment-1095459409 outlines my idea, if you would like to see a full solution before making a further decision I can work towards that as well.
I think the fix can live in VirtualizedList
directly as it will require knowing about a lot of internal state to apply the proper window adjustments. I don't see any uses for adding it as a prop.
I believe last function called customRangeExtractor defined at https://github.com/Expensify/App/issues/7925#issuecomment-1095459409 outlines my idea, if you would like to see a full solution before making a further decision I can work towards that as well.
Can you please attempt to explain in plain English (without using code) how you will fix this problem directly and generically in VirtualizedList
?
It's difficult to evaluate a proposal based off that code, because it's using hard-coded values. Also it just doesn't really make sense to me... the for loop doesn't do anything unless i === itemCount
, so why not just reference that directly?
const customRangeExtractor = ({ itemCount, first, last }) => {
- const itemsPerPage = 1000
- for (let i = itemsPerPage + itemsPerPage; i <= itemCount; i = i + itemsPerPage) {
- if (i !== itemCount) continue;
- const nextFirst = i - itemsPerPage
- const nextLast = i - itemsPerPage + 150
- return { first: nextFirst, last: nextLast }
- }
- return { first, last }
+ const itemsPerPage = 1000;
+ return itemCount % itemsPerPage === 0
+ ? {first: itemCount - itemsPerPage, last: itemCount - itemsPerPage + 150}
+ : {first, last};
}
static getDerivedStateFromProps(newProps: Props, prevState: State): State {
return customRangeExtractor({ itemCount: newProps.data.length, ...prevState })
}
How will you determine itemsPerPage
, and where did 150
come from? When would itemCount % itemsPerPage
not be 0
?
Here's my proposal for the issue described here.
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:
{first,last}
state by the number of new items added at the start of the listWe need to find how many items have been added before the first visible one. To do that we save the key of the first visible 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 just using index won't work.
In practice we could also simply save the first item in the list instead of the first visible item, as tracking visibility is more complex. For the case of bidirectional pagination this will work as items are only added at the start or end of the list.
{first,last}
state the the number of new items added at the start of the listOnce we have the adjusted number we can save this in a new state value maintainVisibleContentPositionAdjustment
. Note that this needs to be in a separate state and cannot update {first,last}
directly as other updates can happen between window adjustment and when react-native is done rendering. This also needs to be done in getDerivedStateFromProps
to avoid invalid intermediary state that will cause position adjustment to fail.
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.
I have a draft implementation that covers most of this proposal. It is on top of https://github.com/Expensify/react-native/pull/7.
https://github.com/Expensify/react-native/pull/8
It currently doesn't properly implement the first point and assumes all items are added at the start of the list. This causes windowing to be broken when scrolling down, until the next scroll event happens.
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
Just tested the changes on my local branch and this works pretty well. This is a modified version of the concept I mentioned above.
FlatList#keyExtractor
first
and last
values by diffmaintainVisibleContentPosition
shift the list and emit onScroll
eventstatic getDerivedStateFromProps(newProps: Props, prevState: State): State {
...
// calculate diff of appended items
let shiftMetrics = prevState.shiftMetrics || {}
if (!shiftMetrics?.firstItem && getItemCount(data)) {
shiftMetrics.firstItem = data[0].key
} else if (shiftMetrics?.firstItem && shiftMetrics?.firstItem !== data[0].key) {
// @TODO: add key extractor
shiftMetrics.diff = data.findIndex(item => item.key === shiftMetrics?.firstItem)
}
// ensure component is not updated by shiftMetrics reset by returning null
return {
first: first + (shiftMetrics.diff || 0),
last: last + (shiftMetrics.diff || 0),
shiftMetrics,
}
_onScroll = (e: Object) => {
// add an extra check to ensure no race-condition happen between actual scroll
// and the side-effect of the content shift emitted by maintainVisibleContentPosition
this.setState(state => ({
...state,
shiftMetrics: {
firstItem: this.props.data[0].key,
}
}))
we could also simply save the first item in the list instead of the first visible item, as tracking visibility is more complex. For the case of bidirectional pagination this will work as items are only added at the start or end of the list
What would be involved in tracking visibility? I have a couple questions that make me think using the first item in the list might not be ideal:
onStartReachedThreshold
, such that the first item on the list is not visible when we call onStartReached
and start loading new content? Would that cause a scroll jump up to what was previously the first item in the list?minIndexForVisible
parameter in the maintainVisibleContentPosition
prop? Would we just use the Nth item in the list instead of the first one?For the case of bidirectional pagination this will work as items are only added at the start or end of the list.
Ideally we wouldn't rely on this assumption. I agree it's unlikely that content will be added in the middle of a list, but is it possible that deletion of content might simulate this case?
Not sure if it's a problem, but it's something we should be aware of. We should explicitly call out any assumptions we make in documentation updates.
keys need to be stable, and just using index won't work
Again, not sure if it will be a problem for us or not, but we should call this out in documentation updates. Also, can we succinctly define what is meant by "stable"?
In our case, the list items are called "report actions", and when they're created we first generate an optimistic clientReportActionID
and clientSequenceNumber
. When the write response comes back from the API, we overwrite the optimistic reportActionID and
sequenceNumber` with the "canonical" one returned from our servers. In that regard, neither value is necessarily "stable".
Once we have the adjusted number we can save this in a new state value maintainVisibleContentPositionAdjustment
How will this state value be used to extend the render window?
This state is then cleared when we receive updated scroll metrics
How will clearing this state shrink the render window back to the expected range?
@azimgd I think your latest proposal is conceptually very similar to @janicduplessis's latest proposal. Both proposals:
first
and last
, extending the range to include not only the prepended items, but also the first item before the new items were prepended.first
and last
in onScroll
.However, there are some notable differences:
maybeCallOnEdgeReached
if the adjustment state is non-null.What would be involved in tracking visibility? I have a couple questions that make me think using the first item in the list might not be ideal:
Visibility tracking should be relatively straightforward as VirtualizedList has context for scroll metrics and all item positions. However I'm not sure it is needed. Basically the goal of this piece of code is to determine if the native implementation of maintainVisibleContentPosition will be triggered, and by how many our items have been moved by new items added before our virtualization window (first state).
Detecting items added at the start of the list is good enough for bidirectional pagination, but of course will break if items are added at other positions.
I don't think this matters as VirtualizedList has knowledge of all items, even those not rendered. I can test to confirm, but I did play with that value a bit and seemed fine.
Right now we don't consider this value, I think its main use is to ignore changes to header views, which VirtualizedList separate from data, so as far as I know it is fine to ignore, at least when using first item to detect changes.
Ideally we wouldn't rely on this assumption. I agree it's unlikely that content will be added in the middle of a list, but is it possible that deletion of content might simulate this case?
Good point, I haven't tested how maintainVisibleContentPosition
behaves when removing items, we just need to make sure the JS implementation does the same thing so if content will be adjusted by the native implementation we also adjust the VirtualizedList window. In practice this probably won't break unless deleting a large amount of items at once that would cause a shift that moves visible content outside the virtualization window.
Right now when using the first item, a delete in the middle would cause no changes to the virtualization window since the first item did not change, which I think might be correct.
Again, not sure if it will be a problem for us or not, but we should call this out in documentation updates. Also, can we succinctly define what is meant by "stable"?
This could be added to documentation for maintainVisibleContentPosition
of VirtualizedList. By stable I mean keys are id based and not index based. This is already the preferred way to handle keys in VirtualizedList, but we could detect it and show a warning if used incorrectly.
So for example when adding new items key should be a unique id for the item like:
Good, id based key:
// Good we can detect where new item was added.
[{key: 'a'}, {key: 'b'}] -> [{key: 'c'}, {key: 'a'}, {key: 'b'}]
Bad, index based key:
// Looks like item was added at the end, even if it was added at the start since all key shifted.
[{key: 1}, {key: 2}] -> [{key: 1}, {key: 2}, {key: 3}]
In our case, the list items are called "report actions", and when they're created we first generate an optimistic clientReportActionID...
I think this would work still, it would be more like an item update / replace. In this case the code to find first item will not detect any change, or detect the first item no longer exist, which both case result in no change to windowing. Should add this in the list of things to test though!
How will this state value be used to extend the render window?
It is added to the first / last state in render (https://github.com/Expensify/react-native/pull/8/files#diff-7481ec8ed5532798b075df637e805a8e439807aa2ce671208c24068c286361e8R977).
How will clearing this state shrink the render window back to the expected range?
This window in not actually bigger, but shifted. Once we have received a scroll event we have the proper scroll position that has been updated by the native implementation of maintainVisibleContentPosition
. This means that the next time we calculate the window bounds (first/last) it will result in the same as when we were shifting it using maintainVisibleContentPositionAdjustment.
Here's an example of how events happen:
state: {first: 5, last: 10}
js scroll position: 100
rendered window: {first: 5, last: 10}
onStartReached, new item added at start
detect 1 new item added, set maintainVisibleContentPositionAdjustment to 1
rendered window: {first: 6, last: 11} // (5 from state + 1 from maintainVisibleContentPositionAdjustment)
js scroll position: 100 // this is currently invalid since the native implementation has updated this value but we haven't received that update in JS yet
prevent updates since we have invalid js scroll position
scroll event, js scrollPosition updated to 110 (whatever adjustement was needed to maintain visible item after adding 1)
set maintainVisibleContentPositionAdjustment to null, recalculate window state based on new scroll position
state: {first: 6, last: 11}
rendered window: {first: 6, last: 11}
You can also see how the virtualization window behaves in the video I posted, the debug view shows the window on the right.
Okay @janicduplessis I'm going to assign this issue to you. The next steps will be:
maintainVisibleContentPosition
is not yet supported)We can work together on all these steps.
๐ฃ @janicduplessis You have been assigned to this job by @roryabraham! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review ๐งโ๐ป Keep in mind: Code of Conduct | Contributing ๐
Updated https://github.com/Expensify/react-native/pull/8 to use the first item key as indicator of items being added, the bidirectional paging example now works properly when scrolling in both directions.
@janicduplessis , for visibility, can you provide an update?
Hey @mallenexpensify!
I'm currently working with @roryabraham on https://github.com/Expensify/react-native/pull/8. Addressing minor review comments.
PR was just merged into the bidirectional-scrolling feature branch!
Putting this on hold while we work. It seems like this particular bug is fixed, but we'll keep this open until the fix is verified in our app.
@roryabraham can you provide an update? Did the bidirectional-scrolling feature branch hit production when we did the big deploy earlier this week?
@roryabraham 's working on, I think we're waiting on something
https://github.com/Expensify/App/issues/7860 is being held on this.
Rory's been out all week, should have an update next week
Made some great progress on Android implementation last week, posted update here
Asking for an update I can post in here for visibility
Still working on some pieces here, making slow progress
Haven't really made progress the last two weeks. Hoping to push this forward in the second half of this week.
Lots of recent updates in the expensify-margelo slack room
Fork was published yesterday, we are trying to upgrade E/App to use it in https://github.com/Expensify/App/pull/9841
This fix has landed along with all the other bidirectional pagination features in main. All that remains is to see them used in action via a comment linking implementation. Moving this to monthly because it might be a few weeks before that's done.
Thanks for the update @roryabraham , couple things...
This fix has landed along with all the other bidirectional pagination features in main. All that remains is to see them used in action via a comment linking implementation.
When should payment be issued? If the fix can be tested, I wouldn't think we'd need to wait "to see them used in action via a comment linking implementation" in order to pay for the work being done (that said... my level of expertise here is low)
โ ๏ธ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
@mallenexpensify as far as I know, we can't easily test the bidirectional pagination features in Expensify/App without building comment linking or some other test/feature using the onStartReached prop.
That said, we'll be meeting with Margelo next week to discuss next steps. https://expensify.slack.com/archives/C035J5C9FAP/p1661412756144339
โ ๏ธ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Hey everyone, I am a developer at Steuerbot and me and our frontend team are struggling with this issue for a long time, because our app needs to hold the position even if new elements were added at the top of the chat list.
For that reason we hoped an official android support for the maintainVisibleContentPosition
property would be developed, but this never happened ๐
So we tried many different other approaches: Clientside-only hacks, patching RN-Core and other things. Finally we came up with a solution, which fixes the issue on our side ๐ What did we do? We are providing our own version of the Android RN ScrollView implementation (overriding functions from the RN class). This way we can correct the scroll position based on information the JS side provides us (this works also during a "fling animation" by restarting the fling with corrected position!).
I sharing the link to our developed RN module with you; maybe this will work for your project too ๐ https://github.com/steuerbot/react-native-bidirectional-flatlist
Just give it a try. You can use it as a FlatList-Replacement. We are looking forward to your feedback!
Thanks! We've actually completed a bidirectional pagination solution ourselves in our React Native fork, available on npm. @janicduplessis implemented maintainVisibleContentPosition
directly in our React Native fork, and we're hoping to open PRs to upstream these features soon.
We're kicking off comment linking, so hopefully we'll have an update on this issue in the coming weeks.
Still on HOLD for comment linking, which is WIP
@roryabraham , I'm OOO for a month, no assigning a BZ cuz our process for this issue is an edge case. Unassign me and add the Bug
label if you need someone to take any BZ actions
Btw, comment linking is on the horizon at this point. Internal design is happening.
Is there a link to comment linking discussions or a doc being worked on?
Job added to Upwork: https://www.upwork.com/jobs/~01fe321bebf9b78f69
Triggered auto assignment to Contributor Plus for review of internal employee PR - @aimane-chnaif (Internal
)
Not sure if C+ is explicitly required at this point, I mainly wanted to clear up that we have a plan that we are working on internally, thus I update the issue with the internal
label appropriately.
Since this is a big change, I can help review and test fixes if needed ๐
@janicduplessis @roryabraham can you provide an update?
Upstream PR is in review: https://github.com/facebook/react-native/pull/35993
If you havenโt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Here is a very minimal reproduction of this bug in the rn-tester sample project, that's not dependent upon any new code in the React Native codebase. Run the rn-tester app, find the
ScrollView
example, and observe the following behavior:If
minIndexForVisible
is0
, then the scroll position will be maintained iff:contentOffset
is at leasty
, wherey
is the height of the new content being prependedSimilarly, if
minIndexForVisible
is1
, then the scroll position will be maintained iff:contentOffset
is at leasty
, wherey
is the height of the new content being prependedAnd so on... If either of those conditions are not met, the scroll position will not be maintained.
Expected Result:
As long as the list has enough content that it is scrollable, the
contentOffset
should be adjusted such that the scroll position is maintained when new items are added to the start or end of the list.Actual Result:
The
contentOffset
is not adjusted, and the scroll position is not maintained.Additional Details
Background
How does the
maintainVisibleContentPosition
prop work?At a high level, when new UI elements are added to the start of the list, React Native will:
contentOffset
of the scroll container by the amount calculated in the previous step, such that:However, this prop does not work consistently โ sometimes in step 3 the difference in position is incorrectly calculated to be zero. Furthermore, we have noticed that this seems only to happen consistently when the content length of newly-prepended list items is long.
Motivation
For a few months now, we have been endeavoring to get a working solution for a bidirectional-scrolling virtualized list in React Native. After working through many potential solutions, we have come very close to a working solution directly in React Native's VirtualizedList through the code in this PR. However, after lots of debugging we determined that the issues we were seeing weren't caused by the JS /
VirtualizedList
at all, but instead by this bug in ScrollView'smaintainVisibleContentPosition
prop, which is implemented in the native layer.The result of this bug is that our implementation of the
onStartReached
prop inVirtualizedList
suffers from the following issue:contentOffset
of0
),onStartReached
is called.onStartReached
prepends new items into the list.maintainVisibleContentPosition
fails to update thecontentOffset
to account for those new list items.contentOffset
is still0
, so the list position jumps to the start of the new content.contentOffset
is0
,onStartReached
is called again, and we get an infinite loop (at least, until there's no more content to load).Android considerations
Another important piece of information is that the
maintainVisibleContentPosition
prop is not yet available on Android (implementation in progress). We have examined the in-progress Android implementation and found that it is very similar to the iOS one, and likely shares the same problem.For the sake of this issue, the scope is focused on
iOS
, but we believe that the solution in one platform will be applicable in the other.Potential cause
According to review comments from a Meta engineer, this bug is likely caused by a race condition between the items being added to the list and content offset being adjusted.
They also suggest implementing a binary search for the first visible item, which seems like it might improve the issue, but (in my opinion) is unlikely to resolve the race condition entirely.
Evidence of potential race condition?
In the FlatList example linked above, if you tweak these parameters as follows:
The problem is mitigated but not completely solved (a few pages load before
maintainVisibleContentPosition
seems to "catch up" and function as expected). This hints that the problem may indeed be a race condition as suggested above.autoScrollToTopThreshold
wonkinessAccording to the React Native documentation:
This suggests that with an
autoScrollToTopThreshold
of0
, then no auto-scrolling should occur if you have a non-zerocontentOffset
before new items are appended to the list. We have observed that this is not the case by:minIndexForVisible
item out of view)Despite having an
autoScrollToTopThreshold
of0
and a non-zerocontentOffset
, theScrollView
auto-scrolls to the top.Interestingly, this particular
ScrollView
example listed in the reproduction steps can be fixed by removing theautoScrollToTopThreshold
parameter entirely. While this might be a hint at how to solve this, it does not seem to be a viable solution for us. Even without anautoScrollToTopThreshold
parameter, the same problem occurs in this FlatList example. It's unclear why removing theautoScrollToTopThreshold
parameter fixes the problem, but setting a value of0
does not. ๐คWorkaround:
While there may be workarounds possible via hacks in the JS code involving
setTimeout
or extra calls toscrollToIndex
/scrollToOffset
, these would not solve the root problem. In order to have a proposal accepted, it must fix the problem in the React Native codebase itself, probably in the native layer.Platform:
Right now, this problem has been confirmed on iOS. It very likely exists on Android as well in this implementation. We are working on confirming the issue there, and will be following the progress of that pull request.
For the scope of this issue, we'll only require a fix in iOS or Android (preferably iOS), submitted as a PR against the our react-native fork: https://github.com/Expensify/react-native. Applying the same fix in the other platform should be comparatively easy and can be treated as a follow-up.
View all open jobs on GitHub
Upwork Automation - Do Not Edit