Fix inverted flat list

[kosmydel]

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:

1. Use this reproducer: https://github.com/WoLewicki/reproducer-react-native/tree/%40wolewicki/flatlist-inverted
2. Apply changes from this PR & build the app.
3. Scroll a bit the list, so it changes the position.
4. The onPress should be fired when the button is clicked.
5. Do the following tests:
6. Add a horizontal prop to the FlatList - verify everything works.
7. Remove a inverted prop - verify everything works.
8. Remove a inverted prop and add a horizontal prop - verify everything works.
9. 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

react-native-flat-list.mov

RN tester

android-rn-tester.mov

[facebook-github-bot]

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 cla@meta.com. Thanks!

[cipolleschi]

/rebase - this comment rebase the PR on top of main automatically

[cipolleschi]

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?

I wonder how many other instance of similar issues we have sprinkled around. 🤔

[kosmydel]

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.

[roryabraham]

We tested this in a release build of the Expensify app and verified that it solves the problem for us 👍🏼

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[cipolleschi]

/rebase - this comment rebase the PR on top of main automatically

[cipolleschi]

CI is red with an invalid operation. I suspect that this is usually a warning, but we have a "treat warnings as errors" build flag, so it fails in CI.

[NickGerleman]

See above

[kosmydel]

I've addressed the review and solved the mentioned issues

[analysis-bot]

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: 6a3a305
Branch: main

[NickGerleman]

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 #43192 more broadly I think, since we need to after resolve transforms relative to view size.

[intergalacticspacehighway]

Suggested change

	auto result = props.transform * Vector{-contentOffset.x, -contentOffset.y, 0, 1};
	auto result = props.resolveTransform(layoutMetrics_) * Vector{-contentOffset.x, -contentOffset.y, 0, 1};

cc - @NickGerleman this should take the transformOrigin into consideration

[intergalacticspacehighway]

This might end up interacting with #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.

[facebook-github-bot]

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

[NickGerleman]

I took a closer look at the function where we are doing this, and found a couple more potential issues:

1. Making getContentOriginOffset() return transformed offset breaks the contract assumed by DOM code
2. This is called by getRelativeLayoutMetrics() which can be configured whether or not to use a transform. So we can't assume all callers there want a transform either

I.e. we're doing this transform at the wrong layer I think

[kosmydel]

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.

[NickGerleman]

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.

[WoLewicki]

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:

react-native/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp

Line 224 in c884d19

resultFrame.origin += currentShadowNode->getContentOriginOffset();

[NickGerleman]

@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).

[NickGerleman]

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.

[kosmydel]

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.

[NickGerleman]

@realsoelynn you were digging into this, right? Could you take a look at current revision?

[realsoelynn]

Sorry about my late reply. Was busy with ReactConf and internal summit. I am still doing some more investigation.

[realsoelynn]

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 #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 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.

[realsoelynn]

@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};
}

[facebook-github-bot]

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

[realsoelynn]

/rebase - this comment rebase the PR on top of main automatically

[realsoelynn]

/rebase

[realsoelynn]

@kosmydel Please help us rebase this PR so that we can do intake. Thanks in advance

[facebook-github-bot]

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

[facebook-github-bot]

@realsoelynn merged this pull request in 3753b7a.

[github-actions]

This pull request was successfully merged by @kosmydel in 3753b7a.

^{When will my fix make it into a release? | How to file a pick request?}

[realsoelynn]

Thank you everyone for contributing to fix Inverted FlatList 🥳