[HOLD for payment 2022-12-02] [$1000] [Bug] Pressing escape when a popover is open closes the chat and the page gets stuck in the middle - reported by @Puneet-here

[mvtglobally]

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. Decrease the screen size
2. Go to any chat
3. Click on actions (the :heavy_plus_sign: sign at the left side of message input)
4. Press escape

Expected Result:

Only the popover should be closed

Actual Result:

The popover closes but the chat also gets closed and then it gets stuck in the middle of chat and chat list Also. On web staging v1.2.16-4 and when doing the steps, the popover did not close, the chat closed and user was not able to leave the popover (had the LHN on the background) and had to refresh the page to get to working state

Additionally, this should be resolved in the About section --> Copy To Clipboard page

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.2.15-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: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/196281354-0293809b-28e1-4474-84d7-afa26c4b51f2.mov

Expensify/Expensify Issue URL: Issue reported by: @Puneet-here Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663088510944699

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[adelekennedy]

@mountiny was able to reproduce here! I think this a real one

[melvin-bot[bot]]

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Julesssss]

Confirmed, opening up to contributors

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[adelekennedy]

internal external

[chauchausoup]

Also the issue is found on About section --> Copy To ClipboardPopover.

[Julesssss]

Awaiting proposals

[melvin-bot[bot]]

@eVoloshchak, @Julesssss, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@eVoloshchak, @Julesssss, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Julesssss]

Awaiting proposals

[roryabraham]

This is a regression so let's please do an RCA and follow-up, ideally adding some automated test coverage to prevent this regression from happening again in the future.

[adelekennedy]

What are the next steps to do an RCA? @Julesssss @eVoloshchak

[melvin-bot[bot]]

@eVoloshchak, @Julesssss, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@eVoloshchak, @Julesssss, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Julesssss]

@adelekennedy could you please double the payment amount? Thanks!

[Julesssss]

Hey @adelekennedy, bumping for bounty increase

[melvin-bot[bot]]

@eVoloshchak, @Julesssss, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[adelekennedy]

doubled the price - @Julesssss @roryabraham from the comment here is this still an external issue? What are our next steps?

[Julesssss]

Hey @adelekennedy, for now we're awaiting a proposal. Once we're settled on a solution the cause will likely become apparent. Then we'll make sure that tests are added in the PR.

[adelekennedy]

kk - will keep doubling this!

[adelekennedy]

we just hit the doubling mark

[melvin-bot[bot]]

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[adelekennedy]

@isabelastisser reassigning to you while I'm gone to keep it doubling!

[Julesssss]

@adelekennedy I reproduced issue, may I work for this issue too?

@richdev1124 you'll need to submit a proposal first, and then get selected. Please see our Contributing guides for more help.

[huzaifa-99]

Are we handling accessibility (like using escape key)? I asked about this here

[s77rt]

Proposal

diff --git a/node_modules/react-native-modal/dist/modal.d.ts b/node_modules/react-native-modal/dist/modal.d.ts
index b63bcfc..bd6419e 100644
--- a/node_modules/react-native-modal/dist/modal.d.ts
+++ b/node_modules/react-native-modal/dist/modal.d.ts
@@ -161,6 +161,7 @@ export declare class ReactNativeModal extends React.Component<ModalProps, State>
     getDeviceHeight: () => number;
     getDeviceWidth: () => number;
     onBackButtonPress: () => boolean;
+    handleEscape: (e: KeyboardEvent) => void;
     shouldPropagateSwipe: (evt: GestureResponderEvent, gestureState: PanResponderGestureState) => boolean;
     buildPanResponder: () => void;
     getAccDistancePerDirection: (gestureState: PanResponderGestureState) => number;
diff --git a/node_modules/react-native-modal/dist/modal.js b/node_modules/react-native-modal/dist/modal.js
index 80f4e75..fe028ab 100644
--- a/node_modules/react-native-modal/dist/modal.js
+++ b/node_modules/react-native-modal/dist/modal.js
@@ -75,6 +75,13 @@ export class ReactNativeModal extends React.Component {
             }
             return false;
         };
+        this.handleEscape = (e) => {
+            if (e.key === 'Escape') {
+                if (this.onBackButtonPress() === true) {
+                    e.stopImmediatePropagation();
+                }
+            }
+        };
         this.shouldPropagateSwipe = (evt, gestureState) => {
             return typeof this.props.propagateSwipe === 'function'
                 ? this.props.propagateSwipe(evt, gestureState)
@@ -454,9 +461,15 @@ export class ReactNativeModal extends React.Component {
             this.open();
         }
         BackHandler.addEventListener('hardwareBackPress', this.onBackButtonPress);
+        if (Platform.OS === 'web') {
+            document?.body?.addEventListener?.('keyup', this.handleEscape, true);
+        }
     }
     componentWillUnmount() {
         BackHandler.removeEventListener('hardwareBackPress', this.onBackButtonPress);
+        if (Platform.OS === 'web') {
+            document?.body?.removeEventListener?.('keyup', this.handleEscape, true);
+        }
         if (this.didUpdateDimensionsEmitter) {
             this.didUpdateDimensionsEmitter.remove();
         }

Details

The issue is caused by a conflict between react-navigation and react-native-modal. react-navigation set a listener on keyup and captures the keyup for the Escape key to close the active drawer modal (chat in this case) and if so, it does not bubbles up thus stopping further events from execution including the react-native-modal. react-native-modal only uses BackHandler where react-navigation uses BackHandler and an extra keyup listener (the one just mentioned). In my proposal (patch), I added a similar listener of that of react-native, but this is not enough as the react-native listener is set first, the same issue will occur, that's why you can see a third parameter in the addEventListener and removeEventListener function calls, it's set to true to use capture instead of bubbling, which guarantees that key events will go first to the modal, and if so, we try to close the modal if it's open and close-able, if so we stop immediate propagation to prevent closing the chat drawer modal, else everything continues normally.

PS: Don't forget to clear the cache (rm -rf node_modules/.cache/) if you want to apply the patch. PS: I didn't submit a PR on react-native-modal because I think it's not their duty to follow react-navigation. I think a patch is the best thing to do here PS: The solution can be applied on the BaseDrawerNavigator level without the need for the patch, but it's gonna need an additional logic check (is there a modal open or not). Since this is only related to modals, applying the fix on react-native-modal is the way to go

This fixes modals not responding to Escape key, including the compose actions and the clipboard on the about section page.

https://user-images.githubusercontent.com/16493223/201496355-f83bd12f-1692-4b77-9407-fd0caec7f3bf.mp4

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[Julesssss]

Hey @s77rt. Thanks for the analysis and write-up.

The patch solution makes sense, but we avoid platform-specific logic, as explained here. I'd be happy to review an updated proposal that applies this rule.

---

Hey @huzaifa-99, we're definitely interested in solutions that align with accessibility. But we're not specifically focusing on this as a priority yet.

[s77rt]

Hi @Julesssss I know the rules, but this is more specific to react-native-modal, this is a patch and won't be a part of the Expensify App code.

Suppose that the patch was applied to react-native-modal and you use the new patched react-native-modal, are you breaking the rules? because react-native-modal use that platform specific code and so react-navigation. Are we breaking the rules? I don't think so. Same thing applied here.

I'm asking to reconsider your decision

[Julesssss]

Hey @s77rt. Good point, I'm actually not sure so I'm asking here in slack for others' opinions.

[eVoloshchak]

Looks like there's an opinion about whether we should apply our code styling to library patches

I would probably say it should be written using the code style of the authors since we should be trying to get patches merged upstream.

I'll review and test @s77rt's proposal tomorrow

[Julesssss]

@eVoloshchak yeah, that seems the correct plan to me 👍

[eVoloshchak]

The current behavior is a little different from the issue description, now after pressing enter popup just stays open and there is no way to close it.

https://user-images.githubusercontent.com/9059945/202514937-1f836321-712c-4407-bd84-70b74898948f.mov

@s77rt, I've tested your proposal, but the result is unchanged. Probably a fresh change to the navigation. Could you please retest pulling the latest sources?

[s77rt]

@eVoloshchak Looks like you didn't clear the cache.

[eVoloshchak]

You're right, weird, changes didn't take place until I cleared the cache and restarted the computer, even though it was hot reloading

@s77rt's proposal works well and looks good 🎀👀🎀 C+ reviewed!

@Julesssss, what should we do? Open a PR in react-native-modal or apply a patch?

[s77rt]

@eVoloshchak Changes to node_modules are not a part of the hotload process, you have to clear node_modules/.cache and restart the app.

[eVoloshchak]

Oh I see, thank you

[Julesssss]

Lets go with the patch. As mentioned above, it seems unlikely that this will be accepted as an upstream RNModal PR

[melvin-bot[bot]]

📣 @s77rt You have been assigned to this job by @Julesssss! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[s77rt]

Applied in Upwork. (+50% Bonus) PR is ready.

[isabelastisser]

Hey @Julesssss, did you create this job and hire @s77rt and @eVoloshchak in Upwork? Can you please share the job status links to this issue? Please let me know what steps I need to take moving forward. Thanks!

[Julesssss]

Hey @Julesssss, did you create this job and hire @s77rt and @eVoloshchak in Upwork?

Hey @isabelastisser, I've not created a job in UpWork as usually, the CM team does this.

[s77rt]

@isabelastisser @Julesssss https://github.com/Expensify/App/issues/11930#issuecomment-1284460120

[Julesssss]

Ah thanks. @isabelastisser do you have access to that UpWork job?

[isabelastisser]

I do, @Julesssss ! @s77rt , can you please apply for that job at Upwork? Let me know when you do and I should be able to hire you there.

[s77rt]

@isabelastisser I did already https://github.com/Expensify/App/issues/11930#issuecomment-1320006228

[Julesssss]

Merged, awaiting deployment to prod