iOS issue: Modal closes immediately after launch

[stephanieelliott]

If you haven’t already, check out our contributing guidelines for onboarding!

---

Expected Result:

Add attachment modal should remain open until dismissed.

Actual Result:

Add Attachment modal closes immediately when we attempt to display it. This only seems to occur when we attempt to launch Modal B from a callback within Modal A

Action Performed:

At present, we've applied a temporary fix for this issue by using setTimeout to delay the launching of the second Modal However, that fix is sub-optimal and we are seeking a better permanent solution. See screen recording for a demo of the issue.

1. Open Expensify.cash on iOS
2. Tap into a chat with 2 or more participants
3. Click the paperclip icon to launch Modal A
4. Click Add Attachment to launch Modal B
5. Notice the Add Attachment modal (Modal B) closes immediately

Notes

This is the location of the problem/workaround.

Platform:

iOS

Version Number: 1.0.2-89

Videos:

E.cash157543.mov

[stephanieelliott]

Upwork: https://www.upwork.com/jobs/~014b63897c4e74f239

[adnan1naeem]

@stephanieelliott .I have solved this issue in many project.I have also bid on your upwork post.
Please contact me this is my profile
https://www.upwork.com/freelancers/~01f994b11784d8d018

solution to problem:
react-native-image-picker/react-native-image-picker#1633

https://stackoverflow.com/questions/59574338/react-native-image-picker-dialog-automatically-closes-after-launching-in-ios

[barun1997]

@stephanieelliott I couldn’t find the modal A in the master branch. Is it yet to be merged? Or am I missing something?

[muratti32]

inside the function that assigned to the onShow props, The state that holds the modal visible value is set to false, causing the modal to vanish after it has been seen.

[Julesssss]

Hi @barun1997, thanks for mentioning this. The issue is occurring on a branch that has not yet been merged and I've added the details to the description.

Please note that as this issue is not yet merged we will need to keep any potential PR on hold.

[kidroca]

The problem comes from the next view that appears after the modal closes is another kind of modal - action sheet (ios).
They are both modal type components they use the same native mechanism to handle a modal.
The first modal need to fully close before triggering the second one
I see you are using the onModalHide prop witch should fire when the modal is completely hidden, but since a delay fixes the issue it means that onModalHide does not fire when the native modal controller is released only when visible animations are over

If it was up to me I would merge the pending PR (if there are no other issues in there). You could swap the setTimeout(..., 100) with a requestAnimationFrame(...) for the same temp fix. Then I would hunt for a better solution.

Here are some interesting things about the 2nd modal. It's an ActionSheetIOS managed from inside the react-native-image-picker. You are using v2. of that library. In v3 they've dropped support for this API (showImagePicker) - it's up to you to present a suitable way to select camera, gallery or other options.
Continuing on that thought it means that if you ever want to switch to v3 (or use a different camera lib) you'd need to implement the 2nd modal/ActionSheet anyway

If you want to preserve the same UI you can use something like this: https://github.com/expo/react-native-action-sheet works on ios/android/web. I've used id precisely to present the choices "Camera", "Gallery" in a project of mine (pure react-native, the lib doesn't depend on expo). If you check the props it supports useModal flag and it can work without hogging the native modal controller.
The entire code changes to make this reality would be encapsulated in your AttachmentPicker/index.native.js

Another approach is again not to rely on react-native-image-picker for the ActionSheet but instead of closing your first modal - when "Add Attachment" is pressed, slide the modal further up and review the available option for "Add Attachment" as a slightly indented list of options below. Something similar to the Material UI Nested List. This is potentially a UX improvement as it removes the "modal close another modal open" transition

[Julesssss]

FYI, we have merged the PR now, so you should be able to reproduce locally.

Thanks for your proposal @kidroca. Ideally we would use our own option-picker UI rather than the iOS style action-sheet, which didn't exist when we implemented ImagePicker. But as this s built on top of Modal, we would still have the Modal issue.

Because of this, our ideal proposal would provide a solution to the 2-modal problem, rather than replace the second Modal with another component. We will most likely like to use the same 2-Modal pattern elsewhere in the app.

Does anyone have a proposal for this? @adnan1naeem @muratti32 you responded but haven't yet shared a proposal.

[kidroca]

Thanks for the info @Julesssss

What you have planned would actually work if you use a Modal for the 2nd menu, as long as it's not an Action Sheet

I've played around with this and narrowed the problem to precisely the Modal closes -> Action Sheet opens transition.
I've checked and if you open another modal (similar to the one used in CreateMenu) there's no such problem - the first modal closes and the next one opens without the need of any kind of timeout.

Proposal

My proposal is to implement an option-picker UI that will replace the current AttachmentPicker functionality
It will be based on a Modal as per your request
If the design is similar or close to the way the CreateMenu component works I can extract the menu items here to a prop and provide them from a parent component:
https://github.com/Expensify/Expensify.cash/blob/fe0ea569c98ba54e585eae0d81e20455fcb85c6e/src/components/CreateMenu.js#L48

So that you can reuse CreateMenu to list different options

If that's undesired and you have a specific design for a Attachment Picker Modal I can implement it based on your Popover component or a suitable Modal component

---

For the curious: Details about why Modal to Modal works but Modal to ActionSheet does not

The reason why it would work with Modal to Modal transition is Modals in react native (ios) are implemented to wait for each other - it seems a workflow like modal to modal was anticipated or kind of get covered by the fact that ios does not allow more than one modals at once (ux antipattern) so if more than one modals happen to be active at the same time the 2nd one will be shown after the first one closes. But a Modal to ActionSheet seems to not have been anticipated - the ActionSheet takes over the view that is just about to get destroyed and results in response.didCancel

[Julesssss]

Thanks for your updated proposal!

If that's undesired and you have a specific design for a Attachment Picker Modal I can implement it based on your Popover component or a suitable Modal component

Let's await feedback from the design team to confirm what type of UI we should use here to replace the default react-native-image-picker UI.

Hey @Expensify/design, we're looking to replace the native-style attachment UI that is used on mobile (we need to update to fix a bug in the library and can no longer use the built in UI). We didn't have a consistent component style when we originally implemented this, so it's a good chance to redo this properly. Could you suggest a replacement? Perhaps we have a preference for custom or native UI here?

This will only be displayed on mobile -- here is what this currently looks like:

iOS

Android

[shawnborton]

I think it makes sense to use the same styles that we use for the green floating action button on the home screen. So basically just launch a bottomDocked modal that has a few options. Would that work well?

[Julesssss]

I believe that's exactly what @kidroca had in mind here:

If the design is similar or close to the way the CreateMenu component works I can extract the menu items here to a prop and provide them from a parent component:

Sounds good to me!

[kidroca]

Yep, it's pretty close to CreateMenu and it can be refactored for reuse:

I see 2 easy approaches:

The question is do you prefer the MENU_ITEMS extracted and provided by a parent or a HOC?
Or do you prefer to keep it the way it is and include the attachment options (Take Photo, Choose from Gallery...) in MENU_ITEMS https://github.com/Expensify/Expensify.cash/blob/fe0ea569c98ba54e585eae0d81e20455fcb85c6e/src/components/CreateMenu.js#L48

Like:

const MENU_ITEMS = {
  [CONST.MENU_ITEM_KEYS.TAKE_PHOTO]: {
    icon: Camera,
    text: 'Take Photo',
  },
  ...
}

And just provide a prop like:

  menuOptions={[
    CONST.MENU_ITEM_KEYS.TAKE_PHOTO,
    CONST.MENU_ITEM_KEYS.GALLERY, 
    CONST.MENU_ITEM_KEYS.GALLERY,
  ]}

The attachment options could come from a new mapping like CONST.ATTACHMENT_OPTION_KEYS if you prefer

My idea is no matter which approach is selected to create a replacement component for AttachmentPicker which would encapsulate how the actual options work and delegate rendering to CreateMenu
E.g. the call to RNImagePicker.launchCamera() would not be in CreateMenu but in the AttachmentPicker component

[AndrewGable]

@kidroca - Thank you for your solution, I have sent sent you the job on upwork. We look forward to your pull request.

[Julesssss]

Hi @kidroca,

The question is do you prefer the MENU_ITEMS extracted and provided by a parent or a HOC?
Or do you prefer to keep it the way it is and include the attachment options (Take Photo, Choose from Gallery...) in MENU_ITEMS

A colleague has a similar issue and has created a new MenuComponent for their new menu. Let's just do that for now, and we'll worry about refactoring them into a single Component later on (out of scope of this PR).

[kidroca]

Ok, so create a standalone component, encapsulating attachment options and delegating rendering to the <Popover /> component (to render as a bottom docked modal)

Thanks for the reference

[Julesssss]

Yeah, exactly 👍

[kidroca]

@shawnborton
Do you want to add a title for the bottom picker?

---

I need to get the "Camera" and the "Picture" icons (svg), they are yet to be added to the current assets. I don't know where to find them?

[kidroca]

@Julesssss
Can you tell me more about the logic here. Particularly why the input should focus after any modal close?
https://github.com/Expensify/Expensify.cash/blob/46d6cb4911c923bb9ee367e42076f33e8cd93f45/src/pages/home/report/ReportActionCompose.js#L81

// in componentDidUpdate
    if (
        (prevProps.modal.isVisible && !this.props.modal.isVisible)
        || (prevProps.reportID !== this.props.reportID)
    ) {
       this.setIsFocused(true);
    }
    
// in setIsFocused
    if (shouldHighlight && this.textInput) {
         this.textInput.focus();
    }

This is causing a few side effects:

• when we want to switch to the attachment modal, componentDidUpdate would invoke setIsFocused(), because the 1st modal closes, this in turn focuses the field and brings up the keyboard. The 2nd modal is prevented from appearing. The call to this.textInput.focus() would actually close any open modal, because the field is outside the modal. This is problematic as the call happens just between the modal-to-modal transition
• another (I think bigger) problem is: even when you are on a different screen the code in componentDidUpdate runs making side effects exactly as described here: [Chat] [IOU] Mobile keyboard should not be displayed at IOUConfirm step #1855. Because ReportActionCompose stays mounted any modal can trigger a side effect.

The decision to fix #1855 is to make ReportActionCompose aware of whether it's focused or not receiving a prop isScreenFocused and prevent focusing the text input in case it's not. I like the approach, but it won't help the modal to modal transition

These unintentional side effects are (I guess) why some code styles flag componentDidUpdate as an antipattern.
In order to help you come up with an optimal solution I need to know the use case. Here's what I know so far:

We want to bring focus back to the input after an attachment is selected so the user don't have to manually tap it -> this can happen by bringing focus back from here:

<AttachmentModal
    title="Upload Attachment"
    onModalHide={() => this.setIsFocused(true)} // my change
    onConfirm={(file) => {
        addAction(this.props.reportID, '', file);
        this.setTextInputShouldClear(false);
    }}
>

We want to bring focus back to the input if the field was focused prior to displaying a modal -> This already happens automatically on mobile (without the logic in componentDidUpdate)

I think I've covered most of the "critical" cases regarding bringing back focus to the text input in my code without adding much changes and without relying on componentDidUpdate. I'm yet to clean up and post a PR, but in the meantime I would like to know your opinion on this?

[shawnborton]

@kidroca no need for a title. Here are the assets you need: Icons.zip

[Julesssss]

Hi @kidroca,

We want to bring focus back to the input after an attachment is selected so the user don't have to manually tap it

It looks like you worked out the reason yourself, but yeah -- this was to allow us to keep the ReportActionCompose Component focused -- originally this was to regain focus after launching the attachment modal (there was only one modal at this point) -- and has led to multiple side effects as we built out the app as you have seen.

Ideally, we desire this behaviour for any modal that is launched while the ReportActionCompose is on screen. While your solution works for the attachment Modal, I don't think it would cover other cases. For example: Tapping the FAB and launching 'new chat' Modal, then closing it by tapping outside.

Do you have any idea whether this is solvable? My only question is whether your solution might have changed at all since the discussion around the HOC on 1855?

[kidroca]

So far my solution, which I'm yet to push 😅, seems to work good on mobile (specifically iOS, need to test Android).

• If some modal opens while you have focus on the TextInput when the modal goes away you are returned to the TextInput and keyboard is back up
• If you press the (+) attachment button, there's a call to first focus the text input and then popup the first Modal (CreateMenu). After you're done - cancelling, selecting attachment then cancelling etc. focus is brought back to the text input

For web though, the focus is only brought back to the input after selecting an attachment. So pressing the FAB and deciding to bail would not bring you back to the field, is that what you mean?

My personal preference as a user is if I haven't been with the keyboard up when a modal opens I don't expect to be when it closes - with the exception of selecting an attachment this is convenient!
But regardless I'll respect the design decision selected here

---

Do you have any idea whether this is solvable?

If you totally must keep the currently intended functionality - focus on the input after any modal close (while you are inside a chat), regardless of whether focus was on the field or not yes, it's still possible, but it will require slight restructuring:

Instead of having a isMenuVisible state, we'll need make a small state machine - something like: menuStage: closed|menu|attachment

• closed none of the ReportActionCompose menus are visible
• menu -> the CreateMenu is open
• attachment (or secondary) -> the 2nd modal is open

In componentDidUpdate respect the prevProps.modal.isVisible logic when none of our own modals are up - closed

This will flatten the render structure a bit:

<AttachmentModal>{/* This is the only modal that can keep a renderProp */}
   <Touchable onPress={() => setMenuStage(MENU)}>
   <CreateMenu isVisible={menuStage == MENU} onItemSelected={item => {/* Decide how to change menuStage */}}  />
   <AttachmentPicker  isVisible={menuStage == ATTACHMENT}  onClose={() => setMenuStage(CLOSED)} />
</AttachmentModal>

I'll have to change the interface of the AttachmentPicker from a component with children render prop to more conventional modal with isVisible onClose onAttachmentPicked etc... To be honest I wanted to do that right from the start, but didn't to keep less changes in ReportActionCompose and make the review easier

---

Another direction can be to add some ids to modals and track which are visible and stuff like that and respond only to certain modal IDs...

---

My current solution doesn't not depend much on the outcome of #1855 - the problem is our own modals are triggering the modal.isOpen prop passed from outside

---

Sidenote, most good chats that I've used seems to remember whether I had focus prior to leaving the char or not. When I open a chat it will initially show with the keyboard collapsed so I can view more of the conversation, but if I had focus prior to leaving this conversation, it will focus the field and show the keyboard

[Julesssss]

For web though, the focus is only brought back to the input after selecting an attachment. So pressing the FAB and deciding to bail would not bring you back to the field, is that what you mean?

Yeah.. but I can see this being tricky, and this is out of scope of this current issue considering we don't currently do this.

My personal preference as a user is if I haven't been with the keyboard up when a modal opens I don't expect to be when it closes - with the exception of selecting an attachment this is convenient!

No you're right, I totally agree with this as a mobile user. I wouldn't want the keyboard to display when opening --The focus|showKeyboard events were separate though aren't they? So can't we request focus, leaving the mobile keyboard hidden? (I thought that was the default, until the user taps in the field again)

Instead of having a isMenuVisible state, we'll need make a small state machine - something like: menuStage: closed|menu|attachment

closed none of the ReportActionCompose menus are visible
menu -> the CreateMenu is open
attachment (or secondary) -> the 2nd modal is open

Thanks for sharing this solution, I think this too would require further discussions, so as mentioned above, I think for now we could keep the scope to the original issue + ensuring that focus remains after AttachmentPicker is hidden.

[kidroca]

No you're right, I totally agree with this as a mobile user. I wouldn't want the keyboard to display when opening --The focus|showKeyboard events were separate though aren't they? So can't we request focus, leaving the mobile keyboard hidden? (I thought that was the default, until the user taps in the field again)

When a modal opens the keyboard would always hide - it won't be visible under the modal
The UX problem for me is when a modal closes the input is focused (and the keyboard is brought up) regardless of previous state. I would expect to focus on the input only if it was focused before the modal

When we call this.textField.focus() it will focus the field and open the keyboard. I don't think we can keep focused with the keyboard collapsed. Unless we have an external keyboard or something. The point is when we request focus the keyboard will open as well

I'm talking about iOS specifically, on mobile Safari you can press "done" to hide the keyboard but focus is lost too

[kaushiktd]

Hi,

This is Kaushik here. I would like to put a technical and pretty self explanatory solution for this.

1. this issue is happening when two modal are launching like: first is close and the second is open at the same time.
   we will put a small timer to resolve this.

[Julesssss]

At present, we've applied a temporary fix for this issue by using setTimeout to delay the launching of the second Modal However, that fix is sub-optimal and we are seeking a better permanent solution.

@kaushiktd thanks for your suggestion, but that is our current solution and we are trying to remove this workaround.