[RNMobile] Resolve Text Entry Issues/Errors in Alt Text Settings #33975
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit aims to fix the render error outlined here: #33057 (review) The "outside" changes that happen when text is changed in the BottomSheetTextControl component should not be called directly from that component. Instead, they're now called within the useEffect hook.
SiobhyB
added
[Type] Code Quality
|
Size Change: +262 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
The purpose of this commit is to address the text entry issues that currently exist for the TextInput component, as described here: facebook/react-native#30503 A working workaround propose in that thread is to replace the "value" prop with "defaultValue".
SiobhyB
changed the title
SiobhyB
changed the title
2 tasks
Contributor
Author
|
@fluiddot, looking into a fix for issues with text entry in the alt text settings also had the happy side effect of addressing the error you first reported at #33057 (review). :) I created installable builds for Android and iOS to help with testing, as I think it's easier to both replicate the issues and confirm the fix on physical devices. Let me know if I can help to clarify anything on this PR! |
Contributor
Awesome, thanks @SiobhyB for addressing all the fixes here and create the installable builds 🎊 ! I'll try to spare some time this week to take a look and let you know if I have any feedback 🙇 . |
fluiddot
reviewed
Aug 18, 2021
Contributor
fluiddot
left a comment
fluiddot
left a comment
There was a problem hiding this comment.
Thanks @SiobhyB for addressing the issues 💯 !
I verified the different test cases but I posted a comment regarding the possibility to simplify the component, let me know your opinion.
- Test Case 1: General Flow ✅
- Test Case 2: Deleting Text ✅
- Test Case 3: Typing Swiftly ✅
- [Developer] Test Case 4: Verify No Errors in Developer Console ✅
Tested on Simulator iPad Pro (12.9-inch) - 4th generation (iOS 14.4) and Samsung Galaxy S20 FE 5G (Android 10).
packages/components/src/mobile/bottom-sheet-text-control/index.native.js
Outdated
Show resolved
Hide resolved
The introduction of the "defaultValue" prop in f6d9697 means that it's no longer necessary to keep the value in state. "defaultValue" allows for text to be automatically changed as it's typed. See: https://reactnative.dev/docs/textinput#defaultvalue We don't need to use "onChangeText" for the purpose of updating the input's text and can instead use it to invoke the "onChange" function. We can also remove the useState and useEffect hooks, as they're not necessary to update the component's value.
fluiddot
approved these changes
Sep 2, 2021
Contributor
fluiddot
left a comment
fluiddot
left a comment
There was a problem hiding this comment.
LGTM 🎊 !
Tested on Simulator iPhone 11 Pro Max (iOS 14.4) and Samsung Galaxy S20 FE 5G (Android 10).
I tested all cases again and they worked perfect 💯 :
- Test Case 1: General Flow ✅
- Test Case 2: Deleting Text ✅
- Test Case 3: Typing Swiftly ✅
- [Developer] Test Case 4: Verify No Errors in Developer Console ✅
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes:
Gutenberg Mobile:
Description
The code introduced in this PR addresses the following three issues with the text entry field in the image block's alt text settings:
An open issue in the React Native repository, facebook/react-native#30503, lines up with the behavior described in the first two issues. That issue outlines the fact that this was introduced in React Native 0.63.0 and is related to global state management with the
TextInputcomponent. The third issue is semi-related, and resolved through the code from this PR, given it also touches on state management.Further technical details around the code changes are provided in the Types of changes section below.
How has this been tested?
I completed the following test cases on a physical Pixel 5, Android version 11 and an iPhone XR, iOS 14.6.
While going through the below flows, it'd be useful to also think about and experiment with different edge cases e.g. entering return at random while typing swiftly or clicking delete abruptly.
Test Case 1: General Flow
To start with, the general flow for adding new alt text is as follows.
Test Case 2: Deleting Text
We should also verify that the cursor no longer changes position at random after deleting text.
Test Case 3: Typing Swiftly
Next, we should verify that it's possible to type swiftly into the text editor, with no unwanted side effects, such as duplication.
[Developer] Test Case 4: Verify No Errors in Developer Console
While testing the branch on a local setup, confirm that there is no longer a warning related to the
BottomSheetTextControlcomponent when opening the image block's settings.Screenshots
Previously, hitting the backspace/delete key would lead to the cursor jumping, but you'll see this is no longer the case in the following video:
Screen.Recording.2021-08-10.at.19.28.44.mov
Types of changes
This PR introduces a bug fix (a non-breaking change which fixes an issue) with the following high-level overview of the code changes:
TextInputcomponent is being used in theBottomSheetTextControlcomponent. It's this component that's used to enable users to enter alt text via the image block's settings.onChangeprop was being used to create a side effect when text was typed into theTextInputcomponent. AsonChangeusessetState, this was creating the error outlined here. The solution was to remove the reference toonChangeand move the functionality to auseEffecthook.TextInputcomponent'svalueprop was changed todefaultValue, as per the suggestion in this open bug report to workaround a known React Native issue with theTextInputcomponent.Checklist:
*.native.jsfiles for terms that need renaming or removal).