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

[Android] [Lists] Workaround a wrong fling direction for inverted ScrollViews on Android P #21117

Closed

Conversation

mandrigin
Copy link

@mandrigin mandrigin commented Sep 14, 2018

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

Test Plan:

  1. Create a test application with a FlatList with inverted={true}.
  2. Fill it with data
  3. Scroll and release your finger

Expected:
On Android 8 and 9 the behaviour is the same.

Release Notes:

[ANDROID] [BUGFIX] [ScrollView] - Workaround a wrong fling direction for inverted ScrollViews on Android P

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 14, 2018
@mandrigin mandrigin force-pushed the workaround/fling-on-android-P branch 2 times, most recently from 59cf86c to 107a9ec Compare September 14, 2018 13:25
@mandrigin mandrigin changed the title [Android] [ScrollView] Workaround a wrong fling direction for inverted ScrollViews on Android P [Android] [Lists] Workaround a wrong fling direction for inverted ScrollViews on Android P Sep 14, 2018
chrisnojima added a commit to keybase/react-native that referenced this pull request Sep 17, 2018
chrisnojima added a commit to keybase/react-native that referenced this pull request Oct 15, 2018
//
// Hence, we can use the absolute value from whatever the OS gives
// us and use the sign of what mOnScrollDispatchHelper has tracked.
velocityY = (int)(Math.abs(velocityY) * Math.signum(mOnScrollDispatchHelper.getYFlingVelocity()));
Copy link

Choose a reason for hiding this comment

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

I tested this and is working, what is not so good practice in general is changing the argument value, instead create a new var. Thank you for this!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing!
I’m away from my laptop this week, will address you comment on Monday.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Ruben's feedback, once the PR is updated I'll try to import it :) Thanks for doing this btw 👏

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot!
Yeah, I agree about a variable, just wanted to make a smallest possible diff.
I fixed it and rebased to the latest master.

@watadarkstar
Copy link

We will be using this in production until it gets merged! 👍 CC: @listicos who reviewed this for us on our team!

@mandrigin mandrigin force-pushed the workaround/fling-on-android-P branch from 107a9ec to f9ed777 Compare November 13, 2018 17:02
@mandrigin mandrigin force-pushed the workaround/fling-on-android-P branch from f9ed777 to 1ae9b9a Compare November 13, 2018 17:06
@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

chrisnojima added a commit to keybase/react-native that referenced this pull request Nov 14, 2018
@mandrigin
Copy link
Author

@listicos @kelset please ping me if anything else needs to be done in this PR 🙂
I assume the feedback is addressed and it is rebased. I don't know why analyze fails, it doesn't seem to be related to my changes.

@kelset
Copy link
Contributor

kelset commented Nov 15, 2018

Hey Igor, thanks!

I'll try to trigger the import, but can't assure when it will land because this past week the FB team seemed super busy 🤷‍♂️

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 15, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@kelset is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@mandrigin merged commit b971c5b into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Nov 15, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 15, 2018
kelset pushed a commit that referenced this pull request 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
sjchmiela pushed a commit to expo/react-native that referenced this pull request 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 pull request 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
@hramos hramos removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 6, 2019
@hramos hramos removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 6, 2019
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants