[No QA] fixed keyboard appears on split bill page #2022

parasharrajat · 2021-03-23T21:33:30Z

Please review.

Details

ReportActionCompose component has Controlled focus functionality. So when we close the modal we trigger the focus over the Keyboard on ReportActiveCompose. This behaviour was causing an issue when we navigate to the Split Bill page or possibly any other screen which we open from this button's modal.

Added a generic HOC component.

Fixed Issues

Fixes #1855 & Fixes #2377

Tests/ QA steps

we need to enable the Split bill page first. for that replace the CreateMenu's menuItems with the following on line L252.

          menuItems={[
                                                    {
                                                        icon: Paperclip,
                                                        text: 'Upload Photo',
                                                        onSelected: () => {
                                                            openPicker({
                                                                onPicked: (file) => {
                                                                    displayFileInModal({file});
                                                                },
                                                            });
                                                        },
                                                    },
                                                    {
                                                        icon: Paperclip,
                                                        text: 'Split Bill',
                                                        onSelected: () => {
                                                            Navigation.navigate(ROUTES.IOU_BILL);
                                                        },
                                                    },
                                                ]}

Open any chat and then open the Split Bill page from the icon's modal. The keyboard should not open as the Split bill page has an onscreen keyboard.

Tested On

Android

Screenshots

Android

1616533120290.mp4

Julesssss

Looking good., but is there a reason you didn't end up going with this solution?

My understanding was that it would replace the need for us to handle the focus/blur lifecycle. But please correct me if I'm wrong here.

parasharrajat · 2021-03-25T20:25:08Z

@Julesssss. It was not working. Navigation.isDrawerOpen is only helpful when we have only a direct modal. But it involved multiple modals. The first modal would open when you press + icon. then on clicking split bill closes this modal. So the Navigation.isDrawerOpen check will allow the keyboard to open and then the Split bill modal will open. On the other hand, screen focus is working well. As we can't use the Hooks, thus we have to work with event listeners

nickmurray47

had one small comment, looks good otherwise

src/components/withNavigationContext.js

…asharrajat/keyboard-fix

parasharrajat · 2021-03-25T21:24:17Z

@nickmurray47 @Julesssss Updated.

nickmurray47

LGTM - will leave final review / merging up to @Julesssss

src/components/withNavigationContext.js

Julesssss

Looks good to me, but I'm going to ask for one more reviewer.

Julesssss · 2021-03-26T14:22:17Z

Requested a review from someone more experienced with React Native

marcaaron

Not blocking this PR. But I'm concerned this pattern will add complexity we don't need and overall feels like there might be a better way to solve this issue.

There are really only a handful of cases where we will want to focus the report comment input. Rather than add another check to see if we are focused on the report page it would be good to try to come up with some solution to explicitly focus the input based on actions the user is taking e.g. navigating to a report, closing the attachment modal etc.

I think if we can figure out how to do this... then there'd be no need to track screen focus or whether modals have "just been hidden" etc.

parasharrajat · 2021-03-26T17:41:42Z

@marcaaron Exactly. I was thinking that keyboard focus has many issues and this solution is limited to a small scope. We should only focus on special cases. Let me see into this.

Few issues :

Navigating between chats keyboard remains open.
When we scroll up and see an older message and open its context menu and do some action which opens a modal after modal is closed the keyboards opens again which should not happen.

It would be helpful if I can get a list of actions when we have to explicitly set the focus.
Thanks.

parasharrajat · 2021-03-26T17:45:27Z

@Julesssss @nickmurray47 Thanks for the review. But I think we can keep this on hold.

It would be helpful if I can get a list of actions when we have to explicitly set the focus.

I would like to mold the PR in this direction.

marcaaron · 2021-03-26T17:53:08Z

I think the cases are:

When we are navigating to a report/chat from some other route
When the drawer has opened from the LHN to the report
When we are closing an "attachment modal"

waiting for new implementation

trjExpensify · 2021-04-01T22:24:08Z

Hey guys, can we get a quick update on where we're at with this please?

parasharrajat · 2021-04-01T22:40:22Z

@trjExpensify. I am looking at other options to manage the focus on the Composer. There is a lot of discussion going around this issue. There are some issues #2100, #2154 which could change the behavior of the composer so I was waiting to have some update on that. I also saw that It was suggested that we should not focus on the Composer at all when we open the report from LHN.
Thus, I am taking some time before I put some logic in place. But I will soon share some Logical update.

parasharrajat · 2021-04-13T01:28:40Z

@marcaaron Updated. Let me know your thoughts. And keyboard dismiss issue is gone after adding loader for report screen.

parasharrajat · 2021-04-13T19:06:56Z

@marcaaron I have pushed the new changes and there are a few lint errors which I will resolve after your review.

marcaaron

Working well but have a few clean up requests. Thanks!

src/components/withNavigationContext.js

src/pages/home/report/ReportActionCompose.js

marcaaron · 2021-04-13T20:23:43Z

src/pages/home/report/ReportActionCompose.js

-            this.textInput.focus();
+            InteractionManager.runAfterInteractions(() => {
+                this.textInput.focus();
+            });


I'm confused about what issue this is addressing. Can we add some comments to explain what interactions we are waiting to finish?

Animations do not run smoothly, as a side effect of the focus. So this case should suffice when the modal closes. It's a precautionary check that's all. Not particularly needed for these changes.

Cool whatever it is, please add an explanation so we have some insights when looking at the code. Out of curiositym is there a particular modal closing that causes jank? Maybe we can add a test to verify it does not look janky?

cc @Julesssss We should also maybe update these test steps so that it is testable by QA?

So, the Most effect of focus interfering with animation can be seen on a new chat or new group screen where we set the focus on Search input. Here, we may need this on the web when we close modals and then set focus back to Input but it's not apparent. Due to the similar effect happening on New Chat modals, I thought it's better to apply Interactionmanager on all the places where we are setting focus.

So, the Most effect of focus interfering with animation can be seen on a new chat or new group screen where we set the focus on Search input.

But these are due to the autoFocus. The logic here would be applied for manual/programmatic focuses only
If you want to make auto focus wait for interactions as well you'll need to remove the autoFocus prop and call this.focus() when the component mount

I thought it's better to apply Interactionmanager on all the places where we are setting focus.

This applies it only here on compose though right? Opening a new chat or search use the TextInputFocusable
Instead of adding this logic to all the places that set focus couldn't it perhaps be added to TextInputFocusable.
TextInputFocusable can wrap inputRef.focus with this behavior (before exposing it to parents)

Since focusing is done inside a callback, is it guaranteed you will always be inside a chat when this callback is invoked? Could it be possible for the user to tap/swipe too fast, open a chat and go back to LHN or somewhere else and then this callback runs and focuses the field?

if (this.textInput) - textInput - could be defined before setting this interaction, but could have changed inside the actual callback

you will want to abort this interaction in case the component is unmounted but the callback is still scheduled to run. Similar to how you would clean up timers when a component unmounts

focus() { this.scheduledIntercation = Interactionmanager.runAfterInteractions(() => {...}) } componentWillUnmount() { if(this.scheduledIntercation) this.scheduledIntercation.cancel() }

I think this works great, just that it should have been reusable/configurable for the other input fields as well

marcaaron · 2021-04-13T21:03:53Z

Open any chat and then open the Split Bill page from the image icon's modal. The keyboard should not open as the Split bill page has an onscreen keyboard.

Not seeing how this can be done.

…asharrajat/keyboard-fix

parasharrajat · 2021-04-13T21:52:16Z

@marcaaron Updated the QA steps.

parasharrajat · 2021-04-13T21:59:44Z

Code Updated. Thanks.

marcaaron

Looks great thanks!

OSBotify · 2021-04-13T22:38:29Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

kidroca · 2021-04-14T08:14:55Z

src/pages/home/report/ReportActionCompose.js

-            this.textInput.focus();
+            InteractionManager.runAfterInteractions(() => {
+                this.textInput.focus();
+            });


So, the Most effect of focus interfering with animation can be seen on a new chat or new group screen where we set the focus on Search input.

But these are due to the autoFocus. The logic here would be applied for manual/programmatic focuses only
If you want to make auto focus wait for interactions as well you'll need to remove the autoFocus prop and call this.focus() when the component mount

I thought it's better to apply Interactionmanager on all the places where we are setting focus.

This applies it only here on compose though right? Opening a new chat or search use the TextInputFocusable
Instead of adding this logic to all the places that set focus couldn't it perhaps be added to TextInputFocusable.
TextInputFocusable can wrap inputRef.focus with this behavior (before exposing it to parents)

Since focusing is done inside a callback, is it guaranteed you will always be inside a chat when this callback is invoked? Could it be possible for the user to tap/swipe too fast, open a chat and go back to LHN or somewhere else and then this callback runs and focuses the field?

if (this.textInput) - textInput - could be defined before setting this interaction, but could have changed inside the actual callback

you will want to abort this interaction in case the component is unmounted but the callback is still scheduled to run. Similar to how you would clean up timers when a component unmounts

focus() { this.scheduledIntercation = Interactionmanager.runAfterInteractions(() => {...}) } componentWillUnmount() { if(this.scheduledIntercation) this.scheduledIntercation.cancel() }

I think this works great, just that it should have been reusable/configurable for the other input fields as well

kidroca · 2021-04-14T08:19:13Z

src/pages/home/report/ReportActionCompose.js

+        // open creates a jarring and broken UX.
+        if (this.shouldFocusInputOnScreenFocus && this.props.isFocused
+            && prevProps.modal.isVisible && !this.props.modal.isVisible) {
+            this.setIsFocused(true);


setIsFocused(true) would be called when the field is focused anyway.
Though if there is a legit reason to call it here, the call can just be moved inside the focus method

this.setIsFocused(true); this.focus();

vs

this.focus();

Nice thoughts. Yeah Cancellation of Interaction is good to have.

But about autofocus, I think there are some PR on react native that are handling animation and focus issue. But I agree lets just move the interaction based focus to TextInputFocusable. I didn't do this here as that covers bigger scope then the current issue. So its better to first analyze the effect of these changes and consider them.

isagoico · 2021-04-14T19:08:24Z

@marcaaron @Julesssss @nickmurray47 I don't think we're able to QA this PR since it requires changing some code. Can you guys assign someone to QA and let me know once it's done so we can check it off the list? Thanks in advance.

Julesssss · 2021-04-15T10:34:57Z

Just tested this and noticed that while the Keyboard doesn't display when opening a Modal, adding an attachment seems to have broke (iOS only, Android works fine) (if this is a regression from another issue please correct me).

Open a chat report on mobile
Tap the '+' icon to add an attachment
The Keyboard displays on top of the CreateMenu

RPReplay_Final1618482151.MP4

parasharrajat · 2021-04-15T10:40:40Z

@Julesssss as per the changes here, we have removed automatic opening of keyboard on Native devices. So autofocus should not happen at all. I think this is regression from another PR

#2022 (comment)

kidroca · 2021-04-15T10:52:25Z

@Julesssss @parasharrajat
This is a leftover from the modal to modal fix

https://github.com/Expensify/Expensify.cash/blob/29fe119755ab887edf010d38565b162f3269de6f/src/pages/home/report/ReportActionCompose.js#L232-L243

Due to the modal handling at the time (autofocusing) it was necessary to add this. Otherwise the 2nd modal would disappear immediately

The decision to drop autofocus happened a week or more later.
The focus there should be removed as well

kidroca · 2021-04-15T10:56:58Z

Previously with the autofocus behavior something similar would happen (again only on iOS)

When the input is not focused

The first modal closes and triggers focus from componentDidUpdate
As the 2nd modal opens the input is focused and this is treated as interaction that dismissed the 2nd modal
This wasn't happening when the field was already focused

parasharrajat · 2021-04-15T11:18:46Z

I see. I will patch in another PR for that but its not a regression of this one so no need to reopen this.

kidroca · 2021-04-15T11:54:07Z

@parasharrajat @Julesssss
It turns out there's more to this actually

The fix above resolves the issue - see the first part of the video below
But on iOS it looks like focus is restored to the input anyway. So when the input was focused and you go through the attachment flow. the keyboard pops up briefly in between the modals - second part of the video

Removing the focus line

Screen.Recording.2021-04-15.at.14.35.47.mov

This is why I originally had a this.blur function - I had disabled the autofocus to see how stuff would work and stumbled on this behavior.
My fix was to blur the input when you tap on the (+) button, because otherwise focus would try to return the input, though this is iOS specific as it's not caused by our code.

Replacing with blur

Screen.Recording.2021-04-15.at.14.49.04.mov

Replacing with blur seem to work better. As far as I tested back then it didn't cause any issues on the other mobile platforms

If you want to I can open another PR with these changes

Julesssss · 2021-04-15T12:02:06Z

Thanks @kidroca, that sounds good to me but I think it's best to await confirmation from @marcaaron in case this has wider repercussions (I'm unable to look any further into this myself currently).

marcaaron · 2021-04-15T17:56:38Z

Tbh I'm having some trouble following the discussion. Maybe we should create a new issue as a next step so we can evaluate what is happening and what to do about it and who will do it?

github-actions · 2021-04-15T17:56:54Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

OSBotify · 2021-04-19T22:41:56Z

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2021-04-23T00:46:05Z

@parasharrajat, it seems like this PR cannot be tested without some code modifications, is that correct? If that's the case, this is not going to be testable on staging (at least not until the IOU beta is set up). Given that this page isn't visible on staging or production yet, I'm going to check it off in the deploy checklist.

For future reference, when pull requests are not possible to QA on staging, the title of the issue should be prefixed with [No QA], and the "QA Steps" section of the pull request template should say "None".

parasharrajat · 2021-04-23T00:58:22Z

@roryabraham There is twist for this PR. Initially we started fixing the issue for Split bill page but then moved to manage the focus on the report action composer.

To test it we can just switch the chat screen and drawer or open the modals and then close them.

We have to check that the focus is not set automatically to the composer on mobile devices but it is set on the web and desktop.

This eventually fix the clre issue linked to the PR.

So it is testable but the instructions written on the description are for linked issue.

fix: keyboard appears on split bill page

170f5f3

parasharrajat requested a review from a team as a code owner March 23, 2021 21:33

botify requested review from nickmurray47 and removed request for a team March 23, 2021 21:33

typo fixed

6e40e39

nickmurray47 assigned parasharrajat Mar 23, 2021

Julesssss self-requested a review March 25, 2021 18:05

Julesssss reviewed Mar 25, 2021

View reviewed changes

nickmurray47 suggested changes Mar 25, 2021

View reviewed changes

src/components/withNavigationContext.js Outdated Show resolved Hide resolved

parasharrajat added 2 commits March 26, 2021 02:38

Merge branch 'master' of github.com:Expensify/Expensify.cash into par…

780f0d2

…asharrajat/keyboard-fix

refactor

c4f64e5

parasharrajat requested a review from nickmurray47 March 25, 2021 21:22

nickmurray47 previously approved these changes Mar 25, 2021

View reviewed changes

Julesssss reviewed Mar 26, 2021

View reviewed changes

src/components/withNavigationContext.js Outdated Show resolved Hide resolved

Julesssss reviewed Mar 26, 2021

View reviewed changes

src/components/withNavigationContext.js Outdated Show resolved Hide resolved

Julesssss previously approved these changes Mar 26, 2021

View reviewed changes

marcaaron reviewed Mar 26, 2021

View reviewed changes

marcaaron mentioned this pull request Mar 26, 2021

Focus compose box when navigate away from RHN #2050

Merged

5 tasks

This was referenced Mar 28, 2021

[Discussion] ReportView and ReportActionCompose enhancements #2129

Closed

Fix: Modal closes immediately after launch #2114

Merged

marcaaron requested changes Apr 13, 2021

View reviewed changes

marcaaron requested a review from Julesssss April 13, 2021 20:24

Merge branch 'master' of github.com:Expensify/Expensify.cash into par…

e88cf38

…asharrajat/keyboard-fix

parasharrajat mentioned this pull request Apr 13, 2021

Conversation - Tapping on "Upload Photo" will quickly flash keyboard #2377

Closed

refactor the Composer focus logic.

80411e8

marcaaron approved these changes Apr 13, 2021

View reviewed changes

marcaaron merged commit 7a0e16b into Expensify:master Apr 13, 2021

OSBotify mentioned this pull request Apr 13, 2021

Deploy Checklist: Expensify.cash 2021-04-13 #2380

Closed

66 tasks

kidroca reviewed Apr 14, 2021

View reviewed changes

parasharrajat changed the title ~~fixed keyboard appears on split bill page~~ [No QA] fixed keyboard appears on split bill page Apr 23, 2021

bondydaa mentioned this pull request Dec 13, 2022

[$1000] [HOLD for payment 2023-02-08] mWeb/Chrome - Focus is lost and the keyboard does not reopen after pressing "+" button #13443

Closed

[No QA] fixed keyboard appears on split bill page #2022

[No QA] fixed keyboard appears on split bill page #2022

Conversation

parasharrajat commented Mar 23, 2021 • edited Loading

Details

Fixed Issues

Tests/ QA steps

Tested On

Screenshots

Android

Julesssss left a comment

Choose a reason for hiding this comment

parasharrajat commented Mar 25, 2021 • edited Loading

nickmurray47 left a comment

Choose a reason for hiding this comment

parasharrajat commented Mar 25, 2021

nickmurray47 left a comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented Mar 26, 2021

marcaaron left a comment

Choose a reason for hiding this comment

parasharrajat commented Mar 26, 2021

parasharrajat commented Mar 26, 2021 • edited Loading

marcaaron commented Mar 26, 2021

trjExpensify commented Apr 1, 2021

parasharrajat commented Apr 1, 2021

parasharrajat commented Apr 13, 2021

parasharrajat commented Apr 13, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

parasharrajat Apr 13, 2021

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

parasharrajat Apr 13, 2021

Choose a reason for hiding this comment

kidroca Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

marcaaron commented Apr 13, 2021

parasharrajat commented Apr 13, 2021

parasharrajat commented Apr 13, 2021

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 13, 2021

kidroca Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

kidroca Apr 14, 2021

Choose a reason for hiding this comment

parasharrajat Apr 14, 2021

Choose a reason for hiding this comment

isagoico commented Apr 14, 2021

Julesssss commented Apr 15, 2021

parasharrajat commented Apr 15, 2021 • edited Loading

kidroca commented Apr 15, 2021

kidroca commented Apr 15, 2021 • edited Loading

parasharrajat commented Apr 15, 2021

kidroca commented Apr 15, 2021

Removing the focus line

Replacing with blur

Julesssss commented Apr 15, 2021

marcaaron commented Apr 15, 2021

github-actions bot commented Apr 15, 2021 • edited Loading

OSBotify commented Apr 19, 2021

roryabraham commented Apr 23, 2021

parasharrajat commented Apr 23, 2021 • edited Loading

parasharrajat commented Mar 23, 2021 •

edited

Loading

parasharrajat commented Mar 25, 2021 •

edited

Loading

parasharrajat commented Mar 26, 2021 •

edited

Loading

kidroca Apr 14, 2021 •

edited

Loading

kidroca Apr 14, 2021 •

edited

Loading

parasharrajat commented Apr 15, 2021 •

edited

Loading

kidroca commented Apr 15, 2021 •

edited

Loading

github-actions bot commented Apr 15, 2021 •

edited

Loading

parasharrajat commented Apr 23, 2021 •

edited

Loading