[HOLD] [$250] ReportActionContextMenu not closing properly

[kavimuru]

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 ND
2. Hover on any chat for report action context menu
3. Press Add reaction
4. Now press CTRL+K
5. Press the back arrow to close the search (RHP)
6. Now check the report action context menu after closing modal

   Expected Result:

   Report action context menu should be hidden, and be shown only on hovering

   Actual Result:

   Report action context menu gets stuck

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.48-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/be611be0-6900-438e-b88c-eb1b6b31347f

https://github.com/Expensify/App/assets/43996225/cdd035a1-892a-4edb-b17c-480c4b25215e

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013c8f7f084556466f
• Upwork Job ID: 1687252626772779008
• Last Price Increase: 2024-07-01
• Automatic offers:
• bernhardoj | Contributor | 26013711
• shubham1206agra | Reporter | 26013714

Issue Owner

Current Issue Owner: @ishpaul777

[robertKozik]

I don't think that the bug is that bad to prove patch to be reasonable. In my eyes this bug requires very specific setup and is not that easy to reproduce frequently for normal users

[bernhardoj]

The patch is targetting a fix to the RN modal issue, so it would benefit the whole app and also fix some existing issues (https://github.com/Expensify/App/issues/29610#issuecomment-1763055270, https://github.com/Expensify/App/issues/19558) with the same modal issue root cause. But I just remembered that we would need to create 2 patches, one for RN and one for react-native-modal. I guess it would be hard to maintain both patches (the rn-modal one is simple though)

[adelekennedy]

@robertKozik @puneetlath any thoughts one the patch conversation?

[puneetlath]

I think I agree with @robertKozik that this bug doesn't feel worth the effort required to maintain the patch. I think if we aren't able to fix this upstream, then it's best for us to just close this out.

[shubham1206agra]

@puneetlath Can I get reporting bonus in that case?

[bernhardoj]

@puneetlath what about we try to close the current upstream PR and split it into smaller parts (so we get a new reviewer 😄) and let's see the progress for another month.

[shubham1206agra]

@puneetlath @robertKozik Since RNWeb version of this issue is already merged. So can we fix this issue for web platforms as a partial fix?

[bernhardoj]

I closed the current upstream PR and was planning to split it into smaller parts, but it was not possible, so I opened a new identical PR. Let's see the progress for this one

[bernhardoj]

The new PR is in review.

[adelekennedy]

New PR is in review, looks like it may take a bit more time

[adelekennedy]

new pr was last reviewed 5 hours ago

[bernhardoj]

The PR is merged. Now waiting for it to get into a new RN release.

[shubham1206agra]

I will be taking over here as @robertKozik is no longer C+. Slack https://expensify.slack.com/archives/C02NK2DQWUX/p1705449256141069

[shubham1206agra]

@bernhardoj Would you work on this on draft PR as a headstart?

[bernhardoj]

@shubham1206agra I think I will wait until all upstream PR is done. We still need to update react-native-modal too to use onDismiss.

[shubham1206agra]

Do you have a upstream PR for that?

[bernhardoj]

Not yet, we need the RN PR to be released first which probably happens in 0.74 because it contains a breaking change (the breaking change log was added by the RN reviewer).

The RN release discussion doesn't contain the 0.74 yet, so it might take a while.

[bernhardoj]

The PR was reverted because of an internal issue. The team will try to come up with a plan to split the PR into smaller chunks (and probably fewer changes).

[adelekennedy]

still pending update here

[adelekennedy]

@bernhardoj @robertKozik just checking here - I see this PR is now closed but it looks like @bernhardoj you still are looking for updates, at this point are we just waiting for @cipolleschi to respond?

[bernhardoj]

@adelekennedy correct, I will try to bump it next week if we don't get a reply

[bernhardoj]

Bumped the PR

[bernhardoj]

Hey guys, here is the latest update:

Just as a reminder, what we are trying to achieve is adding an onDismiss support for Android, and fixing onDismiss isn't firing on iOS (Fabric only).

Last month, the PR was merged but was reverted due to Meta's internal issues where they have some code that expects the Android to not fire the onDismiss event.

The reviewer then managed to land the fix for iOS (with minimal changes) based on the PR. What is left is the Android part.

The reviewer then suggest this:

1. We create a PR in the discussion-and-proposal repo and we get the buy in from various people, including internal people, to make this happens in Android.
2. While we wait for the above, you can create a component based on <Modal> with the fix. Something like <EXPModal> and include that component as a dependency of Expensify. Even better if, rather than copying and pasting the code, you manage to have a component that extends the native classes of Modal so that when we push some improvements, your component benefits from them too.

But it's not possible (it's actually possible but more complex) to create a custom native component that extends the react-native Modal and use it because we use react-native-modal. So, I ask the reviewer whether using a patch-package is a viable options or not and he agrees with it.

The question is, do we want to pursue that plan? The patch will include the Android files (3) changes from the upstream PR and removing some platform checks in react-native/Modal.js, so it works for all platforms.

Having this fixed allow us to also fix properly these other related issues (that is caused by delaying the input focus):

• https://github.com/Expensify/App/issues/19558
• https://github.com/Expensify/App/issues/29610 (the issue already has a PR that only delaying the input focus on native)
• https://github.com/Expensify/App/issues/36829 (this can also be solved with focus trap, the issue is closed waiting for the focus trap PR)

and we don't need to delay the input focus anymore when closing a modal.

@shubham1206agra @puneetlath

[shubham1206agra]

@bernhardoj Please post the same in the open source channel in Slack to start a discussion.

[adelekennedy]

@bernhardoj did you post this in Slack? I might have missed it!

[bernhardoj]

@adelekennedy oh sorry, I actually changed my mind and still drafting the react native proposal. I will let you know here once I publish the proposal. If there is no movement on the proposal for a while, then I will reconsider asking for a patch in Slack.

[bernhardoj]

Published the react native proposal here

[adelekennedy]

@bernhardoj it looks like we're still paused here until the rn proposal gets traction is that correct?

[bernhardoj]

@adelekennedy yes, correct.

[adelekennedy]

downgrading to monthly

[adelekennedy]

We're still waiting for feedback on the proposal here - keeping this as monthly

[adelekennedy]

we're still held on the react-native conversation, @bernhardoj it's been a month and no response there yet. What's the appropriate way to get them to respond to us?

[bernhardoj]

@adelekennedy Hmm, I'm not sure what we should do to get their attention. I think they would get interested with it if the many of the community vote for it too, but as you can read from this comment, they are aware that many apps out there already use some workaround with the modal issue.

It's not a new issue btw (the lack of dismiss event on Android), it's been there I guess since the react native is built, so I doubt we will get an update sooner or later.

Going with a patch approach will make it hard to maintain since we need to patch the react-native and react-native-modal library and considering there are no attention to the react-native proposal and react-native-modal repo been inactive, we might need to maintain the patch forever.

IMO, the best path forward for now is to go with my main solution that is previously approved by @robertKozik

cc: @puneetlath @shubham1206agra

[puneetlath]

Would you mind re-explaining that solution? I have forgotten all the context.

[bernhardoj]

Sure.

Issue brief explanation The issue we have here is that if isNavigating is true, we "don't call" the onModalHide. isNavigating is currently set to true every time we close the modal using shortcut. https://github.com/Expensify/App/blob/5699ae382505ed0d962e6170320a367e41e7858c/src/components/EmojiPicker/EmojiPicker.tsx#L109-L112

This is useful for a case like composer that will focus the input after the emoji picker is closed, but we don't want that to happen when we use a shortcut to navigate to the search page for example. https://github.com/Expensify/App/blob/5699ae382505ed0d962e6170320a367e41e7858c/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L487

But this affects other components too that need the onModalHide to be called. (onEmojiPickerClosed is called in onModalHide) https://github.com/Expensify/App/blob/5699ae382505ed0d962e6170320a367e41e7858c/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L146-L149

Solution The solution is:

1. Always call the onModalHide.
2. Pass the isNavigating value to onModalHide
3. Let each component decide what to do with the isNavigating value, for example, composer won't call the focus function if isNavigating is true.

   onModalHide={(isNavigating) => {
   if (isNavigating) return;
   focus();
   }}

[bernhardoj]

@puneetlath so, what do you think?

[mallenexpensify]

@puneetlath 👀 above plz.

[melvin-bot[bot]]

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

[puneetlath]

Sorry for the delay. I re-added the External label in order to get a C+. @ishpaul777 what do you think of the proposal?

[shubham1206agra]

@puneetlath This was actually taken by me as C+ https://github.com/Expensify/App/issues/23959#issuecomment-1895537192

[shubham1206agra]

@bernhardoj Can you create a test branch for this?

[puneetlath]

Ah, my apologies.

[bernhardoj]

@shubham1206agra Here is the test branch: https://github.com/bernhardoj/Expensify/tree/fix/23959-mini-context-menu-doesnt-close

There is an additional change needed, that is removing the freeze capture here. This causes the hover state to not update. https://github.com/Expensify/App/blob/6ac95e4c7fdf555b7ba8c9365fb522305d239dc2/src/pages/home/report/ReportActionItem.tsx#L878-L879

It was added at https://github.com/Expensify/App/pull/41859. @waterim May I know what bug is it with the report preview? Is it that the message isn't highlighted while the payment option popover is shown?

Btw, looks like the price is reduced from the original when the external label is re-applied.

cc: @puneetlath

[shubham1206agra]

Btw, looks like the price is reduced from the original when the external label is re-applied.

I will ask this to be changed later.