react-native
react-native copied to clipboard
facebook
Fix inverted flat list
Summary:
This PR solves this issue. Inverted FlatList doesn't work (elements cannot be clicked) when the list is scrolled.
Changelog:
[GENERAL] [FIXED] - Fix clicking items on the inverted FlatList on the new architecture
Test Plan:
- Use this reproducer: https://github.com/WoLewicki/reproducer-react-native/tree/%40wolewicki/flatlist-inverted
- Apply changes from this PR & build the app.
- Scroll a bit the list, so it changes the position.
- The
onPressshould be fired when the button is clicked. - Do the following tests:
- Add a
horizontalprop to the FlatList - verify everything works. - Remove a
invertedprop - verify everything works. - Remove a
invertedprop and add ahorizontalprop - verify everything works. - Test different combinations of transforms of the FlatList, example:
<FlatList
inverted
horizontal
style={{
transform: [
{scaleY: -1},
{scaleY: -2},
{scaleY: -0.5},
{translateY: 20},
{translateY: -10},
{skewX: '10deg'},
{rotateX: '10deg'},
],
}}
/>
Reproducrer
https://github.com/facebook/react-native/assets/104823336/28cfe607-43e8-4f80-bbfb-59085ae0f986
RN tester
https://github.com/facebook/react-native/assets/104823336/e00cd488-d98f-4ece-9cab-b8a7212acb04
Hi @kosmydel!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
The code looks good to me. I assume that inverting the list actually apply a transformation that is not applied to the content origin offset, right?
That's right.
I still need to test it before opening it for review, as for now I tested it only in the @WoLewicki reproducer.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 20,364,276 | -142 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 23,569,540 | +1 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: 6a3a3056283a4fad8c527ad3e0fd2d397e040f9e Branch: main
I think this maybe should be adjusted for transformOrigin now that we have that (@intergalacticspacehighway do you know the math we should be doing here?) Otherwise this lgtm
This might end up interacting with https://github.com/facebook/react-native/pull/43192 more broadly I think, since we need to after resolve transforms relative to view size.
This might end up interacting with https://github.com/facebook/react-native/pull/43192 more broadly I think, since we need to after resolve transforms relative to view size.
I think as long as we use resolveTransform and layout metrics are correct, we should be good.
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Hey, today I won't have time to investigate it.
we're doing this transform at the wrong layer I think
Could you provide more details on where the change should be made? This would be helpful.
How did we find this function as the original culprit? I.e. how do we call it for hit testing in the issue case? That would be a clue.
How do we call it for hit testing in the issue case:
We call it when adding offset for elements that are inside the ScrollView: https://github.com/facebook/react-native/blob/c884d19a501081ffbc4b353d751ecc3e32d6f624/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L224
@sammy-SC wrote this code it looks like, but I can see that calculateTransformedFrames() already has some logic for transforming origin based on transform (though, seems special cased instead of using the matrix)? So it seems like there is an assumption there as well, that the returned origin is not yet transformed, but that caller should transform it.
We could maybe pass through policy for whether to include transform to this API, like some others. Because some code calling this (e.g. scroll offset) which rely on it not being transformed, but some other code calling it which already seems to try to apply transforms (and it sounds like one case, where we are forgetting to do this).
I also went to experiment on DOM behavior of scrollTop API the above is using, by adding a translation to scrollable content. Note that this transform is applied to the content inside of the scrolling container.
The expectation is that the offset is really just how much has been scrolled, in the coordinate space of the scrolling layer. This means that e.g. translating the origin of the content down would not effect reported scrollTop start, but that content transforms can still indirectly effect scrollTop etc when they create more scrollable area.
Transforms against scrollview outer view, like in test plan, do not effect returned results.
@NickGerleman so what would be the next proper actions for this one?
Digging into the code calling this, to understand where the fix should be, so that it doesn’t break other usages.
Hello, I made changes last week and applied a transform only where necessary. I would appreciate it if someone could review and confirm that these changes are what we want. Thank you.
@realsoelynn you were digging into this, right? Could you take a look at current revision?
Sorry about my late reply. Was busy with ReactConf and internal summit. I am still doing some more investigation.
Like @NickGerleman mentioned below, scrollAwayPaddingTop should be excluded for transform.
Calculation for transform for ScrollViewNode need to be overridden specifically as done in the commit b25d6fe071e77dc64e6ce1030e97fe504ba96433. However, we will need to avoid break to DOM endpoints like @NickGerleman mentioned, i introduced includeTransform params in this PR https://github.com/facebook/react-native/pull/44822
Code Suggestion
Point ScrollViewShadowNode::getContentOriginOffset(
bool includeTransform) const {
auto stateData = getStateData();
auto contentOffset = stateData.contentOffset;
auto transform = includeTransform ? getTransform() : Transform::Identity();
auto result = transform * Vector{-contentOffset.x, -contentOffset.y, 0, 1};
return {result.x, result.y + stateData.scrollAwayPaddingTop};
}
I also went to experiment on DOM behavior of
scrollTopAPI the above is using, by adding a translation to scrollable content. Note that this transform is applied to the content inside of the scrolling container.The expectation is that the offset is really just how much has been scrolled, in the coordinate space of the scrolling layer. This means that e.g. translating the origin of the content down would not effect reported `scrollTop` start, but that content transforms can still indirectly effect scrollTop etc when they create more scrollable area.
Transforms against scrollview outer view, like in test plan, do not effect returned results.
@kosmydel Please address this code suggestion which is based on your original changes
Point ScrollViewShadowNode::getContentOriginOffset(
bool includeTransform) const {
auto stateData = getStateData();
auto contentOffset = stateData.contentOffset;
auto transform = includeTransform ? getTransform() : Transform::Identity();
auto result = transform * Vector{-contentOffset.x, -contentOffset.y, 0, 1};
return {result.x, result.y + stateData.scrollAwayPaddingTop};
}
@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@kosmydel Please help us rebase this PR so that we can do intake. Thanks in advance
@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@realsoelynn merged this pull request in facebook/react-native@3753b7a0e78f8820e5f5150dd9bdf1b53145a7bd.
This pull request was successfully merged by @kosmydel in 3753b7a0e78f8820e5f5150dd9bdf1b53145a7bd.
When will my fix make it into a release? | How to file a pick request?
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}