App
App copied to clipboard
Expensify
[$1000] [Bug] Pressing escape when a popover is open closes the chat and the page gets stuck in the middle - reported by @Puneet-here
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
- Decrease the screen size
- Go to any chat
- Click on actions (the :heavy_plus_sign: sign at the left side of message input)
- 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
Triggered auto assignment to @adelekennedy (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
Current assignee @Julesssss is eligible for the External assigner, not assigning anyone new.
Also the issue is found on About section --> Copy To Clipboard Popover.
@eVoloshchak, @Julesssss, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@eVoloshchak, @Julesssss, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!
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.
@eVoloshchak, @Julesssss, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!
@eVoloshchak, @Julesssss, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@eVoloshchak, @Julesssss, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
doubled the price - @Julesssss @roryabraham from the comment here is this still an external issue? What are our next steps?
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.
Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
@isabelastisser reassigning to you while I'm gone to keep it doubling!
@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.
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
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.