[HOLD for payment 2024-03-20] [$500] Chat - Two Modals Open Simultaneously During Text Editing

[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: 1.4.40-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1, Go to the conversation.
2, Write a message.
3, Hover over the message, click "more," modal opens.
4, Press the up arrow key, enabling editing.
5, Clear all text and press Enter.
6, Notice two modals open; the first should close before the second, as when clicking "save."

Expected Result:

Upon pressing Enter, the first opened modal should close.

Actual Result:

Two models open simultaneously, and the first one does not close upon pressing Enter.

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

Bug6377306_1707774067269.Screen_Recording_2024-02-12_at_11.58.05_AM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cf48132d330e8d95
• Upwork Job ID: 1757159506265567232
• Last Price Increase: 2024-02-26
• Automatic offers:
• hoangzinh | Reviewer | 0
• jeremy-croff | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[LouzirMohamedAziz]

Proposal

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

Two Modals Open Simultaneously During Text Editing

What is the root cause of that problem?

missing action to hide the opened modals before calling the showDeleteModal() Within the function ReportActionItemMessageEdit at: 318
src/pages/home/report/ReportActionItemMessageEdit.tsx as shown in the following condition:

    // When user tries to save the empty message, it will delete it. Prompt the user to confirm deleting.
    if (!trimmedNewDraft) {
        textInputRef.current?.blur();
        ReportActionContextMenu.showDeleteModal(
            reportID,
            action,
            true,
            deleteDraft,
            // eslint-disable-next-line @typescript-eslint/no-misused-promises
            () => InteractionManager.runAfterInteractions(() => textInputRef.current?.focus()),
        );
        return;
    }

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

add the missing call the hideContextMenu() to hide all the active menus before performing showDeleteModal() as follow:

    // When user tries to save the empty message, it will delete it. Prompt the user to confirm deleting.
    if (!trimmedNewDraft) {
        textInputRef.current?.blur();
        ReportActionContextMenu.hideContextMenu();
        ReportActionContextMenu.showDeleteModal(
            reportID,
            action,
            true,
            deleteDraft,
            // eslint-disable-next-line @typescript-eslint/no-misused-promises
            () => InteractionManager.runAfterInteractions(() => textInputRef.current?.focus()),
        );
        return;
    }

Results:

MacOS: Chrome / Safari tests:

0213.mov

MacOS: Desktop tests:

0213.1.mov

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[LouzirMohamedAziz]

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

[melvin-bot]

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

[LouzirMohamedAziz]

Proposal

[Updated] #36381 (comment)

[jeremy-croff]

Proposal

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

We show two open popovers in this experience

What is the root cause of that problem?

We are not unfocusing the composer when clicking the Context Menu three dots item. So composer keeps focus, and listens to the keyboard events.

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

I reviewed what we do when we add an attachment. On that button press we explicitly blur the Composer.
We can do the same for the context menu item.
On the click of the menu item here:

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Line 474 in 1f1867a

onPress: (closePopover, {openOverflowMenu, event}) => {

if we call:
ReportActionComposeFocusManager?.composerRef?.current?.blur();
We will take the focus away from the composer.
And now the keyboard events will work on the popover menu items.

What alternative solutions did you explore? (Optional)

UPDATES:

After amazing collaboration with @hoangzinh and insights from @aimane-chnaif
We got down to the root cause and altered the proposal.

As per #36381 (comment) and #36381 (comment)

We should:
in the menu item action object, we can add a prop for shouldPreventDefaultEvent: false
we can pass it through until it reaches BaseMiniContextMenuItem
we only call e.preventDefault if shouldPreventDefaultEvent is true here
shouldPreventDefaultEvent defaults to true if it's not passed from other actions to maintain the existing behaviour

Additional RCA from #36381 (comment)
identified that the blur is not propagating because of existing code that prevents it.
It also identified that the edit composer has a bug in which it does allow blur to propagate.

This RCA helps identify that we shouldn't force calling a blur, but rather alter the existing logic to let the event listeners only propagate for certain actions.

[askavyblr]

Proposal

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

Two modals open simultaneously, and the first one does not close upon pressing Enter.

What is the root cause of that problem?

Action is missing to hide the opened modals before calling the Delete Modal

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

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx

Lines 271 to 280 in 0593b45

	const showDeleteModal: ReportActionContextMenu['showDeleteModal'] = (reportID, reportAction, shouldSetModalVisibility = true, onConfirm = () => {}, onCancel = () => {}) => {
	onCancelDeleteModal.current = onCancel;
	onComfirmDeleteModal.current = onConfirm;
	reportIDRef.current = reportID;
	reportActionRef.current = reportAction;
	setShouldSetModalVisibilityForDeleteConfirmation(shouldSetModalVisibility);
	setIsDeleteCommentConfirmModalVisible(true);
	};

Need to add setIsPopoverVisible(false);

**What alternative solutions did you explore? (Optional)

modal.webm

[CortneyOfstad]

@hoangzinh any thoughts on the proposals above? Thanks!

[hoangzinh]

@CortneyOfstad I'm checking the expected behavior here. At the moment, when we tap on the three-dot menu here, the composer is still focused. It makes the keyboard navigation in the three-dot menu is not working as well.

Screen.Recording.2024-02-15.at.08.40.06.mov

I think the composer should be unfocused when we tap on the three-dot menu. Like we tap in the three-dot menu of the header. What do you think?

Screen.Recording.2024-02-15.at.08.44.18.mov

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[hoangzinh]

Waiting for @CortneyOfstad thoughts here #36381 (comment)

[CortneyOfstad]

Sorry, was OoO yesterday! Reviewing now!

[CortneyOfstad]

@hoangzinh so I have been diving into this, and we don't unfocus the field when we click on anything else, but we do remove the cursor in other instances like clicking the emoji option as below:

Do you feel that is a reasonable alternative? I feel that this is more in-line with other aspects of new.expensify, but interested to hear your thoughts!

[hoangzinh]

I think if there is a popup modal appears it should unfocus the main composer. For example: when a user taps on:

• The plus icon on the left of the composer
• Emoji icon as you screenshot
• Right-click to open the context menu

So in this case, if the user taps on the 3 dot menu to open a popup modal, I think we should unfocus the main composer.

[CortneyOfstad]

@hoangzinh I disagree, as we do not unfocus anything at this time. Other apps do not do this functionality, so I feel as though having it unfocus for the sake of unfocusing isn't worth the time.

However, if you feel strongly about this, I do recommend bringing it to the team to discuss as we can dive in further!

[hoangzinh]

Sure @CortneyOfstad

I feel as though having it unfocus for the sake of unfocusing isn't worth the time.

btw could you elaborate on this one? I'm unsure if I understand your point. Thanks.

[CortneyOfstad]

@hoangzinh No worries! I should have explained this a bit better. My reasoning for not unfocusing is that we do not do this with other aspects currently, so essentially having to change all of those actions to unfocus to make this situation work seems like not a high priority. I would rather we adjust this situation to match the others (not unfocusing, but rather just removing the cursor in the text field).

However, if you feel strongly that we should change the other aspects (emoji selection, etc) then I do recommend posting in #open-source for the team to discuss as a whole.

[hoangzinh]

I do recommend posting in #open-source for the team to discuss as a whole.

sure @CortneyOfstad, but before I do that, I would like to understand your opinion first.

not unfocusing, but rather just removing the cursor in the text field

I'm confusing this one. Doesn't removing the cursor in the text field mean unfocusing in this situation? I'm understanding they're the same. By unfocus the text field, we remove the cursor in this case.

if you feel strongly that we should change the other aspects (emoji selection, etc)

Nope, I don't. I would like to reparaphrase my thoughts here. I suggest we only change the behavior of the three-dot item in the context menu

Current: If the text field is focusing, when we tap on three-dot item in the context menu, the text field is still focusing although it's opening a modal. It's not align with other modal like FAB, plus button...

Screen.Recording.2024-02-24.at.11.43.01.mov

Suggest: we should unfocus (or remove the cursor) in the text field when we tap on three-dot item in the context menu

[hoangzinh]

Melvin, the proper solution is still in discussion

[jeremy-croff]

Hi @jeremy-croff, I think your solution works. But I'm comparing it with our existing pattern in Expensify codebase. For example

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

Lines 440 to 443 in 4231ac9

	const relatedTargetId = event.nativeEvent?.relatedTarget?.id;
	if (relatedTargetId && [messageEditInput, emojiButtonID].includes(relatedTargetId)) {
	return;
	}

We check based on the target ID. Or here. Is it possible to apply this targetID approach in our case? What do you think about this targetID approach and your approach using additional props?

Will give an update later, had a couple minutes for initial thoughts but want to spend more time after my office work.

quick thoughts are that shouldPreventDefault prop is also a pattern used in places: https://github.com/search?q=repo%3AExpensify%2FApp+shouldPreventDefault&type=code

When reviewing which is better I want to look at the usage of the https://github.com/Expensify/App/blob/3e74ddf0d2b408faa270ff243cf333065b177a87/src/components/BaseMiniContextMenuItem.tsx#L49C13-L49C38 and if it makes sense to pass in IDs, and can be done so from the actual menu actions, cause otherwise it's a bit of a detached code that might be hard to trace.

[hoangzinh]

if it makes sense to pass in IDs, and can be done so from the actual menu actions, cause otherwise it's a bit of a detached code that might be hard to trace.

Agreed. Can you update your current proposal with your approach using additional props? Then go ahead with PR.

cc @bondydaa

[jeremy-croff]

Thank you @hoangzinh for this collaboration, and helping getting a deeper understanding of the problem with your guidance.

updated proposal

[bondydaa]

thanks for getting all that resolved, I had thought we might do that on the PR sorry, that's why I'd assigned before.

@jeremy-croff you're still going to spin up a PR right?

[jeremy-croff]

@bondydaa Yes, thank you!
Just opened the draft and testing. Will have the checklist completed in an hour or so.

[CortneyOfstad]

@jeremy-croff any update on the PR? Thanks!

[jeremy-croff]

@jeremy-croff any update on the PR? Thanks!

It's merged! 😄

[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.51-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix(36381): blur composer on edit menu action #37861

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

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

• @hoangzinh requires payment automatic offer (Reviewer)
• @jeremy-croff 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:

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

[CortneyOfstad]

@hoangzinh can you complete the checklist by EOD? Thanks!

[CortneyOfstad]

Payment Summary

• @hoangzinh — paid $500 via Upwork
• @jeremy-croff — paid $500 via Upwork

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: I don't think it's caused by any PR. It's just an adjustment in our expectation when tapping on the three-dot menu here.
• 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: I don't think we need a regression test here. Just a small UX twist here where we unfocus main chat when tap on three-dot menu

[CortneyOfstad]

Thanks @hoangzinh!