-
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] ScrollView with TextInput not detecting Scroll Gesture #29025
[Android] ScrollView with TextInput not detecting Scroll Gesture #29025
Conversation
Base commit: 4324ca8 |
Base commit: 4324ca8 |
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.
@JoshuaGross has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@fabriziobertoglio1987 This doesn't work quite as I would expect. Specifically, in the second "horizontal" example, I can scroll the TextInput left but it seems there's no way to scroll the TextInput right, even if my finger is touching the TextInput and not the ScrollView space. That doesn't feel intuitively correct to me? |
Maybe there's a different repro that would help me understand the issue. Using the example you provided, it seems correct without any Java changes and doesn't feel right with these Java changes. |
canScrollHorizontally(-1) detects if a TextInput can scroll to the right, but it does not work with Gravity.CENTER. Consequence is that the TextInput can not be scrolled right and the parent component (in our example a ScrollView) is instead scrolled. For this reason the TextInput Gravity is set to Gravity.LEFT when focused for editing and set back to Gravity.CENTER once the focus leaves the TextInput. canScrollHorizontally(1) is used for detecting that a TextInput can scroll left, but does not work with Gravity.CENTER. The TextInput will not scroll to the last character and canScrollHorizontally(1) returns allways true with Gravity.CENTER. Consequence is scroll is never delegated to the parent View (ScrollView in our example) and the parent view (ScrollView) can not be scrolled left once the end of the TextInput is reached. I was aware of this issue and fixed previously by with setGravity.(Gravity.LEFT), but I did not use the correct event for settings the gravity back to Gravity.CENTER. This commit uses event onScrollChanged to set the gravity back to Gravity.CENTER if previously resetted. The gravity is set back to Gravity.CENTER once the focus leaves the TextInput and the Scrolling is ended.
as explained in previous commit de24234 canScrollHorizontally() does not work with Gravity.CENTER. For this reason we set the Gravity to LEFT once the focus is moved into the TextInput and the user edits the TextInput content, once the TextInput loses the focus, the Gravity is set back to CENTER. The functionality allows the user to Scroll Both the TextInput and the Parent once the TextInput reached his end, yet other functionality make the TextInput always move to his cursor position, which means that the cusor should move to scroll the TextInput, but changes to this that should be discussed in another pull request.
Thanks a lot and Sorry @JoshuaGross for my mistakes. The changes to fix the current Java functionalities are included in commits de24234 514da13 and 5a8bf3a. I also improved RNTester to be easy to test with a Real Device. I tested with two devices and did not experience anymore issues. Thanks a lot for your help and tolerance for my errors.
|
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.
@JoshuaGross has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@fabriziobertoglio1987 No need to apologize, I appreciate you spending time on this. I think the onscroll code makes sense now. My only remaining concern is with the scroll gravity logic - is it necessary? I think it could break some TextInputs if the default gravity for that TextInput isn't specified as LEFT. It seems to work fine without the gravity bit at all? |
@JoshuaGross thanks a lot for the reviewing this pull request. I appreciate your feedback. QUESTION 1
The previous functionality was built in 372d001, @andreicoman11 may also have an interesting opinion on this change. As demonstrated in the comment below from #25749 (comment), commits de24234, 514da13, summary #29025 (comment), canScrollHorizontally does not work with The code below does not work with react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java Lines 183 to 184 in cfa4260
QUESTION 2
I am not currently aware of those scenarios, but I will keep researching. I did not invest time searching for an alternative solution to 514da13. Commit 514da13 fixes #25594, #12167 and #13997 I will be contributing to ReactNative master full-time until end of August 2020 and I will solve any related issue to this pull request in 1-2 days. I am subscribed to all react-native threads and I read all
I believe the workaround will only change the @Override
protected void onFocusChanged(boolean focused, int direction, Rect previouslyFocusedRect) {
super.onFocusChanged(focused, direction, previouslyFocusedRect);
if (focused && mSelectionWatcher != null) {
mSelectionWatcher.onSelectionChanged(getSelectionStart(), getSelectionEnd());
}
boolean resetGravity = getGravity() == Gravity.CENTER &&
mMultiLine == false;
if(focused && resetGravity) {
setGravity(Gravity.LEFT);
mGravityReset = true;
};
if(!focused && mGravityReset) {
setGravity(Gravity.CENTER);
mGravityReset = false;
}; I remain availalbe for further help and additional changes. |
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.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
Outdated
Show resolved
Hide resolved
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.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
Summary
This issue fixes #25594 fixes #12167 fixes #13997
The ScrollView functionality is not compatible with TextInput scrolling. This pull request implements the scroll functionality of a TextInput in a ScrollView and fixes a bug causing ScrollView to not detect scroll gesture when including a TextInput with textAlign center.
ADDITIONAL INFORMATION
canScrollHorizontally(-1) detects if a TextInput can scroll to the right, but it does not work with Gravity.CENTER. Consequence is that the TextInput can not be scrolled right and the parent component (in our example a ScrollView) is instead scrolled. For this reason the TextInput Gravity is set to Gravity.LEFT when focused for editing and set back to Gravity.CENTER once the focus leaves the TextInput. Similar issues with scrolling left (the TextInput never reaches the end).
This commit uses event onScrollChanged to set the gravity back to Gravity.CENTER if previously resetted. The gravity is set back to Gravity.CENTER once the focus leaves the TextInput and the Scrolling is ended.
The functionality allows the user to Scroll Both the TextInput and the Parent once the TextInput reached his end, yet other
functionality make the TextInput always move to his cursor position, which means that the cusor should move to scroll the TextInput, but changes to this that should be discussed in another pull request.
Changelog
[Android] [Fixed] - ScrollView with TextInput not detecting Scroll Gesture and issues with textAlign center
Test Plan
CLICK TO OPEN TESTS RESULTS
This pull request solves problems connected to 372d001 as explained in #25749 (comment) and #25749 (comment)
The below example is available in RNTester.