Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

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

Closed kavimuru closed 7 months ago

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open chat
  2. Focus the composer such that the keyboard is open
  3. click plus icon in the composer
  4. click Add Attachment

Expected Result:

They keyboard doesn't reopen briefly, such that it's a smooth animation to the next modal screen with the attachment upload options

Actual Result:

The keyboard shows again briefly before the modal with the attachment upload options appears, resulting in what looks like the screen flickering

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: v1.2.49.0 Reproducible in staging?: Y Reproducible in production?:Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/210187345-d69ad341-e2ed-4952-a881-3192c477451c.MP4

https://user-images.githubusercontent.com/43996225/210187348-8ed66089-a8dd-4807-9227-615800b162cc.mp4

Expensify/Expensify Issue URL: Issue reported by: @grgia Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1672422586959209

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01eb1d86a3237e2dfd
  • Upwork Job ID: 1611147936271515648
  • Last Price Increase: 2023-07-27
  • Automatic offers:
    • situchan | Contributor | 26109669
jasperhuangg commented 1 year ago

Was a bit too quick to assign myself, @kavimuru does this only happen on iOS, or does this also happen on Android/Mobile Web?

I feel like this might be related to the logic here

cc @parasharrajat

kavimuru commented 1 year ago

@jasperhuangg not able to repro in Android and mWeb

https://user-images.githubusercontent.com/43996225/210414107-74d62b0f-3ace-4bfc-9919-12d711de794b.mp4

grgia commented 1 year ago

@kavimuru did you test android with the keyboard open as well?

trjExpensify commented 1 year ago

Hm, what exactly is happening here? We should make the OP as descriptive as we can versus "janky". I can't quite catch what page is opening after the Add attachment button press?

Also, the screen recording above of Android seems to have the same problem of a brief flicker on press doesn't it?

I feel like we've had a similar problem before with the copy to clipboard modal momentarily showing, but it doesn't seem like that's what is showing here.

trjExpensify commented 1 year ago

@jasperhuangg are you taking this one on?

grgia commented 1 year ago

Looks like the keyboard isn't closing when pressed and it's reopening for a single frame which is causing the flashing.

https://user-images.githubusercontent.com/38015950/210866703-6d3b0377-3091-4f37-821b-25dfcb2f6452.MOV

grgia commented 1 year ago

If I scrub the video it's the keyboard during that flash

image

trjExpensify commented 1 year ago

Nice, okay. I've updated the OP and issue title to be more descriptive and reflect the problem at hand. @jasperhuangg, can you let us know today if you want to take this on? If not, let's open it up with the External label.

jasperhuangg commented 1 year ago

Feel free to assign External! Have some customer issues I need to get to this week. Thanks for the bump!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01eb1d86a3237e2dfd

melvin-bot[bot] commented 1 year ago

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @joelbettner (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

priyeshshah11 commented 1 year ago

Proposal

Root cause

This issue occurs because iOS by default dismisses the Keyboard before opening any modals and it remembers the keyboard's state and reopens the keyboard when the modal is dismissed/closed which is ideal for most scenarios. However, this does not work well when you're switching between two modals as iOS tries to reopen the keyboard as soon as you dismiss the first modal and then tries to dismiss the keyboard again as soon as the second modal is opened which causes this "janky" effect.

Solution

I think the ideal scenario here is to dismiss the keyboard ourselves before we open the first modal, that way native code thinks that the keyboard was already closed prior to opening that modal and won't try to reopen & close the keyboard between modal transitions.

The downside of this is that the keyboard won't open back again automatically once the attachment is selected which IMO is not big issue and it comes down to choice. If we still want to open that keyboard back after the attachment being selected, we can do that by remembering the keyboard state ourselves and reopen it if it was open at the start. However, IMO it might be an overkill but happy to hear other's thoughts on it.

Solution 1 - Dismiss keyboard before modal opens

I tried to avoid setTimeout by using InteractionManager but even that doesn't work in this case thus had to resort to setTimeout with 500ms minimum required. The other alternative we can use is the keyboardDidHide event but that involves keeping track of the keyboard state.

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 7811d1c25..89a726099 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -601,11 +602,12 @@ class ReportActionCompose extends React.Component {
                              <TouchableOpacity
                                   ref={el => this.actionButton = el}
                                   onPress={(e) => {
+                                       Keyboard.dismiss();
                                        e.preventDefault();

                                        // Drop focus to avoid blue focus ring.
                                        this.actionButton.blur();
-                                       this.setMenuVisibility(true);
+                                       setTimeout(() => this.setMenuVisibility(true), 500);
                                   }}
                                   style={styles.chatItemAttachButton}
                                   underlayColor={themeColors.componentBG}

Solution 2 - Dismiss keyboard before modal opens & reopen after selecting attachment action is completed

We can achieve this by keeping track of the keyboard state in this component by adding keyboard event listeners and then along with Solution 1 we can reopen the keyboard by focusing the composer input once attachment picker is closed.

I can post a code solution later if there's interest in this solution.

Video

https://user-images.githubusercontent.com/38547776/211009155-315b827a-32e4-425e-bb1d-4555d5ed48c5.mov

@thesahindia @trjExpensify

trjExpensify commented 1 year ago

The downside of this is that the keyboard won't open back again automatically once the attachment is selected which IMO is not big issue and it comes down to choice.

To confirm on this in the first proposed solution.. after the user selects the attachment and clicks Send on the attachment preview to return to the chat view, the composer would not be focused (and thus the keyboard dismissed?). If so, I don't think we want that, as it wouldn't be consistent with sending a message and the focus of the composer remaining.

I'll defer to @thesahindia on an opinion re: the keyboard event listeners as an alternative solution.

thesahindia commented 1 year ago

Actually I wasn't able to repro this issue. It is working fine for me

https://user-images.githubusercontent.com/83179295/211379294-c39900f8-9868-40e5-9069-4a8976f3be25.mov

after the user selects the attachment and clicks Send on the attachment preview to return to the chat view, the composer would not be focused (and thus the keyboard dismissed?)

Yeah agree, the composer should stay focused.

Also I think we should not use setTimeout we try to avoid using it unless it's super necessary.

trjExpensify commented 1 year ago

Actually I wasn't able to repro this issue. It is working fine for me

You need to click Add attachment to navigate to the second modal with the attachment upload options to see the brief flicker of the keyboard as it opens/closes again.

thesahindia commented 1 year ago

You need to click Add attachment to navigate to the second modal with the attachment upload options to see the brief flicker of the keyboard as it opens/closes again.

I was able to repro. Thanks! Let's wait for new proposals.

trjExpensify commented 1 year ago

Cool! Did you have any feedback on the keyboard event listeners alt proposal 2 from the above?

thesahindia commented 1 year ago

Did you have any feedback on the keyboard event listeners alt proposal 2 from the above

We can do that but I think we should wait for a solution without setTimeout.

trjExpensify commented 1 year ago

Oh I see, it still uses setTimeout. Hm yeah, ideally that can be avoided.

trjExpensify commented 1 year ago

@priyeshshah11 any further thoughts on not using setTimeout?

priyeshshah11 commented 1 year ago

@priyeshshah11 any further thoughts on not using setTimeout?

yes I have a potential solution that can listen to keyboardDidHide event & open the modal after that and remember that the keyboard was open & then reopen the keyboard once attachment is sent or the modal is closed. However, I am having some npm issues so can't really test it right now. Will post the code solution once I can test & validate it. Meanwhile, let me know if you have ideas for the issue that I am getting.

priyeshshah11 commented 1 year ago

Proposal Update

This is an update/improvement of my initial proposal here to avoid the use of setTimeout

Solution

This keeps track of the keyboard state prior to dismissing the keyboard & opening modals & will reopen the keyboard once all the modals are closed only if the keyboard was open before the opening the modals. We should also call the reOpenKeyboard function when any of the modals are dismissed and no other modals are opened up. Just trying to test that solution but currently stuck with build errors, will update once it's ready.

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index acd262d2e..361a986b0 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -134,6 +135,7 @@ class ReportActionCompose extends React.Component {
         this.getInputPlaceholder = this.getInputPlaceholder.bind(this);
         this.getIOUOptions = this.getIOUOptions.bind(this);
         this.addAttachment = this.addAttachment.bind(this);
+        this.reOpenKeyboard = this.reOpenKeyboard.bind(this);

         this.comment = props.comment;
         this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();
@@ -150,6 +152,7 @@ class ReportActionCompose extends React.Component {
             },
             maxLines: props.isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES,
             value: props.comment,
+            wasKeyboardOpen: false,

             // If we are on a small width device then don't show last 3 items from conciergePlaceholderOptions
             conciergePlaceholderRandomIndex: _.random(this.props.translate('reportActionCompose.conciergePlaceholderOptions').length - (this.props.isSmallScreenWidth ? 4 : 1)),
@@ -513,6 +516,12 @@ class ReportActionCompose extends React.Component {
         this.props.onSubmit(comment);
     }

+    reOpenKeyboard() {
+        if (this.wasKeyboardOpen) {
+            this.focus();
+        }
+    }
+
     render() {
         // Waiting until ONYX variables are loaded before displaying the component
         if (_.isEmpty(this.props.personalDetails)) {
@@ -551,6 +560,7 @@ class ReportActionCompose extends React.Component {
                 ]}
                 >
                     <AttachmentModal
+                        onModalHide={this.reOpenKeyboard}
                         headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
                         onConfirm={this.addAttachment}
                     >
@@ -607,7 +617,16 @@ class ReportActionCompose extends React.Component {

                                                             // Drop focus to avoid blue focus ring.
                                                             this.actionButton.blur();
-                                                            this.setMenuVisibility(true);
+                                                            if (this.props.isKeyboardShown) {
+                                                                const listener = Keyboard.addListener('keyboardDidHide', () => {
+                                                                    this.setState({wasKeyboardOpen: true});
+                                                                    this.setMenuVisibility(true);
+                                                                    listener.remove();
+                                                                });
+                                                                Keyboard.dismiss();
+                                                            } else {
+                                                                this.setMenuVisibility(true);
+                                                            }
                                                         }}
                                                         style={styles.chatItemAttachButton}
umair-upwork commented 1 year ago

Proposal

Problem

When the composer is focused, the keyboard is set to show. However, after opening a modal, it keeps checking whether the composer is focused or not and does not wait for the modal animation to close. As a result, when the user clicks on "Add attachment," a second modal appears after a delay. During this delay, the keyboard is briefly shown, causing the screen to flicker.

Solution

To fix this issue, we need to use Keyboard.isVisible() and add a keyboardHideListener on the button. This will keep checking the keyboard state and wait until the modal is open or closed. Once the modal state is updated, the keyboard state will be changed accordingly.

@trjExpensify This is the simplest solution without using setTimeout. Modal can be open without focusing on composer We can use Keyboard.isVisible() to accomplish this.

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index acd262d2e..ce6916f90 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -513,6 +514,19 @@ class ReportActionCompose extends React.Component {
         this.props.onSubmit(comment);
     }

+    keyboardListener() {
+        Keyboard.dismiss();
+
+        const keyboardHideListener = Keyboard.addListener('keyboardDidHide', () => {
+            this.setMenuVisibility(true);
+            keyboardHideListener.remove();                                              
+        })
+        
+        if(!Keyboard.isVisible()){
+            this.setMenuVisibility(true)
+        }
+    }
+
     render() {
         // Waiting until ONYX variables are loaded before displaying the component
         if (_.isEmpty(this.props.personalDetails)) {
@@ -607,7 +621,7 @@ class ReportActionCompose extends React.Component {

                                                             // Drop focus to avoid blue focus ring.
                                                             this.actionButton.blur();
-                                                            this.setMenuVisibility(true);
+                                                            this.keyboardListener();                                                        
                                                         }}
                                                         style={styles.chatItemAttachButton}
                                                         disabled={isBlockedFromConcierge || this.props.disabled}

Result

https://user-images.githubusercontent.com/79830748/211814228-7f0607a5-018f-49cb-8900-24064cfa050a.mp4

tienifr commented 1 year ago

Proposal

Problem

On IOS platform, modal opening will prevent keyboard, and when user closes modal, keyboard will be opened again

In https://github.com/Expensify/App/blob/main/src/components/PopoverMenu/index.js we're implementing logic to close modal when user select the item. That why when user select Add attachment button, the modal hide then open immediately => Keyboard is blink

Solution

We should open modal in keyboardDidHide listener if the keyboard is open.

we also dismiss the keyboard when user opens any modal as I tested on many apps (Skype)

I'll fix In https://github.com/Expensify/App/blob/main/src/components/Modal/index.ios.js to affect all other modal on IOS. It's really weird if keyboard state is reset on 1 place, but other places like above solutions


diff --git a/src/components/Modal/index.ios.js b/src/components/Modal/index.ios.js
index 85a95142a3..6180e7a1fc 100644
--- a/src/components/Modal/index.ios.js
+++ b/src/components/Modal/index.ios.js
@@ -1,18 +1,40 @@
-import React from 'react';
+import React, {useEffect, useState} from 'react';
+import {Keyboard} from 'react-native';
+import {compose} from 'underscore';
+import withKeyboardState from '../withKeyboardState';
 import withWindowDimensions from '../withWindowDimensions';
 import BaseModal from './BaseModal';
 import {propTypes, defaultProps} from './modalPropTypes';

-const Modal = props => (
-    <BaseModal
+const Modal = (props) => {
+    const [isVisible, setIsVisible] = useState(false);
+
+    useEffect(() => {
+        if (!props.isVisible) {
+            setIsVisible(false);
+        } else if (props.isKeyboardShown) {
+            const keyboardListener = Keyboard.addListener('keyboardDidHide', () => {
+                setIsVisible(true);
+                keyboardListener.remove();
+            });
+            Keyboard.dismiss();
+        } else {
+            setIsVisible(true);
+        }
+    }, [props.isVisible, props.isKeyboardShown]);
+
+    return (
+        <BaseModal
             // eslint-disable-next-line react/jsx-props-no-spreading
-        {...props}
-    >
-        {props.children}
-    </BaseModal>
-);
+            {...props}
+            isVisible={isVisible}
+        >
+            {props.children}
+        </BaseModal>
+    );
+};

 Modal.propTypes = propTypes;
 Modal.defaultProps = defaultProps;
 Modal.displayName = 'Modal';
-export default withWindowDimensions(Modal);
+export default compose(withWindowDimensions, withKeyboardState)(Modal);

Result

https://user-images.githubusercontent.com/113963320/211972726-230ea0f6-6fe7-42ff-b1a5-fbe7115e4d63.mov

thesahindia commented 1 year ago

The composer should get focused after we close the context menu, the proposals above aren't handling that.

to affect all other modal on IOS. It's really weird if keyboard state is reset on 1 place, but other places like above solutions

Let's fix this at every place by solving this at the root. The proposals needs to be updated.

tienifr commented 1 year ago

@thesahindia I see there're 3 schools of thought here for major messaging apps:

  1. (Examples: WhatsApp, Skype, Telegram) We dismiss the keyboard when any modal is opened. Then the user will need to focus on the input again after closing the modal.

The proposal in https://github.com/Expensify/App/issues/13922#issuecomment-1379780815 satisfies this (and solves this at the root so it applies to all modals).

  1. (Examples: Slack) We dismiss the keyboard when any modal is opened, but will open the keyboard again after the modal is closed. If we do this, a "side effect" is that the keyboard will be opened briefly if 1 modal closes and another modal opens right after it. If we handle it correctly, it will not be "janky" but rather smooth.

In order to make it smooth, besides the proposal in https://github.com/Expensify/App/issues/13922#issuecomment-1379780815, we need to store the save focused element (ref) in a global variable so that after any modal is closed, we refocus on that ref to open the keyboard. I can post the implementation for this if it's agreed this 2nd way is acceptable.

  1. (Examples: Messenger, Instagram) Same as 2, but with no side effect since there's no 1 modal opening right after another in the app. We cannot do the same since it requires a redesign of our app.

So 3 for us is impossible. We might need to compromise by choosing either 1 or 2 since I don't think there's any way to overcome that (those examples are quite major apps so I think they tried too).

priyeshshah11 commented 1 year ago

Proposal Update from here

Solution

This solves the issue of reopening the keyboard if it was already open before opening the modals. It reopens the keyboard in all possible scenarios where needed.

Obviously the code can be cleaned up a bit during the PR process.

diff --git a/src/components/AttachmentPicker/index.native.js b/src/components/AttachmentPicker/index.native.js
index 6944f75fd..fcbf88377 100644
--- a/src/components/AttachmentPicker/index.native.js
+++ b/src/components/AttachmentPicker/index.native.js
@@ -102,6 +102,7 @@ class AttachmentPicker extends Component {

         this.state = {
             isVisible: false,
+            onClose: () => {},
         };

         this.menuItemData = [
@@ -192,6 +193,7 @@ class AttachmentPicker extends Component {
             imagePickerFunc(getImagePickerOptions(this.props.type), (response) => {
                 if (response.didCancel) {
                     // When the user cancelled resolve with no attachment
+                    this.state.onClose();
                     return resolve();
                 }
                 if (response.errorCode) {
@@ -241,6 +243,7 @@ class AttachmentPicker extends Component {
     showDocumentPicker() {
         return RNDocumentPicker.pick(documentPickerOptions).catch((error) => {
             if (RNDocumentPicker.isCancel(error)) {
+                this.state.onClose();
                 return;
             }

@@ -303,7 +306,12 @@ class AttachmentPicker extends Component {
       */
     renderChildren() {
         return this.props.children({
-            openPicker: ({onPicked}) => this.open(onPicked),
+            openPicker: ({onPicked, onClose}) => {
+                this.open(onPicked);
+                if (onClose) {
+                    this.setState({onClose});
+                }
+            },
         });
     }

@@ -311,7 +319,10 @@ class AttachmentPicker extends Component {
         return (
             <>
                 <Popover
-                    onClose={this.close}
+                    onClose={() => {
+                        this.close();
+                        this.state.onClose();
+                    }}
                     isVisible={this.state.isVisible}
                     anchorPosition={styles.createMenuPosition}
                     onModalHide={this.onModalHide}

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index f88ea64b4..08b563ef4 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -134,6 +135,7 @@ class ReportActionCompose extends React.Component {
         this.getInputPlaceholder = this.getInputPlaceholder.bind(this);
         this.getIOUOptions = this.getIOUOptions.bind(this);
         this.addAttachment = this.addAttachment.bind(this);
+        this.reOpenKeyboard = this.reOpenKeyboard.bind(this);

         this.comment = props.comment;
         this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();
@@ -150,6 +152,7 @@ class ReportActionCompose extends React.Component {
             },
             maxLines: props.isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES,
             value: props.comment,
+            wasKeyboardOpen: false,

             // If we are on a small width device then don't show last 3 items from conciergePlaceholderOptions
             conciergePlaceholderRandomIndex: _.random(this.props.translate('reportActionCompose.conciergePlaceholderOptions').length - (this.props.isSmallSc
reenWidth ? 4 : 1)),
@@ -513,6 +516,14 @@ class ReportActionCompose extends React.Component {
         this.props.onSubmit(comment);
     }

+    reOpenKeyboard() {
+        if (!this.state.wasKeyboardOpen) { return; }
+        InteractionManager.runAfterInteractions(() => {
+            this.textInput.focus();
+            this.setState({wasKeyboardOpen: false});
+        });
+    }
+
     render() {
         // Waiting until ONYX variables are loaded before displaying the component
         if (_.isEmpty(this.props.personalDetails)) {
@@ -551,6 +562,7 @@ class ReportActionCompose extends React.Component {
                 ]}
                 >
                     <AttachmentModal
+                        onModalHide={this.reOpenKeyboard}
                         headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
                         onConfirm={this.addAttachment}
                     >
@@ -608,7 +620,16 @@ class ReportActionCompose extends React.Component {

                                                                 // Drop focus to avoid blue focus ring.
                                                                 this.actionButton.blur();
-                                                                this.setMenuVisibility(true);
+                                                                if (this.props.isKeyboardShown) {
+                                                                    const listener = Keyboard.addListener('keyboardDidHide', () => {
+                                                                        this.setState({wasKeyboardOpen: true});
+                                                                        this.setMenuVisibility(true);
+                                                                        listener.remove();
+                                                                    });
+                                                                    Keyboard.dismiss();
+                                                                } else {
+                                                                    this.setMenuVisibility(true);
+                                                                }
                                                             }}
                                                             style={styles.chatItemAttachButton}
                                                             disabled={isBlockedFromConcierge || this.props.disabled}
@@ -621,7 +642,10 @@ class ReportActionCompose extends React.Component {
                                             <PopoverMenu
                                                 animationInTiming={CONST.ANIMATION_IN_TIMING}
                                                 isVisible={this.state.isMenuVisible}
-                                                onClose={() => this.setMenuVisibility(false)}
+                                                onClose={() => {
+                                                    this.setMenuVisibility(false);
+                                                    this.reOpenKeyboard();
+                                                }}
                                                 onItemSelected={() => this.setMenuVisibility(false)}
                                                 anchorPosition={styles.createMenuPositionReportActionCompose}
                                                 menuItems={[...this.getIOUOptions(reportParticipants),
@@ -631,6 +655,7 @@ class ReportActionCompose extends React.Component {
                                                         onSelected: () => {
                                                             openPicker({
                                                                 onPicked: displayFileInModal,
+                                                                onClose: () => this.reOpenKeyboard(),
                                                             });
                                                         },
                                                     },
priyeshshah11 commented 1 year ago

@thesahindia @trjExpensify Attaching videos for the above proposal. Had to break it down into 3 parts to cover most scenarios

Part 1

https://user-images.githubusercontent.com/38547776/212392464-17469812-3b58-4173-a6b8-09c5b3f8e196.mov

Part 2

https://user-images.githubusercontent.com/38547776/212392493-c9bf1ff6-e8f5-479d-9b10-085f37f6432f.mov

Part 3

https://user-images.githubusercontent.com/38547776/212392662-748ab2a2-6f4b-41d4-8189-dbf71788be5a.mov

trjExpensify commented 1 year ago

@thesahindia thoughts on this?

umair-upwork commented 1 year ago

Updated Proposal

The composer should get focused after we close the context menu, the proposals above aren't handling that.

to affect all other modal on IOS. It's really weird if keyboard state is reset on 1 place, but other places like above solutions

Let's fix this at every place by solving this at the root. The proposals needs to be updated.

We can set focus to the composer after closing the modal by passing a function as a prop to openPicker in PopoverMenu. This function will be called after the user closes the addAttachment modal or select document and then composer will get focused.

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index acd262d2e..2e1f4b3b1 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -513,6 +514,22 @@ class ReportActionCompose extends React.Component {
         this.props.onSubmit(comment);
     }

+    keyboardListener() {
+        this.textInput.blur()
+        
+        const keyboardHideListener = Keyboard.addListener('keyboardDidHide', () => {
+            this.setMenuVisibility(true)
+            keyboardHideListener.remove()
+        })
+
+        if(!Keyboard.isVisible()){
+            this.setMenuVisibility(true)
+        }else {
+            this.setMenuVisibility(false)
+        }
+
+    }
+
     render() {
         // Waiting until ONYX variables are loaded before displaying the component
         if (_.isEmpty(this.props.personalDetails)) {
@@ -607,7 +624,7 @@ class ReportActionCompose extends React.Component {

                                                             // Drop focus to avoid blue focus ring.
                                                             this.actionButton.blur();
-                                                            this.setMenuVisibility(true);
+                                                            this.keyboardListener()
                                                         }}
                                                         style={styles.chatItemAttachButton}
                                                         disabled={isBlockedFromConcierge || this.props.disabled}
@@ -619,7 +636,10 @@ class ReportActionCompose extends React.Component {
                                             <PopoverMenu
                                                 animationInTiming={CONST.ANIMATION_IN_TIMING}
                                                 isVisible={this.state.isMenuVisible}
-                                                onClose={() => this.setMenuVisibility(false)}
+                                                onClose={() => {
+                                                    this.setMenuVisibility(false);
+                                                    this.focus()
+                                                }}
                                                 onItemSelected={() => this.setMenuVisibility(false)}
                                                 anchorPosition={styles.createMenuPositionReportActionCompose}
                                                 menuItems={[...this.getIOUOptions(reportParticipants),
@@ -629,6 +649,7 @@ class ReportActionCompose extends React.Component {
                                                         onSelected: () => {
                                                             openPicker({
                                                                 onPicked: displayFileInModal,
+                                                                onAttachmentModalClose: () => this.focus()
                                                             });
                                                         },
                                                     },

Now in AttachmentPicker.js, onAttachmentModalClose function will be executed after closing the attachment modal and composer will be focused.

diff --git a/src/components/AttachmentPicker/index.native.js b/src/components/AttachmentPicker/index.native.js
index 6944f75fd..6d6f8345d 100644
--- a/src/components/AttachmentPicker/index.native.js
+++ b/src/components/AttachmentPicker/index.native.js
@@ -102,6 +102,7 @@ class AttachmentPicker extends Component {

         this.state = {
             isVisible: false,
+            onAttachmentModalClose: () => {},
         };

         this.menuItemData = [
@@ -275,6 +276,7 @@ class AttachmentPicker extends Component {
       */
     close() {
         this.setState({isVisible: false});
+        this.state.onAttachmentModalClose();
     }

     /**
@@ -303,7 +305,10 @@ class AttachmentPicker extends Component {
       */
     renderChildren() {
         return this.props.children({
-            openPicker: ({onPicked}) => this.open(onPicked),
+            openPicker: ({onPicked, onAttachmentModalClose}) => {
+                this.open(onPicked); 
+                this.setState({onAttachmentModalClose: onAttachmentModalClose})
+            },
         });
     }

Demo

https://user-images.githubusercontent.com/79830748/212896315-9d980920-db4c-4689-b6ba-83efa4ca1a2d.mp4

thesahindia commented 1 year ago

Hey guys I am seeing that the latest proposals are only about attachment picker but as mentioned in https://github.com/Expensify/App/issues/13922#issuecomment-1381018556 we wanna fix this for all modals.

thesahindia commented 1 year ago

@tienifr, we wanna go with 2nd example. The behaviour that slack has is the expected behaviour.

priyeshshah11 commented 1 year ago

Hey guys I am seeing that the latest proposals are only about attachment picker but as mentioned in #13922 (comment) we wanna fix this for all modals.

@thesahindia @trjExpensify This issue only occurs when you move from one modal to another modal and I think attachment picker is the only place that does this, do you know of any other places that has the same behaviour? Composer already gets focused in case of closing a single modal/context menu. Have you tried my proposal above?

trjExpensify commented 1 year ago

Yeah, I couldn't think of where else we have this in App? @thesahindia did you have somewhere in mind?

tienifr commented 1 year ago

Updated Proposal

@thesahindia Thanks for your review. The behavior that slack has is the expected behavior, when user opens the modal we'll hide the keyboard and just open the keyboard again if user clicks outside to close the modal as I'll reopen the keyboard on onClose of AttachmentPicker and PopoverMenu. I also reopen the keyboard if AttachmentModal is hided

diff --git a/src/components/AttachmentPicker/attachmentPickerPropTypes.js b/src/components/AttachmentPicker/attachmentPickerPropTypes.js
index 8338ad9bb9..2479c527c4 100644
--- a/src/components/AttachmentPicker/attachmentPickerPropTypes.js
+++ b/src/components/AttachmentPicker/attachmentPickerPropTypes.js
@@ -28,6 +28,7 @@ const propTypes = {

 const defaultProps = {
     type: CONST.ATTACHMENT_PICKER_TYPE.FILE,
+    onClose: () => {},
 };

 export {
diff --git a/src/components/AttachmentPicker/index.native.js b/src/components/AttachmentPicker/index.native.js
index 6944f75fd2..604e6afb93 100644
--- a/src/components/AttachmentPicker/index.native.js
+++ b/src/components/AttachmentPicker/index.native.js
@@ -274,6 +274,7 @@ class AttachmentPicker extends Component {
       * Closes the attachment modal
       */
     close() {
+        this.props.onClose();
         this.setState({isVisible: false});
     }

@@ -293,7 +294,7 @@ class AttachmentPicker extends Component {
             200,
         );

-        this.close();
+        this.setState({isVisible: false});
     }

     /**
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index f88ea64b4f..5dec643d64 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -134,6 +135,7 @@ class ReportActionCompose extends React.Component {
         this.getInputPlaceholder = this.getInputPlaceholder.bind(this);
         this.getIOUOptions = this.getIOUOptions.bind(this);
         this.addAttachment = this.addAttachment.bind(this);
+        this.onCloseModal = this.onCloseModal.bind(this);

         this.comment = props.comment;
         this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();
@@ -153,6 +155,7 @@ class ReportActionCompose extends React.Component {

             // If we are on a small width device then don't show last 3 items from conciergePlaceholderOptions
             conciergePlaceholderRandomIndex: _.random(this.props.translate('reportActionCompose.conciergePlaceholderOptions').length - (this.props.is
SmallScreenWidth ? 4 : 1)),
+            shouldReFocus: false,
         };
     }

@@ -513,6 +516,12 @@ class ReportActionCompose extends React.Component {
         this.props.onSubmit(comment);
     }

+    onCloseModal() {
+        if (this.state.shouldReFocus) {
+            this.focus();
+        }
+    }
+
     render() {
         // Waiting until ONYX variables are loaded before displaying the component
         if (_.isEmpty(this.props.personalDetails)) {
@@ -553,10 +562,13 @@ class ReportActionCompose extends React.Component {
                     <AttachmentModal
                         headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
                         onConfirm={this.addAttachment}
+                        onModalHide={this.onCloseModal}
                     >
                         {({displayFileInModal}) => (
                             <>
-                                <AttachmentPicker>
+                                <AttachmentPicker
+                                    onClose={this.onCloseModal}
+                                >
                                     {({openPicker}) => (
                                         <>
                                             <View style={[
@@ -608,7 +620,17 @@ class ReportActionCompose extends React.Component {

                                                                 // Drop focus to avoid blue focus ring.
                                                                 this.actionButton.blur();
-                                                                this.setMenuVisibility(true);
+                                                                if (this.props.isKeyboardShown) {
+                                                                    const keyboardListener = Keyboard.addListener('keyboardDidHide', () => {
+                                                                        this.setState({shouldReFocus: true});
+                                                                        this.setMenuVisibility(true);
+                                                                        keyboardListener.remove();
+                                                                    });
+                                                                    Keyboard.dismiss();
+                                                                } else {
+                                                                    this.setState({shouldReFocus: false});
+                                                                    this.setMenuVisibility(true);
+                                                                }
                                                             }}
                                                             style={styles.chatItemAttachButton}
                                                             disabled={isBlockedFromConcierge || this.props.disabled}
@@ -621,7 +643,10 @@ class ReportActionCompose extends React.Component {
                                             <PopoverMenu
                                                 animationInTiming={CONST.ANIMATION_IN_TIMING}
                                                 isVisible={this.state.isMenuVisible}
-                                                onClose={() => this.setMenuVisibility(false)}
+                                                onClose={() => {
+                                                    this.setMenuVisibility(false);
+                                                    this.onCloseModal();
+                                                }}
                                                 onItemSelected={() => this.setMenuVisibility(false)}
                                                 anchorPosition={styles.createMenuPositionReportActionCompose}
                                                 menuItems={[...this.getIOUOptions(reportParticipants),

Result

https://user-images.githubusercontent.com/113963320/213083798-6b1fba5f-4dd3-45a0-a79f-911ca12f82f4.mp4

thesahindia commented 1 year ago

Yeah, I couldn't think of where else we have this in App?

I think we should fix this at the root because the same issue will be reproducible if we implement a similar flow of transitioning from one modal to another.

@thesahindia did you have somewhere in mind?

The same issue can be seen at emoji picker. The keyboard is not visible like the other instance but it's the same issue -

https://user-images.githubusercontent.com/83179295/213187804-c7fa262c-1f95-4810-94e4-d1bcc4738138.mov

priyeshshah11 commented 1 year ago

@thesahindia The root cause is still the same which is that iOS tries to reopen the keyboard when the modal is closed. A more generic solution would be to implement the same logic from my solution proposed above to the Modal component. However, that will still not fix the issue of switching between two modals as it will try to reopen the keyboard after closing the 1st modal & before opening the 2nd modal. Thus, I feel this is always going to be a seperate issue.

Issue 1: Dismiss keyboard before closing all modals & then reopen keyboard when modal is dismissed. (generic) Issue 2: Handle reopening the keyboard when switching between two modals (specifically for Attachments)

Considering that so much time has already been spent proposing solutions so far, I would like to get a clear statement on the expected behaviour & the scope of this issue from the team before proposing any new solutions. As it keeps changing after every proposal.

@trjExpensify Please share your thoughts.

thesahindia commented 1 year ago

@priyeshshah11, FYI the expected result was mentioned in https://github.com/Expensify/App/issues/13922#issuecomment-1381018556 6 days ago

priyeshshah11 commented 1 year ago

@priyeshshah11, FYI the expected result was mentioned in https://github.com/Expensify/App/issues/13922#issuecomment-1381018556 6 days ago

@thesahindia Hopefully my comment here addresses that comment which says that they both are kind of different bugs i.e. solving issue 1 want solve issue 2. what are your thoughts on that?

priyeshshah11 commented 1 year ago

Proposal Update

As mentioned above, in order to fix both the issues we need to

  1. solve keyboard flash issue for all modals
  2. solution from 1 will still not work in case of using two consecutive modals, thus we need to fix that too.

In order to fix both we should apply the diff below along with my solution from here https://github.com/Expensify/App/issues/13922#issuecomment-1382215767

Solution

The code below handles the hiding & reopening the keyboard in the Modal/index.ios.js component itself. It also introduces a new prop to Modal called shouldReopenKeyboard and is set to false by default. We can set this prop to true, anywhere we want to let the Modal handle the keyboard, everywhere else it would use the default behaviour or you can override it in a specific component/use-case like attachments.

Note: Adding this prop is totally optional and we can still work without it if we want Modal to handle the keyboard by default for all Modals. This still works fine with my previous solution above. We can even apply this to Android for consistency but I don't think it's really necessary as it an iOS specific bug.

diff --git a/src/components/EmojiPicker/EmojiPicker.js b/src/components/EmojiPicker/EmojiPicker.js
index be621f368..941997068 100644
--- a/src/components/EmojiPicker/EmojiPicker.js
+++ b/src/components/EmojiPicker/EmojiPicker.js
@@ -141,6 +141,7 @@ class EmojiPicker extends React.Component {
         // emojis. The best alternative is to set it to 1ms so it just "pops" in and out
         return (
             <PopoverWithMeasuredContent
+                shouldReopenKeyboard
                 isVisible={this.state.isEmojiPickerVisible}
                 onClose={this.hideEmojiPicker}
                 onModalShow={this.focusEmojiSearchInput}
diff --git a/src/components/Modal/index.ios.js b/src/components/Modal/index.ios.js
index 85a95142a..1cbe7cf02 100644
--- a/src/components/Modal/index.ios.js
+++ b/src/components/Modal/index.ios.js
@@ -1,18 +1,61 @@
-import React from 'react';
+import React, {useState, useEffect} from 'react';
+import {InteractionManager, Keyboard} from 'react-native';
+import compose from '../../libs/compose';
+import ReportActionComposeFocusManager from '../../libs/ReportActionComposeFocusManager';
+import withKeyboardState from '../withKeyboardState';
 import withWindowDimensions from '../withWindowDimensions';
 import BaseModal from './BaseModal';
 import {propTypes, defaultProps} from './modalPropTypes';

-const Modal = props => (
-    <BaseModal
-            // eslint-disable-next-line react/jsx-props-no-spreading
-        {...props}
-    >
-        {props.children}
-    </BaseModal>
-);
+const Modal = (props) => {
+    const [isVisible, setIsVisible] = useState(false);
+    const [wasKeyboardOpen, setWasKeyboardOpen] = useState(false);
+
+    const reOpenKeyboard = () => {
+        if (!wasKeyboardOpen) {
+            setIsVisible(false);
+            return;
+        }
+        setIsVisible(false);
+        InteractionManager.runAfterInteractions(() => {
+            const composerRef = ReportActionComposeFocusManager.composerRef;
+            if (props.shouldReopenKeyboard && composerRef.current && wasKeyboardOpen) {
+                composerRef.current.focus();
+            }
+            setWasKeyboardOpen(false);
+        });
+    };
+
+    useEffect(() => {
+        if (!props.isVisible) {
+            reOpenKeyboard();
+        } else if (props.shouldReopenKeyboard && props.isKeyboardShown) {
+            const keyboardListener = Keyboard.addListener('keyboardDidHide', () => {
+                setIsVisible(true);
+                setWasKeyboardOpen(true);
+                keyboardListener.remove();
+            });
+            Keyboard.dismiss();
+        } else {
+            setIsVisible(true);
+        }
+    }, [props.isVisible, props.isKeyboardShown]);
+
+    return (
+        <BaseModal
+        // eslint-disable-next-line react/jsx-props-no-spreading
+            {...props}
+            isVisible={isVisible}
+        >
+            {props.children}
+        </BaseModal>
+    );
+};

 Modal.propTypes = propTypes;
 Modal.defaultProps = defaultProps;
 Modal.displayName = 'Modal';
-export default withWindowDimensions(Modal);
+export default compose(
+    withWindowDimensions,
+    withKeyboardState,
+)(Modal);
diff --git a/src/components/Modal/modalPropTypes.js b/src/components/Modal/modalPropTypes.js
index 8f09588e3..277f9e489 100644
--- a/src/components/Modal/modalPropTypes.js
+++ b/src/components/Modal/modalPropTypes.js
@@ -8,6 +8,9 @@ const propTypes = {
     /** Decides whether the modal should cover fullscreen. FullScreen modal has backdrop */
     fullscreen: PropTypes.bool,

+    /** Should we close modal on outside click */
+    shouldReopenKeyboard: PropTypes.bool,
+
     /** Should we close modal on outside click */
     shouldCloseOnOutsideClick: PropTypes.bool,

@@ -66,6 +69,7 @@ const propTypes = {

 const defaultProps = {
     fullscreen: true,
+    shouldReopenKeyboard: false,
     shouldCloseOnOutsideClick: false,
     shouldSetModalVisibility: true,
     onSubmit: null,
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 4f426986e..a871a97b8 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -718,7 +718,6 @@ class ReportActionCompose extends React.Component {
                     {DeviceCapabilities.canUseTouchScreen() && this.props.isMediumScreenWidth ? null : (
                         <EmojiPickerButton
                             isDisabled={isBlockedFromConcierge || this.props.disabled}
-                            onModalHide={() => this.focus(true)}
                             onEmojiSelected={this.addEmojiToTextBox}
                         />
                     )}
diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 97fffebeb..788b17781 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -255,7 +255,6 @@ class ReportActionItemMessageEdit extends React.Component {
                     <View style={styles.editChatItemEmojiWrapper}>
                         <EmojiPickerButton
                             isDisabled={this.props.shouldDisableEmojiPicker}
-                            onModalHide={() => InteractionManager.runAfterInteractions(() => this.textInput.focus())}
                             onEmojiSelected={this.addEmojiToTextBox}
                             nativeID={this.emojiButtonID}
                         />

I know overall it is a pretty big proposal but the task itself is pretty huge, the code can be cleaned up further during the PR process.

@thesahindia @trjExpensify

thesahindia commented 1 year ago

I think this proposal was the best so far. We just have to add additional logic for focusing the composer again once the user closes the modal. I think we should keep the refocus logic outside of src/components/Modal/index.ios.js @tienifr, can you update your proposal?

tienifr commented 1 year ago

sure @thesahindia , here's my updated proposal

The refocus logic is kept outside of the Modal/index.ios.js as you recommended, while the logic to prevent race conditions between the Keyboard and the Modal opening is kept inside Modal/index.ios.js

diff --git a/src/components/AttachmentPicker/attachmentPickerPropTypes.js b/src/components/AttachmentPicker/attachmentPickerPropTypes.js
index 8338ad9bb9..bc0606b06d 100644
--- a/src/components/AttachmentPicker/attachmentPickerPropTypes.js
+++ b/src/components/AttachmentPicker/attachmentPickerPropTypes.js
@@ -24,10 +24,12 @@ const propTypes = {

     /** The types of files that can be selected with this picker. */
     type: PropTypes.oneOf([CONST.ATTACHMENT_PICKER_TYPE.FILE, CONST.ATTACHMENT_PICKER_TYPE.IMAGE]),
+    onClose: PropTypes.func,
 };

 const defaultProps = {
     type: CONST.ATTACHMENT_PICKER_TYPE.FILE,
+    onClose: () => {},
 };

 export {
diff --git a/src/components/AttachmentPicker/index.native.js b/src/components/AttachmentPicker/index.native.js
index 6944f75fd2..1f81cbc6d0 100644
--- a/src/components/AttachmentPicker/index.native.js
+++ b/src/components/AttachmentPicker/index.native.js
@@ -102,6 +102,7 @@ class AttachmentPicker extends Component {

         this.state = {
             isVisible: false,
+            isPickingAttachment: false,
         };

         this.menuItemData = [
@@ -141,6 +142,8 @@ class AttachmentPicker extends Component {
       * @returns {Promise}
       */
     pickAttachment(attachments = []) {
+        this.props.onClose();
+        this.setState({isPickingAttachment: false});
         if (attachments.length === 0) {
             return;
         }
@@ -293,7 +296,7 @@ class AttachmentPicker extends Component {
             200,
         );

-        this.close();
+        this.setState({isVisible: false, isPickingAttachment: true});
     }

     /**
@@ -314,7 +317,14 @@ class AttachmentPicker extends Component {
                     onClose={this.close}
                     isVisible={this.state.isVisible}
                     anchorPosition={styles.createMenuPosition}
-                    onModalHide={this.onModalHide}
+                    onModalHide={() => {
+                        if (this.onModalHide) {
+                            this.onModalHide();
+                        }
+                        if (!this.state.isPickingAttachment) {
+                            this.props.onClose();
+                        }
+                    }}
                 >
                     <View style={this.props.isSmallScreenWidth ? {} : styles.createMenuContainer}>
                         {
diff --git a/src/components/Modal/index.ios.js b/src/components/Modal/index.ios.js
index 85a95142a3..67a44f8d63 100644
--- a/src/components/Modal/index.ios.js
+++ b/src/components/Modal/index.ios.js
@@ -1,18 +1,46 @@
-import React from 'react';
+import React, {useEffect, useState} from 'react';
+import {Keyboard} from 'react-native';
+import {compose} from 'underscore';
+import withKeyboardState from '../withKeyboardState';
 import withWindowDimensions from '../withWindowDimensions';
 import BaseModal from './BaseModal';
 import {propTypes, defaultProps} from './modalPropTypes';

-const Modal = props => (
-    <BaseModal
+const Modal = (props) => {
+    const [isVisible, setIsVisible] = useState(false);
+
+    useEffect(() => {
+        if (!props.isVisible) {
+            setIsVisible(false);
+        } else if (props.isKeyboardShown) {
+            const keyboardListener = Keyboard.addListener('keyboardDidHide', () => {
+                setIsVisible(true);
+                keyboardListener.remove();
+                if (props.onBecomingVisible) {
+                    props.onBecomingVisible(true);
+                }
+            });
+            Keyboard.dismiss();
+        } else {
+            setIsVisible(true);
+            if (props.onBecomingVisible) {
+                props.onBecomingVisible(false);
+            }
+        }
+    }, [props.isVisible, props.isKeyboardShown]);
+
+    return (
+        <BaseModal
             // eslint-disable-next-line react/jsx-props-no-spreading
-        {...props}
-    >
-        {props.children}
-    </BaseModal>
-);
+            {...props}
+            isVisible={isVisible}
+        >
+            {props.children}
+        </BaseModal>
+    );
+};

 Modal.propTypes = propTypes;
 Modal.defaultProps = defaultProps;
 Modal.displayName = 'Modal';
-export default withWindowDimensions(Modal);
+export default compose(withWindowDimensions, withKeyboardState)(Modal);
diff --git a/src/components/PopoverMenu/index.js b/src/components/PopoverMenu/index.js
index 0dabaa3324..903f951b22 100644
--- a/src/components/PopoverMenu/index.js
+++ b/src/components/PopoverMenu/index.js
@@ -95,6 +95,7 @@ class PopoverMenu extends PureComponent {
                 animationInTiming={this.props.animationInTiming}
                 disableAnimation={this.props.disableAnimation}
                 fromSidebarMediumScreen={this.props.fromSidebarMediumScreen}
+                onBecomingVisible={this.props.onBecomingVisible}
             >
                 <View style={this.props.isSmallScreenWidth ? {} : styles.createMenuContainer}>
                     {!_.isEmpty(this.props.headerText) && (
diff --git a/src/components/PopoverMenu/popoverMenuPropTypes.js b/src/components/PopoverMenu/popoverMenuPropTypes.js
index 94a02525b1..64ec78aa37 100644
--- a/src/components/PopoverMenu/popoverMenuPropTypes.js
+++ b/src/components/PopoverMenu/popoverMenuPropTypes.js
@@ -53,6 +53,8 @@ const propTypes = {

     /** Whether disable the animations */
     disableAnimation: PropTypes.bool,
+
+    onBecomingVisible: PropTypes.func,
 };

 const defaultProps = {
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index f88ea64b4f..a00b45a231 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -134,6 +134,7 @@ class ReportActionCompose extends React.Component {
         this.getInputPlaceholder = this.getInputPlaceholder.bind(this);
         this.getIOUOptions = this.getIOUOptions.bind(this);
         this.addAttachment = this.addAttachment.bind(this);
+        this.onCloseModal = this.onCloseModal.bind(this);

         this.comment = props.comment;
         this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus();
@@ -153,6 +154,7 @@ class ReportActionCompose extends React.Component {

             // If we are on a small width device then don't show last 3 items from conciergePlaceholderOptions
             conciergePlaceholderRandomIndex: _.random(this.props.translate('reportActionCompose.conciergePlaceholderOptions').length - (this.props.isSmallScreenWidth ? 4 : 1)),
+            shouldRefocus: false,
         };
     }

@@ -513,6 +515,12 @@ class ReportActionCompose extends React.Component {
         this.props.onSubmit(comment);
     }

+    onCloseModal() {
+        if (this.state.shouldRefocus) {
+            this.focus();
+        }
+    }
+
     render() {
         // Waiting until ONYX variables are loaded before displaying the component
         if (_.isEmpty(this.props.personalDetails)) {
@@ -553,10 +561,13 @@ class ReportActionCompose extends React.Component {
                     <AttachmentModal
                         headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
                         onConfirm={this.addAttachment}
+                        onModalHide={this.onCloseModal}
                     >
                         {({displayFileInModal}) => (
                             <>
-                                <AttachmentPicker>
+                                <AttachmentPicker
+                                    onClose={this.onCloseModal}
+                                >
                                     {({openPicker}) => (
                                         <>
                                             <View style={[
@@ -621,7 +632,10 @@ class ReportActionCompose extends React.Component {
                                             <PopoverMenu
                                                 animationInTiming={CONST.ANIMATION_IN_TIMING}
                                                 isVisible={this.state.isMenuVisible}
-                                                onClose={() => this.setMenuVisibility(false)}
+                                                onClose={() => {
+                                                    this.setMenuVisibility(false);
+                                                    this.onCloseModal();
+                                                }}
                                                 onItemSelected={() => this.setMenuVisibility(false)}
                                                 anchorPosition={styles.createMenuPositionReportActionCompose}
                                                 menuItems={[...this.getIOUOptions(reportParticipants),
@@ -635,6 +649,9 @@ class ReportActionCompose extends React.Component {
                                                         },
                                                     },
                                                 ]}
+                                                onBecomingVisible={(didCloseKeyboard) => {
+                                                    this.setState({shouldRefocus: didCloseKeyboard});
+                                                }}
                                             />
                                         </>
                                     )}

Working well after the fix:

https://user-images.githubusercontent.com/113963320/213840255-4e2451fc-66cc-4a71-aada-a133e67b6730.mov

priyeshshah11 commented 1 year ago

Proposal Update

@thesahindia I thought you wanted refocusing to be handled by the Modal when you said "Let's fix this at every place by solving this at the root. The proposals needs to be updated."

If that's not the case then my proposal from here along with this change just works fine which fixes all other modals without the need of introducing any new props elsewhere.


diff --git a/src/components/Modal/index.ios.js b/src/components/Modal/index.ios.js
index 85a95142a..20603eca7 100644
--- a/src/components/Modal/index.ios.js
+++ b/src/components/Modal/index.ios.js
@@ -1,18 +1,43 @@
-import React from 'react';
+import React, {useState, useEffect} from 'react';
+import {Keyboard} from 'react-native';
+import compose from '../../libs/compose';
+import withKeyboardState from '../withKeyboardState';
 import withWindowDimensions from '../withWindowDimensions';
 import BaseModal from './BaseModal';
 import {propTypes, defaultProps} from './modalPropTypes';

-const Modal = props => (
-    <BaseModal
-            // eslint-disable-next-line react/jsx-props-no-spreading
-        {...props}
-    >
-        {props.children}
-    </BaseModal>
-);
+const Modal = (props) => {
+    const [isVisible, setIsVisible] = useState(false);
+
+    useEffect(() => {
+        if (!props.isVisible) {
+            setIsVisible(false);
+        } else if (props.isKeyboardShown) {
+            const keyboardListener = Keyboard.addListener('keyboardDidHide', () => {
+                setIsVisible(true);
+                keyboardListener.remove();
+            });
+            Keyboard.dismiss();
+        } else {
+            setIsVisible(true);
+        }
+    }, [props.isVisible, props.isKeyboardShown]);
+
+    return (
+        <BaseModal
+        // eslint-disable-next-line react/jsx-props-no-spreading
+            {...props}
+            isVisible={isVisible}
+        >
+            {props.children}
+        </BaseModal>
+    );
+};

 Modal.propTypes = propTypes;
 Modal.defaultProps = defaultProps;
 Modal.displayName = 'Modal';
-export default withWindowDimensions(Modal);
+export default compose(
+    withWindowDimensions,
+    withKeyboardState,
+)(Modal);
thesahindia commented 1 year ago

@tienifr, your proposal seems simpler. can we use the focus method (code below) for focusing the composer after the popover closes ? If not any reasons why?

https://github.com/Expensify/App/blob/ae66a7f019dda888390173a542b273d97042eaaa/src/pages/home/report/ReportActionCompose.js#L348

tienifr commented 1 year ago

@thesahindia that method is already used in my proposal for refocusing the composer, in ‘onCloseModal’

priyeshshah11 commented 1 year ago

@tienifr, your proposal seems simpler.

@thesahindia May I ask why @tienifr's solution seems simpler than mine?

thesahindia commented 1 year ago

@thesahindia May I ask why @tienifr's solution seems simpler than mine?

It needs less changes.

thesahindia commented 1 year ago

@thesahindia that method is already used in my proposal for refocusing the composer, in ‘onCloseModal’

Oh my bad I misinterpreted. I also meant to ask why we aren't directly passing this.focus at onClose prop of PopoverMenu and AttachmentModal ? Do you think it can be used directly without making a new state?