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

Inverted FlatList doesn't work on Android P #19434

Closed
Rongrs opened this issue May 24, 2018 · 36 comments
Closed

Inverted FlatList doesn't work on Android P #19434

Rongrs opened this issue May 24, 2018 · 36 comments
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Platform: Android Android applications. Resolution: Locked This issue was locked by the bot. Resolution: PR Submitted A pull request with a fix has been provided.

Comments

@Rongrs
Copy link

Rongrs commented May 24, 2018

When using FlatList with inverted={true} the scrolling doesn't work as expected.
it seems that the momentum is not inverted, and at the end of the scroll - the momentum takes it back to the other side

Environment

Environment:
OS: macOS High Sierra 10.13.4
Node: 8.11.1
Yarn: 1.5.1
npm: 5.6.0
Watchman: 4.9.0
Xcode: Xcode 9.3 Build version 9E145
Android Studio: 3.1 AI-173.4670197

Packages: (wanted => installed)
react: 16.3.1 => 16.3.1
react-native: ~0.55.2 => 0.55.4

Steps to Reproduce

With Android P public beta installed -
https://snack.expo.io/BklXgV8Nk7

Expected Behavior

scrolling will work as usual

Actual Behavior

momentum is inverted

@arendjr
Copy link

arendjr commented Jul 30, 2018

For anyone else running into this issue, I could also suggest looking into this library: https://github.com/godness84/react-native-recyclerview-list

It's not my project, but because of this specific issue I've opened a PR for them that allows it to be used in combination with inverted lists: godness84/react-native-recyclerview-list#25 Update: The latest release (0.3.4) includes the inverted prop.

@alebedev
Copy link

alebedev commented Aug 7, 2018

Reproducible on release version of Android P

@Rongrs
Copy link
Author

Rongrs commented Aug 7, 2018

Reproducible also on native android app (example), with scrollView and scaleY=-1
same app don't have the problem on Oreo devices.

so it may be a native issue, and possibly related to this:
https://developer.android.com/about/versions/pie/android-9.0-changes-28#scrolling-element
?

@enahum
Copy link
Contributor

enahum commented Aug 7, 2018

@arendjr that recyclerview-list looks amazing, are you aware of a similar project for iOS?

@arendjr
Copy link

arendjr commented Aug 8, 2018

@enahum No sorry, the catch is indeed it's Android-only. I haven't really looked for iOS counterparts though. Ideally I'd still hope this behavior gets properly fixed in FlatList, of course.

@jarredwitt
Copy link

jarredwitt commented Aug 8, 2018

So I've been digging into this a bit more and have landed on this line:

Seems that thevelocityY thats coming in is negative when the FlatList is inverted and you fling to scroll up causing the list to start to scroll up but then shoot back down. Was able to patch the issue by setting velocityY = velocityY * -1; within the if statement. Obviously not a real fix but hopeful that it at least sheds some light on what may be the cause. Going to continue diving in to see if I can chase it down.

@dgritsko
Copy link

@jarredwitt Any luck getting to the bottom of this?

@smerat
Copy link

smerat commented Aug 16, 2018

I'd appreciate any help on this 🙇

@Rongrs
Copy link
Author

Rongrs commented Aug 16, 2018

Opened an issue for Android, since it happens in native apps with scrollView and scaleY=-1
https://issuetracker.google.com/issues/112385925
you can try and star it to let them know it's needed :-)

@Rongrs
Copy link
Author

Rongrs commented Aug 16, 2018

Our temp solution was to natively inherite scrollView, override onTouchEvent and call fling with the correct velocity when ACTION_UP was called

@Override
public boolean onTouchEvent(MotionEvent ev) {
        if (Build.VERSION.SDK_INT >= 28) {
            initVelocityTrackerIfNotExists();
            _velocityTracker.addMovement(ev);

            if (ev.getActionMasked() == ACTION_UP && _dragging) {
                _velocityTracker.computeCurrentVelocity(1000, _maximumVelocity);
                int velocityY = (int) _velocityTracker.getYVelocity(ev.getPointerId(0));
                int velocityX = (int) _velocityTracker.getXVelocity(ev.getPointerId(0));

                int scrollY = getScrollY();
                if ((Math.abs(velocityY) > _minimumVelocity) && (scrollY > 0 || velocityY > 0) && (scrollY < getScrollRange() || velocityY < 0)) {
                    fling(velocityY);
                }
                recycleVelocityTracker();

                ReactScrollViewHelper.emitScrollEndDragEvent(this, velocityX, velocityY);
                _dragging = false;

                return true;
            }
        }

        return super.onTouchEvent(ev);
    }

@krishom
Copy link

krishom commented Aug 17, 2018

I used a similar workaround to @Rongrs by checking this.getScaleY() < 0 and Build.VERSION in
https://github.com/facebook/react-native/blob/0.54-stable/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L270

  @Override
  public void fling(int velocityY) {
    if (mScroller != null) {
      // FB SCROLLVIEW CHANGE

      // We provide our own version of fling that uses a different call to the standard OverScroller
      // which takes into account the possibility of adding new content while the ScrollView is
      // animating. Because we give essentially no max Y for the fling, the fling will continue as long
      // as there is content. See #onOverScrolled() to see the second part of this change which properly
      // aborts the scroller animation when we get to the bottom of the ScrollView content.

      int scrollWindowHeight = getHeight() - getPaddingBottom() - getPaddingTop();

      if (this.getScaleY() < 0 && Build.VERSION.SDK_INT > Build.VERSION_CODES.O_MR1) {
        velocityY *= -1;
      }

      mScroller.fling(
        getScrollX(),
        getScrollY(),
        0,
        velocityY,
        0,
        0,
        0,
        Integer.MAX_VALUE,
        0,
        scrollWindowHeight / 2);

      postInvalidateOnAnimation();

      // END FB SCROLLVIEW CHANGE
    } else {
      super.fling(velocityY);
    }

@jarredwitt
Copy link

@krishom is this working well for you? I'm trying to test out your code, but can't get it to work. getScaleY always returns 1.0f for me.

@krishom
Copy link

krishom commented Aug 21, 2018

@jarredwitt Yeah, my fix is working on a Pixel 2 running Android P. The ScrollView lives in a <SectionList inverted> component. Before discovering the getScaleY() check, I was dispatching a command across the bridge during the SectionList's first onScroll event to notify the ScrollView to invert the fling velocity. This also worked to some extent, but wasn't as clean of a fix.

@jarredwitt
Copy link

@krishom interesting. Actually debugging right now and can't get getScaleY to return anything other than 1.0. Can I ask what your targetSdkVersion is set to?

@krishom
Copy link

krishom commented Aug 21, 2018

@jarredwitt build.gradle:
targetSdkVersion 27

@scottarivale
Copy link

scottarivale commented Sep 6, 2018

A workaround that works for us that's far simpler but may not be a viable option for everyone: don't use inverted={true} and instead just reverse the order of the data you are passing to the FlatList.

@kelset
Copy link
Contributor

kelset commented Sep 10, 2018

Just tested locally with 0.57-rc4 and sadly neither of the last two commits by @olegbl seems to have fixed it.

Will try to have someone from the core take a closer look at this since it's something that I fear in a couple months will affect a good portion of Pixel users.

cc @dulmandakh

@mandrigin
Copy link

mandrigin commented Sep 14, 2018

@kelset I tried to make a workaround as safe as possible and w/o having to check for a build number: https://github.com/facebook/react-native/pull/21117/files
That fixed the issue for us.

I will be happy to hear your feedback!

@ggsrivas
Copy link

@Rongrs @jarredwitt @krishom Could you please explain, how are you using overridden ReactScrollView.java class with your app? Are you creating a custom build of react-native .aar file?

@githubdoramon
Copy link

Any news on this from the react native team side?

@roylabanon
Copy link

How to apply the changes made for this files?

@mandrigin
Copy link

@roylabanon the easiest way is to use a fork of React Native and apply the patch there.

Having a fork might be useful for other things as well (let's say you need to expose a property that RN doesn't expose by default, etc).

@roylabanon
Copy link

roylabanon commented Oct 10, 2018 via email

@johakr
Copy link

johakr commented Oct 10, 2018

@roylabanon You need to build react-native from source in order to make any android changes take effect. Maintaining a fork of react-native isn't as trivial as forking the github repo and commiting your local changes. It always involves an additional building step, at least for android. Good luck.

@mandrigin
Copy link

@roylabanon it isn't super trivial, but that is how we did it.
Here is our RN fork (with the branch containing the fix): https://github.com/status-im/react-native/tree/status-0.56-1
And here is the PR that tweaks all the project settings: https://github.com/status-im/status-react/pull/4981/files

Note, that apart from that you will need to setup your dev environment (install Android NDK, etc), but that is very well described in the official docs (as @johakr mentioned).

@roylabanon
Copy link

@roylabanon it isn't super trivial, but that is how we did it.
Here is our RN fork (with the branch containing the fix): https://github.com/status-im/react-native/tree/status-0.56-1
And here is the PR that tweaks all the project settings: https://github.com/status-im/status-react/pull/4981/files

Note, that apart from that you will need to setup your dev environment (install Android NDK, etc), but that is very well described in the official docs (as @johakr mentioned).

Great thanks so much guys

@kaitlynbrown
Copy link

@kelset I tried to make a workaround as safe as possible and w/o having to check for a build number: https://github.com/facebook/react-native/pull/21117/files
That fixed the issue for us.

I will be happy to hear your feedback!

Is there any chance of this PR being merged before the next release?

@watadarkstar
Copy link

Following also have this problem 👍

@kelset
Copy link
Contributor

kelset commented Nov 9, 2018

hey sorry, I completely missed all notifications about this lately 🤦‍♂️

I'll double check the PR and we'll try to get it in asap (but probably won't be next patch for 0.57.x, we'll see - if not it will be in the one after that).

@kelset kelset added the Resolution: PR Submitted A pull request with a fix has been provided. label Nov 9, 2018
@watadarkstar
Copy link

Thanks @kelset - this effect all chat apps and anything that uses inverted={true} on a Flatlist.

@scottmas
Copy link

Did this fix land in 0.57.5? The PR #21117 was merged right before the release but I'm crossing my fingers that maybe somehow it sneaked in...

@watadarkstar
Copy link

@kelset will be able to tell you if it landed

@kelset
Copy link
Contributor

kelset commented Nov 20, 2018

@scottmas you can check the changelog to know which commits landed and which not.

You can also suggest commits that landed to be cherry picked in the dedicated issue - in particular, this commit has already been requested.

@ilevator
Copy link

Nope, this fix didnt make into 0.57.3.

@kelset
Copy link
Contributor

kelset commented Nov 26, 2018

@ilevator latest version is 0.57.5

kelset pushed a commit that referenced this issue Nov 26, 2018
…d P (#21117)

Summary:
This is a safe workaround to an issue in Android P: https://issuetracker.google.com/issues/112385925

It is based on a fact that even though `fling` receive a wrong sign in `velocityY`, `mOnScrollDispatchHelper.getYFlingVelocity()` still returns a fling direction.

Fixes #19434
Pull Request resolved: #21117

Differential Revision: D13082740

Pulled By: hramos

fbshipit-source-id: 1b28586d2c7bdcae4a111d3cead4a0455cebb99a
@Gazfay
Copy link

Gazfay commented Nov 30, 2018

0.57.7 looks like fixed!

@facebook facebook locked as resolved and limited conversation to collaborators Dec 3, 2018
sjchmiela pushed a commit to expo/react-native that referenced this issue Dec 15, 2018
…d P (facebook#21117)

Summary:
This is a safe workaround to an issue in Android P: https://issuetracker.google.com/issues/112385925

It is based on a fact that even though `fling` receive a wrong sign in `velocityY`, `mOnScrollDispatchHelper.getYFlingVelocity()` still returns a fling direction.

Fixes facebook#19434
Pull Request resolved: facebook#21117

Differential Revision: D13082740

Pulled By: hramos

fbshipit-source-id: 1b28586d2c7bdcae4a111d3cead4a0455cebb99a
sjchmiela pushed a commit to expo/react-native that referenced this issue Dec 15, 2018
…d P (facebook#21117)

Summary:
This is a safe workaround to an issue in Android P: https://issuetracker.google.com/issues/112385925

It is based on a fact that even though `fling` receive a wrong sign in `velocityY`, `mOnScrollDispatchHelper.getYFlingVelocity()` still returns a fling direction.

Fixes facebook#19434
Pull Request resolved: facebook#21117

Differential Revision: D13082740

Pulled By: hramos

fbshipit-source-id: 1b28586d2c7bdcae4a111d3cead4a0455cebb99a
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…d P (facebook#21117)

Summary:
This is a safe workaround to an issue in Android P: https://issuetracker.google.com/issues/112385925

It is based on a fact that even though `fling` receive a wrong sign in `velocityY`, `mOnScrollDispatchHelper.getYFlingVelocity()` still returns a fling direction.

Fixes facebook#19434
Pull Request resolved: facebook#21117

Differential Revision: D13082740

Pulled By: hramos

fbshipit-source-id: 1b28586d2c7bdcae4a111d3cead4a0455cebb99a
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Nov 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Platform: Android Android applications. Resolution: Locked This issue was locked by the bot. Resolution: PR Submitted A pull request with a fix has been provided.
Projects
None yet
Development

No branches or pull requests