Fix can't close keyboard in some pages #46172
Merged
Conversation
This file contains 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
melvin-bot
bot
requested review from
aimane-chnaif
and removed request for
a team
|
@aimane-chnaif 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] |
bernhardoj
commented
Jul 25, 2024
| @@ -162,8 +162,10 @@ function PrivateNotesEditPage({route, personalDetailsList, report, session}: Pri | |||
| if (!el) { | |||
| return; | |||
| } | |||
| if (!privateNotesInput.current) { | |||
| updateMultilineInputRange(el); | |||
| } | |||
bernhardoj
commented
Jul 25, 2024
| @@ -90,8 +90,10 @@ function RoomDescriptionPage({report, policies}: RoomDescriptionPageProps) { | |||
| if (!el) { | |||
| return; | |||
| } | |||
| if (!reportDescriptionInputRef.current) { | |||
| updateMultilineInputRange(el); | |||
| } | |||
| reportDescriptionInputRef.current = el; | |||
There was a problem hiding this comment.
ios_desc_room.mp4
bernhardoj
commented
Jul 25, 2024
| @@ -178,8 +178,10 @@ function IOURequestStepDescription({ | |||
| if (!el) { | |||
| return; | |||
| } | |||
| if (!inputRef.current) { | |||
| updateMultilineInputRange(el); | |||
| } | |||
bernhardoj
commented
Jul 25, 2024
| updateMultilineInputRange(el); | ||
| if (!inputRef.current) { | ||
| updateMultilineInputRange(el); | ||
| } |
There was a problem hiding this comment.
ios_exit_survey.mp4
There was a problem hiding this comment.
Coming from #50962 (BZ Checklist):
We’re missing a test for this case, and an error is visible in the screenshot video when the user navigates from the reason page to the response page. To address this, we should prevent using focus at the same time as other input functionalities related to input selection.
bernhardoj
commented
Jul 25, 2024
| @@ -83,8 +83,10 @@ function NewTaskDescriptionPage({task}: NewTaskDescriptionPageProps) { | |||
| accessibilityLabel={translate('newTaskPage.descriptionOptional')} | |||
| role={CONST.ROLE.PRESENTATION} | |||
| ref={(el) => { | |||
| if (!inputRef.current) { | |||
| updateMultilineInputRange(el); | |||
| } | |||
There was a problem hiding this comment.
ios_task_description.mp4
bernhardoj
commented
Jul 25, 2024
| @@ -116,8 +116,10 @@ function TaskDescriptionPage({report, currentUserPersonalDetails}: TaskDescripti | |||
| if (!element) { | |||
| return; | |||
| } | |||
| if (!inputRef.current) { | |||
| updateMultilineInputRange(inputRef.current); | |||
| } | |||
| inputRef.current = element; | |||
There was a problem hiding this comment.
ios_task_description_2.mp4
bernhardoj
commented
Jul 25, 2024
| if (!isInputInitializedRef.current) { | ||
| updateMultilineInputRange(el); | ||
| } | ||
| isInputInitializedRef.current = true; |
There was a problem hiding this comment.
ios_profile_desc.mp4
|
^ Additional recording of the fix on other pages. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.mp4Android: mWeb Chromemchrome.mp4iOS: Nativeios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movMacOS: Desktop |
aimane-chnaif
approved these changes
Jul 29, 2024
blimpich
approved these changes
Jul 29, 2024
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
This was referenced Jul 30, 2024
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.15-0 🚀
|
roryabraham
removed
the
DeployBlockerCash
|
I think(not sure though) that this PR might have caused the regression here #46644 |
|
@MonilBhavsar I can't repro it but I think it's from this one #46177 |
|
Thanks for checking! Appreciate it |
6 tasks
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.15-9 🚀
|
6 tasks
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.
Details
In some pages, we use updateMultilineInputRange inside the input ref callback. The ref callback is called multiple times, including when we close the keyboard. updateMultilineInputRange in iOS focus on the composer, so each time we close the keyboard, the updateMultilineInputRange will focus on it again.
Fixed Issues
$ #46134
PROPOSAL: #46134 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
(mWeb/Android/iOS only)
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4