[HOLD for payment 2023-09-04] [$1000] App changes focus from edit message composer to send message composer when mark as unread

[kavimuru]

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:

1. Open any report
2. Send any message
3. Hover message and click edit
4. Right click on report on LHN => mark as unread
5. Observe that the app loses focus on edit composer and switches to focus send message composer

Expected Result:

App still keeps focus on the message being edited

Actual Result:

App loses focus on edit composer and switches to focus send message composer

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.47-2
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-07-30.at.13.49.59.mov

Recording.1407.mp4

Expensify/Expensify Issue URL:
Issue reported by: @namhihi237
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690700101909109

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012c0c567372dabdf6
• Upwork Job ID: 1686829305551839232
• Last Price Increase: 2023-08-09
• Automatic offers:
• Ollyws | Reviewer | 26050166
• tienifr | Contributor | 26050168
• namhihi237 | Reporter | 26050169

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[rayane-d]

Proposal

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

The application changes focus from the edit message composer to the send message composer when an action ("Mark as Read", "Mark as Unread", "Pin", or "Unpin") is triggered in the Left Hand Navigation (LHN) report context menu.

What is the root cause of that problem?

In the file App/src/pages/home/report/ContextMenu/ContextMenuActions.js, the onPress functions for the "Mark as Read", "Mark as Unread", "Pin", and "Unpin" actions in the context menu are causing the focus shift. When a report is marked as read/unread or pinned/unpinned, the context menu is hidden, and the focus is set to the ReportActionCompose component (the message composer), regardless of whether a message was being edited previously.

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

Lines 253 to 266 in eb02006

	{
	isAnonymousAction: false,
	textTranslateKey: 'reportActionContextMenu.markAsRead',
	icon: Expensicons.Mail,
	successIcon: Expensicons.Checkmark,
	shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat, isUnreadChat) => type === CONTEXT_MENU_TYPES.REPORT && isUnreadChat,
	onPress: (closePopover, {reportID}) => {
	Report.readNewestAction(reportID);
	if (closePopover) {
	hideContextMenu(true, ReportActionComposeFocusManager.focus);
	}
	},
	getDescription: () => {},
	},

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

Lines 237 to 251 in eb02006

	{
	isAnonymousAction: false,
	textTranslateKey: 'reportActionContextMenu.markAsUnread',
	icon: Expensicons.Mail,
	successIcon: Expensicons.Checkmark,
	shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat, isUnreadChat) =>
	type === CONTEXT_MENU_TYPES.REPORT_ACTION || (type === CONTEXT_MENU_TYPES.REPORT && !isUnreadChat),
	onPress: (closePopover, {reportAction, reportID}) => {
	Report.markCommentAsUnread(reportID, reportAction.created);
	if (closePopover) {
	hideContextMenu(true, ReportActionComposeFocusManager.focus);
	}
	},
	getDescription: () => {},
	},

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

Lines 311 to 324 in eb02006

	{
	isAnonymousAction: false,
	textTranslateKey: 'common.pin',
	icon: Expensicons.Pin,
	shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat) =>
	type === CONTEXT_MENU_TYPES.REPORT && !isPinnedChat && !ReportUtils.isMoneyRequestReport(reportID),
	onPress: (closePopover, {reportID}) => {
	Report.togglePinnedState(reportID, false);
	if (closePopover) {
	hideContextMenu(false, ReportActionComposeFocusManager.focus);
	}
	},
	getDescription: () => {},
	},

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

Lines 325 to 338 in eb02006

	{
	isAnonymousAction: false,
	textTranslateKey: 'common.unPin',
	icon: Expensicons.Pin,
	shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat) =>
	type === CONTEXT_MENU_TYPES.REPORT && isPinnedChat && !ReportUtils.isMoneyRequestReport(reportID),
	onPress: (closePopover, {reportID}) => {
	Report.togglePinnedState(reportID, true);
	if (closePopover) {
	hideContextMenu(false, ReportActionComposeFocusManager.focus);
	}
	},
	getDescription: () => {},
	},

This is the key line responsible for the focus shift:

hideContextMenu(true, ReportActionComposeFocusManager.focus);

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

To resolve this issue, we should modify the onPress functions for the "Mark as Read", "Mark as Unread", "Pin", and "Unpin" actions to avoid setting the focus to the message composer. we can simply pass undefined as the second parameter to the hideContextMenu function. This would ensure that the focus isn't shifted from the edit composer:

onPress: (closePopover, {reportID}) => { 
    // ... action code
    if (closePopover) { 
        hideContextMenu(true, undefined); 
    } 
}, 

My proposal targets this specific undesired behavior of unintended focus shift. By passing 'undefined' to the 'hideContextMenu' function, we ensure that the current focus state is retained and does not automatically shift to the message composer when the user performs one of the specified actions. If the user is editing a message, it keeps the focus on the edit field; if a user is composing a new message, it keeps the focus on the message composer; and if the user is not focused on any input, it does not impose a forced focus. This approach respects the user's current context and maintains their interaction flow, which results in an improved user experience.

Result:

2023-07-30.23-02-16.mp4

What alternative solutions did you explore? (Optional)

None.

[dummy-1111]

Proposal

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

App changes focus from edit message composer to main composer when mark as unread in LHN

What is the root cause of that problem?

When pressing mark as unread, ReportActionComposeFocusManager.focus function is called as you can see here.

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

Line 247 in eb02006

hideContextMenu(true, ReportActionComposeFocusManager.focus);

We set this callback function when we open any chat

App/src/pages/home/report/ReportActionCompose.js

Lines 248 to 253 in eb02006

	this.unsubscribeNavigationFocus = this.props.navigation.addListener('focus', () => {
	KeyDownListener.addKeyDownPressListner(this.focusComposerOnKeyPress);
	this.setUpComposeFocusManager();
	});
	KeyDownListener.addKeyDownPressListner(this.focusComposerOnKeyPress);
	this.setUpComposeFocusManager();

But we setup focus callback only on main composer. So the focus function of the main composer would be called regardless of which is focused at the time menu is selected.

The root cause of this issue is that we only setup focus callback only for the main composer.

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

The focus manager should be setup for any composer with focus. This is reasonable and consistent to manager's purpose

Logic:

• Manage main composer(ReportActionCompose) and editing composer(ReportActionItemMessageEdit) separately
• When set callback for editing composer, save reportActionID as well
• When focus is called, if there is a callback for editing composer, call that callback. If there is no callback for editing composer, call main composer callback if exists.
• Set callback on focus event and clear calback on unmount event for both main composer and editing composer. On unmount for editing callback, compare the reportActionID first and clear if its reportActionID is equal to reportActionID stored in the manager.

This works as expected

Result

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012c0c567372dabdf6

[melvin-bot]

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

[melvin-bot]

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

[maddylewis]

added external label so that C+ can review existing proposals

[Ollyws]

Thank you both for your proposals.

@rayane-djouah, While it does seem that returning focus does indeed work correctly after selecting from the LHN context menu without using the focus callback, this isn't the case for report screen right-click context menu, removing the callback breaks this as it also uses ContextMenuActions.
I would be interested to know what causes this differing focus behaviour (without the callback) seeing as they are both using the same component (PopoverReportActionContextMenu).

@s-alves10, I agree with your analysis, it does make sense that the focus callback should be set for the last used textInput rather than always targeting the main composer.
However, your proposal also seems to break the focus when selecting from the report screen context menu, it would also break focusing the main composer after we cancel the comment editor, as we would be calling the focus callback for the (now closed) edit composer when we delete the draft.

[rayane-d]

@rayane-djouah, While it does seem that returning focus does indeed work correctly after selecting from the LHN context menu without using the focus callback, this isn't the case for report screen right-click context menu, removing the callback breaks this as it also uses ContextMenuActions.
I would be interested to know what causes this differing focus behaviour (without the callback) seeing as they are both using the same component (PopoverReportActionContextMenu).

@Ollyws I believe that the observed behavior is expected, given the current state of our application. For instance, when actions such as 'Mark as Read/Unread' are triggered from the MiniContextMenu, when the EmojiPicker is opened and subsequently closed, or even when we react to a message, the focus is not explicitly set.

focus.mp4

In my view, this behavior is consistent and logical: interactions with other ReportActionItems or interactions with reports in the Left Hand Navigation (LHN) should not force a focus state. This approach maintains the focus state in the context that is most relevant to the user's current interaction.

Furthermore, we have the option to disable the automatic focus shift for actions such as 'Copy to Clipboard' and 'Delete Comment' from the report context menu. By doing so, we ensure the focus remains where the user anticipates it to be.

[tienifr]

Proposal

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

App loses focus on edit composer and switches to focus send message composer

What is the root cause of that problem?

We're only focusing on the main composer when closing the context menu here, while if we're editing the message, we want to focus on the message edit input instead.

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

We need to make sure the ReportActionComposeFocusManager can handle both:

• general composer (which includes the edit composer): If this is the last composer that was focused on, we want to refocus on this
• main composer: If there's no last composer that was focused on before, this should be the fallback composer that we need to focus

1. Add a new mainComposerFocusCallback variable, add new param isMainComposer to onComposerFocus, clear which if true, the methods will operate on the mainComposerFocusCallback rather than the focusCallback
2. Update the ReportActionComposeFocusManager.focus method so that if the focusCallback does not exist, it will fallback to the mainComposerFocusCallback
3. In ReportActionCompose, set isMainComposer to true when calling ReportActionComposeFocusManager.onComposerFocus and ReportActionComposeFocusManager.clear
4. In Composer, when the input is focused, call the onComposerFocus (this is different from the logic in ReportActionCompose in that we should not use the || !this.props.isFocused condition, because it will always be false in the case of the general composer)

onFocus={(e) => {
    ReportActionComposeFocusManager.onComposerFocus(() => { 
         if (!this.willBlurTextInputOnTapOutside) { 
             return; 
         } 
  
         this.textInput.focus();
     }); 
    if (this.props.onFocus) {
        this.props.onFocus(e);
    }
}}

5. Add ReportActionComposeFocusManager.clear(); before this line

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

   Line 206 in e2694f5

   ReportActionComposeFocusManager.focus();

   to make sure the general composer callback is clear when we exit from draft mode, so that it will fallback correctly to the main composer.

What alternative solutions did you explore? (Optional)

NA

[dummy-1111]

Proposal

Updated

@Ollyws
I updated the logic slightly. It would fix all your concerns, I think

[Ollyws]

@tienifr, Thanks for your proposal. Having a general composer callback set with the onFocus with the ability to fall back to the main composer callback when required seems like a solid solution.

It generally seems to work well, however I noticed one small issue: Contrary to production, If you blur the composer by clicking a report-action then select an item from the LHN context-menu the composer will not be re-focused (although the callback will be called). This seems to be caused by PressableWithSecondaryInteraction.

[melvin-bot]

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

[maddylewis]

hiya @alexpensify - i am OOO until ~Aug 20.

• A C+ has already been assigned to the issue and is reviewing proposals
• This is the latest update - [HOLD for payment 2023-09-04] [$1000] App changes focus from edit message composer to send message composer when mark as unread #23898 (comment)
• Once we decide on a proposal, the C+ will hire the contributor and this will be like any other BZ issue 👍

[melvin-bot]

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

Offer link
Upwork job

[tienifr]

PR ready for review #24481.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @tienifr got assigned: 2023-08-10 13:43:04 Z
• when the PR got merged: 2023-08-22 10:03:37 UTC
• days elapsed: 7

On to the next one 🚀

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

• Fix: Edit composer moves focus to main composer after press LHN menu item #24481

If no regressions arise, payment will be issued on 2023-09-04. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @Ollyws requires payment offer (Reviewer)
• @tienifr requires payment offer (Contributor)
• @namhihi237 requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

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

[maddylewis]

@Ollyws - can you confirm what needs to be done in this checklist? thanks! #23898 (comment)

[maddylewis]

@namhihi237 / @tienifr are paid. offer sent to @Ollyws.

[Ollyws]

No PR is responsible for this bug as it was more of a feature that wasn't previously considered and as focusing the edit composer is not really important for any user user flow or very impactful I don't think a regression test is particularly useful here.

[maddylewis]

everyone is paid - closing!