Fix keyboard issues in add attachment flow #15337

priyeshshah11 · 2023-02-22T01:29:59Z

Details

A. For iOS: When opening the PopoverMenu, the keyboard will be closed (but the state of the keyboard opening is remembered by the OS). Then clicking Add attachment, the PopoverMenu is closed but the OS will reopen the keyboard. So when the AttachmentPicker is opened, due to this the keyboard is immediately closed, causing the glitch. We should only open the Modal component after the keyboard is completely hidden and when the AttachmentPicker, AttachmentModal, and PopoverMenu is closed/hidden, the keyboard will be reopened as expected.

B. For mWeb: When opening the PopoverMenu and then clicking Add attachment, the PopoverMenu will be closed. But the input is then focused and the keyboard is pushed up. After that, the native file picker appears and dismisses the keyboard immediately, causing the flickering. We should add a new state to indicate that we're opening the attachment picker and we do not refocus the input here if this state is true.

This PR does those

Fixed Issues

$ #13922
PROPOSAL: #13922 (comment)

Tests

Click & focus on composer to open the keyboard on mobile.
Click on '+' in composer, verify the keyboard closes smoothly & then the modal appears.
Click add attachment in this modal & verify this modal closes smoothly before opening the second modal.
Complete each attachment flow (photos, camera, files) one by one & the composer should be focused & keyboard should reopen after completing each flow.
Note: Keyboard won't open back on mWeb iOS which is known issue & out of scope of this PR

Verify that no errors appear in the JS console

Offline tests

Same as above

QA Steps

Same as Tests above

Additional step: Verify the keyboard is focused back on cancelling/exiting out of the add attachment flow at every/any step except for mWeb iOS.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

mWeb.Chrome.mov

Mobile Web - Safari

Mweb.-.Safari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

Android.mov

MelvinBot · 2023-02-22T01:30:07Z

@joelbettner @thesahindia One of you needs to 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]

tienifr · 2023-02-22T06:36:18Z

src/pages/home/report/ReportActionCompose.js

@@ -613,8 +626,10 @@ class ReportActionCompose extends React.Component {
                                                        icon: Expensicons.Paperclip,
                                                        text: this.props.translate('reportActionCompose.addAttachment'),
                                                        onSelected: () => {


@priyeshshah11 we need to add the this.focus(true); inside onClose of PopoverMenu as well, otherwise if the user clicks + and then click outside, it will not focus the input

…issing native picker once & DRY refactor

priyeshshah11 · 2023-02-23T08:05:19Z

@tienifr @thesahindia @joelbettner All videos have been uploaded & PR is ready for review

thesahindia · 2023-02-23T08:20:37Z

Reviewing in a few hours.

priyeshshah11 · 2023-02-23T10:21:07Z

Noting here - We have an edge case bug where we won't get the focus back on mWeb iOS if the user cancels the native file picker as there is no way to detect that in mWeb iOS because the focus never goes away from the screen nor there is any event listener for that https://stackoverflow.com/questions/74861458/how-to-tell-if-user-cancels-html-file-input-on-ios

thesahindia · 2023-02-23T16:24:21Z

Hey @priyeshshah11, it looks like the keyboard isn't opening in android when I open actions menu and close it. It works on emulator but not on a real device.

priyeshshah11 · 2023-02-23T16:28:50Z

Hey @priyeshshah11, it looks like the keyboard isn't opening in android when I open actions menu and close it. It works on emulator but not on a real device.

Ohh, unfortunately I don't have an android device to debug. @tienifr do you happen to have an android device? I'll still try to figure out by looking at the code, it could be the textInput.blur() that we added to fix the issue that you were seeing on Android, maybe that was just a simulator bug?

priyeshshah11 · 2023-02-23T16:35:19Z

@thesahindia Meanwhile for quick testing, could I please request you to test it with commenting this line? Normally I wouldn't ask, sorry 😢

thesahindia · 2023-02-23T16:52:51Z

Still no luck

Screenrecording_20230223_222212.mp4

priyeshshah11 · 2023-02-23T17:09:29Z

Passing in true here should fix it, testing on other platforms.

thesahindia · 2023-02-23T18:25:49Z

It's still the same.

priyeshshah11 · 2023-02-23T22:51:24Z

Thanks for trying that @thesahindia and yes you're right that wouldn't have made any difference on native as that condition is always false on native and it looks like we are actually doing that on purpose to avoid some jarring UI when keyboard opens on native

tienifr · 2023-02-24T06:28:03Z

@thesahindia @priyeshshah11 I don't encounter the Android keyboard not opening issue mentioned above, on my physical Android device

Screenrecorder-2023-02-24-13-17-55-409.mp4

priyeshshah11 · 2023-02-24T06:32:32Z

@thesahindia @priyeshshah11 I don't encounter the Android keyboard not opening issue mentioned above, on my physical Android device

@tienifr can you please try in a new chat/empty room like @thesahindia's video?

tienifr · 2023-02-24T06:40:42Z

But I suspect the issue on your device might be due to the timeout set here

App/src/pages/home/report/ReportActionCompose.js

Line 280 in c041d81

setTimeout(() => this.textInput.focus(), 100);

It might be too low for some scenarios and it's been increased before (like in here) to make sure the timeout is enough.

@thesahindia can you try increasing that timeout value to see if the issue still persists?

tienifr · 2023-02-24T06:48:54Z

@priyeshshah11 I tried again just now on a physical device on the same announce chat which is empty, could not see the issue either.

Screenrecorder-2023-02-24-13-44-29-1.mp4

priyeshshah11 · 2023-02-24T06:51:07Z

Thanks @tienifr, Your comment above makes sense & looks like a potential cause.

thesahindia · 2023-02-24T12:39:02Z

I tested this on two physical android devices. I had to increase the delay to 250 for one device and 400 for the other and I was still able to repro the issue after 2-3 tries (at chats with long message history). With emoji picker modal I didn't experience any problem, so I think we should check why the same issue can't be reproduced when we close emoji picker modal.

tienifr · 2023-02-24T16:51:34Z

src/components/AttachmentPicker/index.native.js

        });
    }

    render() {
        return (
            <>
                <Popover
-                    onClose={this.close}
+                    onClose={() => {


@priyeshshah11 We're using onClose here to trigger the focus which will face the same issue on Android where it fails to open the keyboard. We might want to use use onModalHide here instead.

…keyboard-flash-on-add-attachment

…nto fix-keyboard-flash-on-add-attachment" This reverts commit f00881b.

Julesssss · 2023-04-28T15:28:25Z

Build in progress

…al dismiss

rushatgabhane · 2023-04-28T18:17:05Z

Keyboard doesn't reopen on Pixel 6.

screen-20230428-234356_2.mp4

priyeshshah11 · 2023-05-01T15:34:18Z

Ok so my conclusion is that this is an issue with the react native modal lib where it calls the onModalHide callback too soon for which there are already several issues without any clear solution.
react-native-modal/react-native-modal#534
react-native-modal/react-native-modal#484 (comment) #2719

We have seen this same issue multiple times before in our repo itself, again without any permanent solution. However, @s77rt has this PR open for almost 4 months now without any activity from the maintainers or on the repo itself since almost a year now.

I believe we have 2 options to move forward:

To wait for @s77rt's PR to be merged & holding this PR until then.
Or to proceed with the setTimeout fix just like how we have did in the past due to the dead ends.

cc: @trjExpensify @thesahindia

thesahindia · 2023-05-01T15:34:34Z

@rushatgabhane, thanks for the testing!

thesahindia · 2023-05-01T15:36:59Z

Ok so my conclusion is that this is an issue with the react native modal lib where it calls the onModalHide callback too soon for which there are already several issues without any clear solution.
react-native-modal/react-native-modal#534
react-native-modal/react-native-modal#484 (comment) #2719

Thanks for the links! I will check this in few hours. After completing other pending tasks

s77rt · 2023-05-01T16:41:01Z

Hi, this PR react-native-modal/react-native-modal#733 is mostly for Web. I don't know if it will fix the bug on Native too.

priyeshshah11 · 2023-05-01T16:44:45Z

Hi, this PR react-native-modal/react-native-modal#733 is mostly for Web. I don't know if it will fix the bug on Native too.

any particular reason why it might not if it's kind of the same issue? the solution doesn't really seem platform specific.

I do understand that they all behave differently under the hood but just asking if there are any obvious reasons.

s77rt · 2023-05-01T16:59:02Z

@priyeshshah11 The solution was based on another root cause which is the focus trap that is enforced by <Modal /> on RNW. The solution may or may not fix the bug on Native.

thesahindia · 2023-05-03T15:01:09Z

The solution may or may not fix the bug on Native.

@tienifr @priyeshshah11 can you guys check if the fix will solve our problem?

priyeshshah11 · 2023-05-03T17:08:24Z

The solution may or may not fix the bug on Native.

@tienifr @priyeshshah11 can you guys check if the fix will solve our problem?

I don't think we will be able to as neither of us can reproduce that bug but I feel it will fix it for native if it fixes for web but I can't say for sure.

joelbettner · 2023-05-11T20:24:52Z

How are things going here @priyeshshah11 @thesahindia @tienifr?

I don't think we will be able to as neither of us can reproduce that bug but I feel it will fix it for native if it fixes for web but I can't say for sure.

Do we have a solution that we know works for most platforms, but are just unable to test/reproduce it on certain platforms? If that is the case, I think we should go with what we have if we know it fixes it for some platforms. Your thoughts @trjExpensify , @Julesssss?

priyeshshah11 · 2023-05-12T10:06:45Z

Do we have a solution that we know works for most platforms, but are just unable to test/reproduce it on certain platforms? If that is the case, I think we should go with what we have if we know it fixes it for some platforms. Your thoughts @trjExpensify , @Julesssss?

@joelbettner The current code of this PR fixes the main bug but causes a minor regression on some android devices where it doesn't open the keyboard after dismissing a modal (it does focus the TextInput but just doesn't open the keyboard). I do have a fix for this, which is to use a setTimeout without any delay for the focus/keyboard opening code and it is not a "workaround".

I know no one likes setTimeout here but this might be a good use case for it. Using setTimeout without any delay means that it executes the callback immediately as soon as the JS thread is not busy or is free. This SO might help understand this better.

Also, I would really like to understand exactly what's wrong in using setTimeout here?

cc: @thesahindia @trjExpensify

thesahindia · 2023-05-16T11:59:50Z

Our approach is to address the problem at its core, which is why we refrain from using setTimeout. In this case we are already using setTimeout with a delay of 100ms which isn't working so we should definitely research more into it instead of adding another setTimeout.

I was looking into #9252, it's a similar issue (not the same though), and looks like it will be fixed by #11684. So, I think we should wait for #11684 and test this again.

priyeshshah11 · 2023-05-17T15:50:43Z

Our approach is to address the problem at its core, which is why we refrain from using setTimeout. In this case we are already using setTimeout with a delay of 100ms which isn't working so we should definitely research more into it instead of adding another setTimeout.

I understand that & I have already given more than sufficient explanation for the cause & the fix several times above & in the issue. @joelbettner let me know if the explanation above makes sense?

I was looking into #9252, it's a similar issue (not the same though), and looks like it will be fixed by #11684. So, I think we should wait for #11684 and test this again.

well, we have been waiting for far too long on this issue expecting other things to fix it or to try & find the perfect solution even when we have a 99% working solution & we are holding this PR for a seperate known bug that has appeared in several other parts of the app as linked above. I don't think that's fair from a contributor's POV as we don't have unlimited time to be spent on each issue, this should also been taken into consideration.

cc: @trjExpensify

Julesssss · 2023-07-11T08:29:53Z

Hey @priyeshshah11, apologies but I think we dropped this one. QA tested the build again yesterday but it looks like the PR is out of date. If you can resolve the conflicts I'll generate a new build for QA to verify and we can hopefully get this merged.

…keyboard-flash-on-add-attachment

OSBotify · 2023-07-11T16:43:07Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/15337/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/15337/index.html

desktop 💻	web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/15337/NewExpensify.dmg	https://15337.pr-testing.expensify.com

joelbettner · 2023-11-13T19:30:18Z

I'm going to close this for now. We can re-open in the future if needed.

Fix keyboard issues in add attachment flow

44ff6f6

priyeshshah11 requested a review from a team as a code owner February 22, 2023 01:30

melvin-bot bot requested review from joelbettner and thesahindia and removed request for a team February 22, 2023 01:30

priyeshshah11 mentioned this pull request Feb 22, 2023

[HOLD for payment 2023-09-04] [$4000] Keyboard shows again briefly when clicking "Add attachment" while the composer is focused #13922

Closed

2 tasks

tienifr reviewed Feb 22, 2023

View reviewed changes

priyeshshah11 added 2 commits February 22, 2023 19:38

fixed refocusing on first popover close bug & mweb refocus after dism…

17d37d7

…issing native picker once & DRY refactor

Added the blur to fix android keyboard not opening issue

7a9ee23

tienifr reviewed Feb 24, 2023

View reviewed changes

priyeshshah11 added 3 commits February 25, 2023 05:36

fixed android keyboard not opening bug

7771fb0

Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…

f00881b

…keyboard-flash-on-add-attachment

Revert "Merge branch 'main' of https://github.com/priyeshshah11/App i…

cdb9402

…nto fix-keyboard-flash-on-add-attachment" This reverts commit f00881b.

Julesssss added Ready To Build and removed Ready To Build labels Apr 28, 2023

removing the setTimeout hack that was added to reopen keyboard on mod…

997253f

…al dismiss

Julesssss added Ready To Build and removed Ready To Build labels Apr 28, 2023

s77rt mentioned this pull request Jun 30, 2023

[$1000] iOS - Split bill - Keyboard displayed abruptly when split bill animation slides in #21394

Closed

6 tasks

priyeshshah11 added 5 commits July 12, 2023 00:58

Merge branch 'main' of https://github.com/priyeshshah11/App into fix-…

892bec9

…keyboard-flash-on-add-attachment

fixed merge issues

8d66be0

fixed merge issues

8da22e0

fixed lint issues

ed3a666

fixed prettier issues

517e9a9

ntdiary mentioned this pull request Aug 16, 2023

fix keyboard flashing while clicking "Add attachment" #23994

Merged

58 tasks

joelbettner closed this Nov 13, 2023

Fix keyboard issues in add attachment flow #15337

Fix keyboard issues in add attachment flow #15337

Conversation

priyeshshah11 commented Feb 22, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 22, 2023

tienifr Feb 22, 2023

Choose a reason for hiding this comment

priyeshshah11 commented Feb 23, 2023

thesahindia commented Feb 23, 2023

priyeshshah11 commented Feb 23, 2023

thesahindia commented Feb 23, 2023

priyeshshah11 commented Feb 23, 2023

priyeshshah11 commented Feb 23, 2023

thesahindia commented Feb 23, 2023

priyeshshah11 commented Feb 23, 2023

thesahindia commented Feb 23, 2023

priyeshshah11 commented Feb 23, 2023 • edited Loading

tienifr commented Feb 24, 2023

priyeshshah11 commented Feb 24, 2023

tienifr commented Feb 24, 2023

tienifr commented Feb 24, 2023

priyeshshah11 commented Feb 24, 2023

thesahindia commented Feb 24, 2023

tienifr Feb 24, 2023

Choose a reason for hiding this comment

Julesssss commented Apr 28, 2023

rushatgabhane commented Apr 28, 2023

priyeshshah11 commented May 1, 2023

thesahindia commented May 1, 2023 • edited Loading

thesahindia commented May 1, 2023

s77rt commented May 1, 2023

priyeshshah11 commented May 1, 2023 • edited Loading

s77rt commented May 1, 2023

thesahindia commented May 3, 2023

priyeshshah11 commented May 3, 2023

joelbettner commented May 11, 2023

priyeshshah11 commented May 12, 2023

thesahindia commented May 16, 2023

priyeshshah11 commented May 17, 2023

Julesssss commented Jul 11, 2023

OSBotify commented Jul 11, 2023

joelbettner commented Nov 13, 2023

priyeshshah11 commented Feb 22, 2023 •

edited

Loading

priyeshshah11 commented Feb 23, 2023 •

edited

Loading

thesahindia commented May 1, 2023 •

edited

Loading

priyeshshah11 commented May 1, 2023 •

edited

Loading