-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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] Fix scrolling issues with TextInputs in ScrollViews #25749
[Android] Fix scrolling issues with TextInputs in ScrollViews #25749
Conversation
Any updates ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix and for adding an example! Are E2E tests possible to test the example to add a regression safety net?
I tried to run the E2E test suite on Android but cannot seem to make it work, it seems the docker container is missing I can add regression tests from the example, but I cannot really test whether they failed prior to my fix on Android as they should be passing on iOS either way. Is there something else I could do to test it in a better way? |
@RobinCsl If you could add passing e2e tests that'd be great; that way if others have time, someone else can verify that it's broken without your fix on Android and is resolved after. |
Digging into this more, I'm a bit concerned about just reverting the original changes; I assume that we'll just cause the original bug to reappear if we aren't careful. I'll try to get more context about the original issue. In the meantime, is there a way to not delete this logic but instead take your specific repro cases into account in the logic to fix the bug? One more tip: in Android system settings you can enable showing touches, which will make your demonstration video more clear. It's pretty hard to tell in the first video what's wrong without being able to see touches. https://medium.theuxblog.com/enabling-show-touches-in-android-screen-recordings-for-user-research-cc968563fcb9 |
I'll do that this weekend hopefully!
If I remember well, I had tried to play with the logic I offer to delete and I did not manage to fix the issue other than by deleting it. I can try again though.
Absolutely, thank you for the Medium article, I had a look at it and could get my emulator to show touches. I'll re-record the examples this weekend and update my PR description. Thank you for your feedback! |
I tried to add passing e2e tests and I ran into the issue that it's difficult to reliably target where the scrolling takes place in Detox. There's meant to be a way to specify the start point (cf. I then had to modify my example to display the content of the ScrollView almost like a Picker component, in which case I was sure that the scrolling performed by Detox would be done "over" the TextInput I wanted to test. Finally, I ran into another issue that, on iOS, multiline TextInput's do capture the scroll event sometimes and that's probably what the original code that I offer to delete is solving (meaning, for the behaviour to be the same on Android). What's more, one-line TextInput don't scroll on iOS Sorry I don't have something more substantial to share just yet, I'll try and continue on it this week. |
@RobinCsl Thanks for all your work so far on this issue! Definitely an important one to solve. |
Any news on that? |
We need this Update! |
Hi @jschenkDD, |
Still waiting for merge... |
if (mDetectScrollMovement) { | ||
if (!canScrollVertically(-1) | ||
&& !canScrollVertically(1) | ||
&& !canScrollHorizontally(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
372d001 does not detect direction of the scroll
Note: this might change in the future to also detect the direction of the scroll, and
only block the scroll if the component can scroll in that direction.
The checks canScroll
are performed for all directions (for ex. to scroll down, we check that TextInput can scroll Up, Left and Right), if the TextInput
can not scroll in any direction, then the scroll requestDisallowInterceptTouchEvent
is disabled and scrolling goes back to the parent.
I would have to build a logic to detect the scroll direction and handle it correctly.
I'm working on this. Pull Request coming in 1-2 days. Thanks a lot 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is caused by canScrollHorizontally
returns allways true
for:
<TextInput multiline={false} textAlign="center" />
The method definition of canScrollHorizontally
.
I'll prepare a workaround
Hasn't been fixed yet? I'm still struggling with this issue |
any update on this? |
@RobinCsl Thanks for this work really appreciable |
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 7 days with no activity. |
This is my first PR to this repository and I would appreciate any feedback. Thanks
Summary
This change fixes #25594
The motivation is to allow
ScrollView
components to be correctly scrollable on Android when the touch falls on aTextInput
component.When a given
TextInput
component hastextAlign
property set tocenter
orright
, or when its value is long enough that theTextInput
becomes horizontally scrollable, it is no longer possible to use it as an anchor for a touch aiming at scrolling the parentScrollView
.Here is a Snack showing this behaviour (run it for Android, iOS is fine).
Here is a video (in GIF form) to show the behaviour in an example in RNTester (sadly, the touches don't really show, but I'm trying to take each input field as an anchor point to scroll vertically in the first half of the video, and horizontally in the second half of the video).
Even though the original implementation seems completely reasonable, the way Android seems to compute
canScrollHorizontally
orcanScrollVertically
seems non-functional when the text is centered or aligned to the right.I first tried to relax the condition to be "can't scroll vertically OR can't scroll horizontally", which yielded the desired behaviour, and commenting out the whole condition yielded the same. Upon inspection of the commit history, this change is more or less reverting this commit 372d001
Changelog
[Android] [Fixed] - TextInput components can be valid anchors to scroll within a ScrollView component
Test Plan
I ran my changes on RNTester on the Android emulator (Nexus_5X_API_27) but could not test on an Android device since I don't have one handy.
I added an example called "ScrollView with TextInputs".
Here is a video (in GIF form) to show the new behaviour:
TextInput
components can be taken as anchor points to scroll down the view, or sideways in the second example. (Sadly once again, touches aren't visible and the same comment as above applies)The only "downside" to this fix is that it is no longer possible to scroll horizontally single-line
TextInput
components' content if it is within a horizontalScrollView
.