[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] Compose box - Browser's back arrow do not focus composer if first image preview open

[lanitochka17]

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: 1.4.74-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: N/A
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: upload multiple images in chat

1. Open the first uploaded image
2. Click the "Back" button in the browser

Expected Result:

Composer gets focused

Actual Result:

Last uploaded image preview opened

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

Bug6481495_1715770820384.multiple_image.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013929a859819a7038
• Upwork Job ID: 1792347976114130945
• Last Price Increase: 2024-05-27
• Automatic offers:
• DylanDylann | Contributor | 0
• tsa321 | Contributor | 102496699

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013929a859819a7038

[melvin-bot]

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

[jliexpensify]

Can repro, adding External and probably fits into VSB best?

[tsa321]

Proposal

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

Browser back button open other images when user opens first attachment image in attachment view (and won't close attachment view) and press the back button.

What is the root cause of the problem?

In here:

App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

Lines 152 to 157 in e8ae3c5

	beforeRemove: () => {
	// Clear search input (WorkspaceInvitePage) when modal is closed
	SearchInputManager.searchInput = '';
	Modal.setModalVisibility(false);
	Modal.willAlertModalBecomeVisible(false);
	},

When back button is pressed, the beforeRemove listener is called immediately even though the modal is not completely hidden yet. The code above will set modal visibility to false too early and will make:

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx

Lines 283 to 284 in e8ae3c5

	const shouldAutoFocus =
	!modal?.isVisible && isFocused && (shouldFocusInputOnScreenFocus || (isEmptyChat && !ReportActionsUtils.isTransactionThread(parentReportAction))) && shouldShowComposeInput;

shouldAutoFocus to be true. This will make RNMarkdownTextInput:

App/src/components/Composer/index.tsx

Line 324 in e8ae3c5

<RNMarkdownTextInput

autofocus to be true.
This will make composer immediately focused even though the modal is not completelly hidden(because there is some animation). The trap focuse in react native modal will detect if focus is outside modal and will make focus back to element inside the modal, and here will eventually make the image changes to other/ next images.
More context is in here:

#27237 (comment)

More info about image changes in attachment view

@DylanDylann Focus event catch by react native modal ModalFocusTrap is in here:

https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/Modal/ModalFocusTrap.js#L137

The function of this file based on the comment is to trap focus so the focus won't leave modal, if the focus is outside of modal, the trap focus will focus last element inside modal.

The code flow is: Composer focused (from autofocus of RNMarkdownTextInput)-> focus event triggered -> catch by trapFocus (which will try to focus child or last descendant element of the modal -> trap focus looping through descendant element -> until last element of the initial rendered image (initialNumToRender is 3 in AttachmentCarousel) list reached :
The trapFocus try to focus the element of the attachment list will cause onViewableItemsChanged will be executed because the focus will make the displayed image slide(changes), hence will execute handler in:

App/src/pages/home/report/ReportAttachments.tsx

Lines 21 to 25 in 8375abe

	const onCarouselAttachmentChange = useCallback(
	(attachment: Attachment) => {
	const routeToNavigate = ROUTES.REPORT_ATTACHMENTS.getRoute(reportID, String(attachment.source));
	Navigation.navigate(routeToNavigate);
	},

which is navigate back to the attachment modal.

Focus event catch by react native modal ModalFocusTrap is in here:

https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/Modal/ModalFocusTrap.js#L137

The function of this file based on the comment is to trap focus so the focus won't leave modal, if the focus is outside of modal, the trap focus will focus last element inside modal.

The code flow is: Composer focused (from autofocus of RNMarkdownTextInput)-> focus event triggered -> catch by trapFocus (which will try to focus child or last descendant element of the modal -> trap focus looping through descendant element -> until last element of the initial rendered image (initialNumToRender is 3 in AttachmentCarousel) list reached :
The trapFocus try to focus the element of the attachment list will cause onViewableItemsChanged will be executed because the focus will make the displayed image slide(changes), hence will execute handler in:

App/src/pages/home/report/ReportAttachments.tsx

Lines 21 to 25 in 8375abe

	const onCarouselAttachmentChange = useCallback(
	(attachment: Attachment) => {
	const routeToNavigate = ROUTES.REPORT_ATTACHMENTS.getRoute(reportID, String(attachment.source));
	Navigation.navigate(routeToNavigate);
	},

which is navigate back to the attachment modal.

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

So basically we prevent set modal visibility too early, in here we should add a condition:

App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

Lines 155 to 156 in e8ae3c5

	Modal.setModalVisibility(false);
	Modal.willAlertModalBecomeVisible(false);

the code could be:

if (Modal.areAllModalsHidden()) {
    Modal.setModalVisibility(false);
    Modal.willAlertModalBecomeVisible(false);
}

This will check whether all modals is hidden completely.
Then the set of setModalVisibility and willAlertModalBecomeVisible are already exist in BaseModal - hideModal:

App/src/components/Modal/BaseModal.tsx

Line 76 in e8ae3c5

const hideModal = useCallback(

The modal already use it and so the if check above is sufficient to fix this issue.

To solve the closed issue that reappears above, in baseModal, we should call hide modal only in here:

App/src/components/Modal/BaseModal.tsx

Line 244 in e8ae3c5

<ModalContent onDismiss={handleDismissModal}>

so we add hideModal inside handleDismissModal then remove onModalHide and onDismiss property of ReactNativeModal. This is because many types of modal (whether the modal support onModalHide or not) used in the app and put the hideModal call in here will make sure the hideModal is executed once.

Result:

back.button.issue.fix.mp4

Fix result for the other issue that reappears:

other.issue.fix.result.mp4

[arjun-dureja]

Proposal

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

When an image attachment is opened inside a chat, pressing the browser back button will result in another image displaying, rather than being navigated back to the chat.

What is the root cause of that problem?

Inside the AttachmentCarousel component, the updatePage function is unexpectedly being called when the browser back button is pressed (due to how onViewableItemsChanged works on FlatList). This causes onNavigate to be called with the first attachment in the viewable items, which then navigates the user to a different attachment.

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

We should detect when the back button is pressed (potentially using a ref) and then return early inside the updatePage function. This will ensure that we're not unnecessarily fetching a different image and navigating the user to it. It will allow the user to go back to the chat

What alternative solutions did you explore? (Optional)

[CleverWolf1220]

Proposal

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

Compose box - Browser's back arrow do not focus composer if first image preview open

This issue is reproducible for video attachments as well.

What is the root cause of that problem?

In FlatList component of AttachmentCarousel, onViewableItemsChanged is called when pressing browser back button without user interection.

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

We should add waitForInteraction: true in viewabilityConfig

What alternative solutions did you explore? (Optional)

[melvin-bot]

📣 @CleverWolf1220! 📣
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>

[thesahindia]

@jliexpensify, I won't be able to review this soon. Please reassign.

[DylanDylann]

I would love to take over this issue as C+

[melvin-bot]

📣 @DylanDylann 🎉 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 📖

[jliexpensify]

All yours @DylanDylann!

EDIT: Still considering proposals!

[DylanDylann]

@tsa321 Could you detail this point? Which logic does that?

The trap focuse in react native modal will detect if focus is outside modal and will make focus back to element inside the modal, and here will eventually make the image changes to other/ next images.

[DylanDylann]

@arjun-dureja Could you deep dive into this point and explain why the updatePage is called? Is there any reference, document that explain why onViewableItemsChanged is triggered?

due to how onViewableItemsChanged works on FlatList

cc @CleverWolf1220

[DylanDylann]

@CleverWolf1220 Why waitForInteraction will resolve this problem?

[tsa321]

@DylanDylann Focus event catch by react native modal ModalFocusTrap is in here:

https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/Modal/ModalFocusTrap.js#L137

The function of this file based on the comment is to trap focus so the focus won't leave modal, if the focus is outside of modal, the trap focus will focus last element inside modal.

The code flow is: Composer focused (from autofocus of RNMarkdownTextInput)-> focus event triggered -> catch by trapFocus (which will try to focus child or last descendant element of the modal -> trap focus looping through descendant element -> until last element of the initial rendered image (initialNumToRender is 3 in AttachmentCarousel) list reached :
The trapFocus try to focus the element of the attachment list will cause onViewableItemsChanged will be executed because the focus will make the displayed image slide(changes), hence will execute handler in:

App/src/pages/home/report/ReportAttachments.tsx

Lines 21 to 25 in 8375abe

	const onCarouselAttachmentChange = useCallback(
	(attachment: Attachment) => {
	const routeToNavigate = ROUTES.REPORT_ATTACHMENTS.getRoute(reportID, String(attachment.source));
	Navigation.navigate(routeToNavigate);
	},

which is navigate back to the attachment modal.

[tsa321]

@dangrous yes, my previous PR caused a regression. But in latest main, after bumping the latest version of react-native-live-markdown the regression issue and this issue are fixed.
Then I noticed this issue is also happens in edit message compose box. So in my newer PR, I am reverting my previous PR and fixing this same issue on edit compose box.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.81-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix back button arrow won't back when viewing attachment image and additional fixes on modals #42738

If no regressions arise, payment will be issued on 2024-06-18. 🎊

For reference, here are some details about the assignees on this issue:

• @tsa321 requires payment automatic offer (Contributor)
• @DylanDylann requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
• [@DylanDylann] 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:
• [@DylanDylann] 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:
• [@DylanDylann] Determine if we should create a regression test for this bug.
• [@DylanDylann] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

Thanks for confirming everyone - so given that this is a regression, but also isn't (?), how should we handle payments here? Is it $125, or are we keeping at $250?

[dangrous]

seems sorta weird that react-native-live-markdown would have fixed this - and the regression - though, right? Is there some other root cause? @DylanDylann and @tsa321 let me know what you think.

Not sure about payment, if we're able to still use this to fix a bug I think maybe it's fine?

[DylanDylann]

@tsa321 Could you detail the problem mentioned in the new PR? (please attach a video for new bugs)

[tsa321]

@DylanDylann same issue as this issue but in edit composer box

macos-web-d.mp4

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix back button arrow won't back when viewing attachment image and additional fixes on modals #42738

If no regressions arise, payment will be issued on 2024-06-20. 🎊

For reference, here are some details about the assignees on this issue:

• @tsa321 requires payment automatic offer (Contributor)
• @DylanDylann requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
• [@DylanDylann] 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:
• [@DylanDylann] 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:
• [@DylanDylann] Determine if we should create a regression test for this bug.
• [@DylanDylann] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[DylanDylann]

@jliexpensify Let's remove the awaiting payment label. The PR is in the reviewing stage

[DylanDylann]

@dangrous The original issue still reproduces in other places. Give me a bit of time to deep dive into the new PR

[DylanDylann]

@tsa321 I can't reproduce your bug

Screen.Recording.2024-06-17.at.18.37.26.mov

[tsa321]

@DylanDylann you are right. I think the behavior changed, maybe after FocusTrap implemented. Should I close my PR?

[jliexpensify]

@DylanDylann removed, does this mean that I'm not paying on the 20th? If so, will remove that from the title as well.

[DylanDylann]

Summary: we have worked on this issue for a long time. After the original PR was merged, It caused a regression then we reverted the PR. Fortunately, while creating a new PR, we discovered that this issue was fixed by the new FocusTrap implementation.

[DylanDylann]

@jliexpensify @dangrous I am unsure how we handle the compensation for this case. Maybe half of the compensation might be a decent solution

[jliexpensify]

Thanks for the summary. Hmm yeah, this is interesting - a regression would usually mean half the standard payout, which would be $125. But the solution wasn't actually used in the end.

I feel like we shouldn't compensate since another PR fixed the issue, but honestly I'm fine with $125 for each of you since there was some work involved. @dangrous , thoughts?

[dangrous]

So given that another PR fixed the issue, I think typically in case we don't move forward with payment. But would love to work with y'all on future issues!

[jliexpensify]

Thanks Daniel. I've cancelled the contracts and removed the job.