[$500] Chat - Pressing ESC closes the video preview modal instead of Download and Playback modal

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.43-2
Reproducible in staging?: y
Reproducible in production?: new feature
Issue reported by: Applause - Internal Team

Action Performed:

1. Click on "Add attachment"
2. Upload a video
3. Click on three dot then press ESC

Expected Result:

Pressing ESC should not close the video preview modal, it should only close the Download and Playback modal

Actual Result:

Pressing ESC closes the video preview modal instead of Download and Playback modal

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6385631_1708430465452.2024-02-20_14-37-14__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cb3dff1b828d5004
• Upwork Job ID: 1759958094772105216
• Last Price Increase: 2024-02-20
• Automatic offers:
• jjcoffee | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

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

[Julesssss]

Edge case, not a blocker

[hkopser99]

Hey @Julesssss if this is an edge case can we still review/submit proposals or will this be assigned down the road if/when it becomes a higher priority?

[Julesssss]

@hkopser99 yeah, my comment was just to remove the high priority label. THis will be made external 👍

[melvin-bot]

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

[melvin-bot]

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

[hkopser99]

Hi @jjcoffee

Proposal:

Please re-state the problem that we are trying to solve in this issue:

When a user presses esc instead of ending the video player preview during the video attachment upload flow, it escapes from the attachment upload completely.

What is the root cause of that problem?: Default escape key behavior escapes from the video preview modal and prevents user from proceeding with uploading the attachment rather than simply pausing the video preview.

What changes do you think we should make in order to solve the problem?

In BaseVideoPlayer.js I would implement logic to recognize an esc key press, override default behavior, and use a callback that would call pauseVideo() if isVideoPlaying is set to true.

What alternative solutions did you explore? (Optional):

Given existing functionality, this seems like the smoothest solution unless additional functionality is desired when a use presses esc on desktop browsers or MacOS App.

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pressing ESC closes both the video player popover and the video player preview modal.

What is the root cause of that problem?

Both video player preview modal (BaseModal) and video player popover (PopoverWithoutOverlay) catch the ESC event. PopoverWithoutOverlay listens for keydown, while BaseModal (react-native-modal) listens for keyup event.

App/src/components/PopoverProvider/index.tsx

Lines 61 to 72 in 74540f8

	useEffect(() => {
	const listener = (e: KeyboardEvent) => {
	if (e.key !== 'Escape') {
	return;
	}
	closePopover();
	};
	document.addEventListener('keydown', listener, true);
	return () => {
	document.removeEventListener('keydown', listener, true);
	};
	}, [closePopover]);

App/patches/react-native-modal+13.0.1.patch

Lines 21 to 38 in 74540f8

	+ 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)
	@@ -453,10 +460,18 @@ export class ReactNativeModal extends React.Component {
	if (this.state.isVisible) {
	this.open();
	}
	+ if (Platform.OS === 'web') {
	+ document?.body?.addEventListener?.('keyup', this.handleEscape, true);
	+ return;
	+ }

In react-native-modal, we prevent the propagation when a modal handles the ESC, but we didn't do that for PopoverWithoutOverlay, so the event is propagated to all views.

What changes do you think we should make in order to solve the problem?

We should follow the same pattern we already have in react-native-modal, that is to stop the propagation.

if (closePopover()) {
    e.stopImmediatePropagation();
}

closePopover will return true if it passes the early return.

App/src/components/PopoverProvider/index.tsx

Lines 24 to 32 in 74540f8

	const closePopover = useCallback((anchorRef?: RefObject<View | HTMLElement>) => {
	if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) {
	return;
	}
	activePopoverRef.current.close();
	activePopoverRef.current = null;
	setIsOpen(false);
	}, []);

However, this still won't work because PopoverWithoutOverlay and BaseModal listen for different events (mentioned on the root cause). We should decide whether we want to use keyup or keydown.

What alternative solutions did you explore? (Optional)

Use BaseModal instead of PopoverWithoutOverlay by removing withoutOverlay.

App/src/components/VideoPopoverMenu/index.js

Lines 28 to 37 in 74540f8

	return (
	<PopoverMenu
	onClose={hidePopover}
	onItemSelected={hidePopover}
	isVisible={isPopoverVisible}
	anchorPosition={anchorPosition}
	menuItems={menuItems}
	withoutOverlay
	/>
	);

[jjcoffee]

Happy that @bernhardoj's proposal identifies the correct root cause and that the solution fits decently with it. I think it makes sense to switch to keyup to keep things consistent with the BaseModal, unless anyone has reasons to believe keydown is better?

@hkopser99 Thanks for your proposal! Unfortunately it's lacking somewhat in details, and the solution proposed doesn't match the expected result (we don't want to pause the video). Please also make sure to use the proposal template exactly as it appears (don't strip the formatting), as this makes it easier for everyone to parse your proposal quickly. 🙏

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[hkopser99]

@jjcoffee Thanks for reviewing and the feedback. I will do better next time and will use the exact proposal template.

[Christinadobrzyn]

@Julesssss would you be able to review the decision here - #36902 (comment) when you have a moment

[Julesssss]

Yep, I missed this somehow. Done!

[melvin-bot]

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[bernhardoj]

PR is ready

cc: @jjcoffee

[Rajat1120]

Now, it doesn't even show the preview of the video

Screen.Recording.2024-02-23.at.10.27.24.PM.mp4

[melvin-bot]

📣 @Rajat1120! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[Rajat1120]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/rajatkumar16

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[jjcoffee]

This hit production 2 weeks ago.

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - this was a latent bug in PopoverWithoutOverlay that the video player implementation brought to light
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug. Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Desktop only:

1. Open any chat
2. Add a new video attachment. Verify the attachment preview is shown.
3. On the video player control, press the three dots button. Verify the video player popover is shown
4. Press ESC
5. Verify the video player popover is closed
6. Press ESC again
7. Verify the attachment preview is closed

Do we agree 👍 or 👎

[jjcoffee]

@Christinadobrzyn Checklist complete! This one's due payment, the automation just looks to have failed.

[Christinadobrzyn]

Ah thanks for moving this along @jjcoffee

Payouts due:

Contributor: $500 @bernhardoj (paid in Upwork - https://www.upwork.com/nx/wm/workroom/36436009/overview)
Contributor+: $500 @jjcoffee (paid in Upwork - https://www.upwork.com/nx/wm/workroom/36436097/overview)

Upwork job is here.

Regression test: https://github.com/Expensify/Expensify/issues/378294

Closing! let me know if I missed anything!