-
Notifications
You must be signed in to change notification settings - Fork 3k
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 chrome android input bug #55210
Fix chrome android input bug #55210
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@allgandalf 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] |
@allgandalf if you have some android Phones please test this on device using Chrome. Possible bugs would be strange looking keyboard. I'll test it more on Monday with some Androids other than Pixel, but right now I have to go. |
FYI @allgandalf I've checked on three more phones and this seems to work alright on all of them. |
Small bump @allgandalf 🙇 |
sorry, I am low on availability today, I will try to finish review today itself, but can go to monday 😓 |
Take your time, just wanted to make sure we don't forget about it 😄 |
@SzymczakJ sorry for the delay i was out for more than i expected, can you please fix conflicts, I will be quick to review this one |
I'll do it today |
Ready for review 😄 |
Reviewer Checklist
Screenshots/Videos |
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.
I tested this code it LGTM!
All yours @yuwenmemon 🙇
@@ -265,6 +265,9 @@ function BaseTextInput( | |||
|
|||
const inputPaddingLeft = !!prefixCharacter && StyleUtils.getPaddingLeft(StyleUtils.getCharacterPadding(prefixCharacter) + styles.pl1.paddingLeft); | |||
const inputPaddingRight = !!suffixCharacter && StyleUtils.getPaddingRight(StyleUtils.getCharacterPadding(suffixCharacter) + styles.pr1.paddingRight); | |||
// This is workaround for https://github.com/Expensify/App/issues/47939: in case when user is using Chrome on Android we set inputMode to 'search' to disable autocomplete bar above the keyboard. |
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 this comment it helps a lot debugging in the future ❤️
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.0.96-0 🚀
|
This PR is failing because of issue #56681 |
This is kind of intentional(we cannot do anything about this), more info here |
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.96-1 🚀
|
Explanation of Change
This PR introduces workaround for #47939: in case when user is using Chrome on Android we set inputMode to 'search' to disable autocomplete bar above the keyboard.
If we need some other inputMode (eg. 'decimal'), then the autocomplete bar will show, but we can do nothing about it as it's a known Chrome bug.
Fixed Issues
$ #47939
PROPOSAL:
Tests
This PR is solely about Text input on Android chrome. Therefore we should test text inputs Android on:
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop