-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix enter key listeners with IME languages #16065
Conversation
…ation during text composition
This PR causes a minor regression on Android chrome. The regression is that on any On Android devices that uses Gboard (default), when you type text into any text fields on Chrome browser, I don't know if this constitutes as a regression. Please let me know your opinions. This does not happen on Screen.Recording.2023-03-17.at.2.56.18.PM.mov |
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@thienlnam Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/libs/KeyboardShortcut/index.js
Outdated
* @param {Event} event | ||
* @returns {boolean} | ||
*/ | ||
const isEnterWhileComposition = (event) => { |
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 naming for this method is a little confusing, what is the difference between this and when the enter key is pressed?
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.
Usually, when you type in English, the moment you inputted a character, the character is considered already "composed". However, in other languages that uses IME, the character you type is not considered "composed", meaning that the text is still in "draft" mode. When you type Enter
while the text is still draft mode, it's enterWhileComposing
. However, once the text is confirmed ("composed"), it's Enter
key event that happens outside composition flow.
I took hint for the variable name from KeyboardEvent.isComposing
(doc).
This seems like a regression we should try to solve in this PR.
Is this not bad? Space should act as a space and not submit right..? |
Also, let's have this PR reviewed by a C+ as well since it's a big change that could use some thorough testing |
@thienlnam thank yo for the quick review. I requested a review from @mananjadhav.
What I meant is the Gboard seems to consider even English input as in "draft" until you hit space. So the workaround for the regression is that you hit 'space', delete one empty space entered, and hit Enter. This is not a desired behavior and I'm going to investigate more how we can avoid this. |
I'll get to this by tomorrow. I am still going through the issue and the test steps. I'll check the current problem, the fix and also review the code. |
@hayata-suenaga I tried reproducing this on API 33 and API 30. I couldn't reproduce on either of the versions of Android emulator. Quick question though, do we know how many of the users actually use API 30? Because I do get the following error on API 30, when I load the app for the first time.
Is it safe to say we can ignore API 30? The error is related to Airship. |
@mananjadhav thank you for testing this. Could you tell me which part you couldn't reproduce? The original issue? (the original issue - premature sending of texts - only happens on web). And the regression (form cannot be submitted on enter on Android with Gboard) has been already solved. Could you also tell me when did you see the error (what action triggers the error you pasted)? |
@mananjadhav bump |
@hayata-suenaga Could you also please resolve the merge conflicts in PR as well? |
I am so sorry I missed notification on this. @hayata-suenaga Could you please resolve the conflicts and I can test. To answer the above questions, I tested on Android emulator API 30 and API 33, and the change worked fine on both the versions. I don't have a physical device with Gboard to test at the moment. |
# Conflicts: # src/libs/KeyboardShortcut/index.js # src/libs/KeyboardShortcut/index.native.js
@mananjadhav I solved the merge conflicts. CI tests are failing but I don't these tests are related to this PR. |
@hayata-suenaga Are you able to test this on the Android with the latest merge?
It is because we have an empty |
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 empty file src/libs/KeyboardShortcut/index.native.js
is causing a crash on Android. Can we please delete the unwanted file or add noop?
oops fixed @mananjadhav thank you for pointing that out |
I have one random issue on iOS Safari, when switching between the keyboards the composer loses focus. It exists on staging as well. mweb-safari-switch-keyboard-lose-focus.mp4 |
Reviewer Checklist
Screenshots/VideosWebweb-ime-keyboard.movMobile Web - Chromemweb-chrome-ime-keyboard.movMobile Web - Safarimweb-safari-ime-keyboard.mp4Desktopdesktop-ime-keyboard.moviOSios-ime-keyboard.movAndroidandroid-ime-keyboard.movThanks @hayata-suenaga for the patience here. I completed the checklist and updated the screencasts for all the platform. |
@thienlnam All yours. |
@mananjadhav thank you for your thorough testing.
This seems to be a different issue. Have you reported this somewhere else? |
No. I am yet to check if it is just due to the Safari behavior. |
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.
Great, looks good thank you!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.10-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
Details
When tying in a language that requires IME, don't trigger text submit event / callback on
Enter
while the text is being composed.This is from a slack message where I describe the problem and the expected behavior in detail.
Fixed Issues
$ GH_LINK #15798
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests / QA Tests
The tests steps for this PR is long, as this PR modified behaviors for text inputting, which is used across the App. We want to be extra careful that this PR didn't break what's working.
Changed Behaviors
Test what have been changed. In the following test steps, you'll test text input behaviors for both Japanese and English. When you're using English, you're testing that typing in English works as normal. When you're using Japanese, you're testing that the issue with typing in Japanese has been fixed.
For engineers: The following steps test
KeyboardShortcut.bindHandlerToKeydownEvent
(used inReportActionComposer
as well as other components),EmojiPickerMenu
andFormSubmit
(used inAddDebitCardPage
).smile
. Highlight the first option on the popover by pressingTab
key. HitEnter
to select it. HitEnter
again to confirm the text. HitEnter
again to select the highlighted emoji. I know this is a lot ofEnter
presses, but this is the normal behavior when inputting Japanese language.Enter
. Confirm that the form submission is attempted by observing other fields returning red with error messages.Enter
. Confirm that submission is NOT attempted by observing that other fields are NOT turned red. HitEnter
again and confirm that the form submission is attempted by observing other fields returning red with error messages.Unchanged Behaviors
Typing on NewDot running on Android or iOS or mobile web is already working correctly before this PR. Check the behavior of the text input is same as the current one by running this PR on simulators/emulators.
I also attached videos below checking that text input behavior on native Android/iOS devices and mobile web is not affected by this PR.
Offline tests
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web Chrome
Web Safari
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
ios.mp4
Android
android.webm