[HOLD for #42645] [$250] Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.61-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
Email or phone of affected tester (no customers): abebemiherat@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1, Write a comment and hover over it, then click edit
2, Clear the message and press the Enter key
3, Observe that the focus remains on the "Cancel" button, preventing the deletion of the comment using the Enter key

Expected Result:

The focus should not remain on the "Cancel" button when attempting to clear the message and save changes using the Enter key. The user should be able to delete the comment using the Enter key

Actual Result:

The focus remains on the "Cancel" button when attempting to clear the message and save changes using the Enter key. The user is unable to delete the comment using the Enter key

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

Bug6442203_1712579440754.edit.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0125e80c6474a3886e
• Upwork Job ID: 1777347428497317888
• Last Price Increase: 2024-06-04

[melvin-bot]

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

[lanitochka17]

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[ShridharGoel]

Proposal

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

Using up and down arrow keys doesn't change the button focus on confirm modal.

What is the root cause of that problem?

We don't support up and down arrow keys functionality in ConfirmContent.tsx.

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

Handle up and down keyboard keys like this:

const confirmButtonRef = useRef<HTMLButtonElement>(null);
const cancelButtonRef = useRef<HTMLButtonElement>(null);

useEffect(() => {
    const handleKeyDown = (event: KeyboardEvent) => {
        if (event.key === 'ArrowUp') {
            if (confirmButtonRef.current) {
                confirmButtonRef.current.focus();
            }
        } else if (event.key === 'ArrowDown') {
            if (cancelButtonRef.current) {
                cancelButtonRef.current.focus();
            }
        }
    };

    document.addEventListener('keydown', handleKeyDown);

    return () => {
        document.removeEventListener('keydown', handleKeyDown);
    };
}, []);

Then, pass the refs to the buttons.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0125e80c6474a3886e

[melvin-bot]

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

[nayabatir1]

Proposal

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

Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key

What is the root cause of that problem?

There no logic to handle keyboard input in ConfirmContent.tsx

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

App/src/components/ConfirmContent.tsx

Lines 140 to 181 in dd30d13

	{shouldStackButtons ? (
	<>
	<Button
	success={success}
	danger={danger}
	style={[styles.mt4]}
	onPress={onConfirm}
	pressOnEnter
	large
	text={confirmText || translate('common.yes')}
	isDisabled={isOffline && shouldDisableConfirmButtonWhenOffline}
	/>
	{shouldShowCancelButton && (
	<Button
	style={[styles.mt3, styles.noSelect]}
	onPress={onCancel}
	large
	text={cancelText || translate('common.no')}
	/>
	)}
	</>
	) : (
	<View style={[styles.flexRow, styles.gap4]}>
	{shouldShowCancelButton && (
	<Button
	style={[styles.noSelect, styles.flex1]}
	onPress={onCancel}
	text={cancelText || translate('common.no')}
	medium
	/>
	)}
	<Button
	success={success}
	danger={danger}
	style={[styles.flex1]}
	onPress={onConfirm}
	pressOnEnter
	text={confirmText || translate('common.yes')}
	isDisabled={isOffline && shouldDisableConfirmButtonWhenOffline}
	medium
	/>
	</View>

1. add event listener for key press
2. event listener should be added only when modal is visible and listener should be removed when modal is invisible
3. upon up and down arrow keypress button focus be changed accordingly
4. If shouldStackButtons props is true then stacked button should be focus and same for non stacked buttons.

Additionally

1. If top button is focused and up arrow key is pressed then focus should circle back to bottom button and same for down arrow key
2. We can also update styling of focused button to make it look more prominent

[Krishna2323]

Proposal

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

Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key

What is the root cause of that problem?

The issue happens because we set setShouldShowComposeInputKeyboardAware to true when the edit input gets blurred but we shouldn't call setShouldShowComposeInputKeyboardAware when we have delete modal open.

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

Line 456 in dd30d13

setShouldShowComposeInputKeyboardAware(true);

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

We should create a ref isDeleteModalActive and set it to true when before we call input blur and showDeleteModal and in onBlur callback we should check if showDeleteModal.current is true or not, if it is we will return without calling setShouldShowComposeInputKeyboardAware.

Result

Alternatively

We can use isPopoverVisible state from ReportActionContextMenu.

[BhuvaneshPatil]

Proposal

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

Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key

What is the root cause of that problem?

We don't have any event handlers for up and down arrow keys. That's why doesn't have focus change when we press up and down arrow keys.

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

We can make use of useKeyboardShortcut() and useArrowKeyFocusManager() hooks that have been used for the similar purpose (handling focus with cursors).
This involves three steps -

1. Making use of useArrowKeyFocusManager() to track the focused index.

    const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager(
        {
            maxIndex: 1,
            initialFocusedIndex: 0,
        }
    )

2. Use array of refs for storing the refs of two buttons. That we will add refs like following

ref={(ref) => {
    if (!ref) {
        return;
    }
    buttonRefs.current.push(ref);
}}

3. We can make use of useKeyboardshortcut() to listen to - arrow up and and arrow down shortcuts as below -

useKeyboardShortcut(
    CONST.KEYBOARD_SHORTCUTS.ARROW_UP,
    () => {
        if (!(focusedIndex > 0)) {
            return;
        }
        setFocusedIndex(focusedIndex - 1);
    }
);

useKeyboardShortcut(
    CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN,
    () => {
        if (focusedIndex === buttonRefs.current.length - 1) { 
            return; 
        }
        setFocusedIndex(focusedIndex + 1);
    }
);

Sure we can refactor conditions. With help of useKeyboardshortcut() we can avoid managing event listeners since it will do heavy lifting and and it's well tested.

Screen.Recording.2024-04-08.at.9.28.57.PM.mov

What alternative solutions did you explore? (Optional)

[nkdengineer]

Proposal

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

• Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key

What is the root cause of that problem?

• When we open modal, we have a logic to focus on its first descendant here:

            let hasFocused = focusFirstDescendant(trapElementRef.current);

• And we have a logic to check whether we should auto-focus on the main composer or not:
  

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

  Lines 278 to 279 in 89b9cef

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

  
  in this case, shouldAutoFocus is true at the first time because !modal?.isVisible is true. That leads to the main composer being focused for a while. I do not know why the main composer is focused if shouldAutoFocus is true but if you try removing this line, the bug is gone.
• In here we have a logic to handle focus event, so when the above main composer is focused, that event handle is triggered, and the logic to focus on the last descendant is call:

              hasFocused = focusLastDescendant(trapElementRef.current);

• Finally, the "Cancel" button is focused, leads to the bug.

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

1. We need to create a global variable, named willModalBecomeVisible to check if the modal is visible or not, so it will be updated immediately, rather than using the current modal?.isVisible from onyx.

let willModalBecomeVisible = false;

function setWillModalBecomeVisible (isVisible:boolean){
    willModalBecomeVisible = isVisible
}

2. Then, update this to:

            setWillModalBecomeVisible(true)
            return;

3. Then, update this condition to:

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

4. Finally, we need to reset setWillModalBecomeVisible(false) once modal hides

What alternative solutions did you explore? (Optional)

• We can update this to:

                            isModalHidden().then(()=> setShouldShowComposeInputKeyboardAware(true))

• In the above, isModalHidden is the promise that will be resolved if the modal is closed. Other reasons lead to the edit composer is blurred, isModalHidden is always resolved

[isabelastisser]

Hey @shubham1206agra, can you please review the proposals above? Thanks!

[melvin-bot]

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

[isabelastisser]

Bump @shubham1206agra to review the proposals above. I will DM you too for visibility. Thanks!

[shubham1206agra]

I remember arrow keys were supported earlier in this modal. Can someone check if that was the case.
Similar issue #33201

[isabelastisser]

@shubham1206agra can you post in open source to confirm? Thanks!

[melvin-bot]

@isabelastisser, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[isabelastisser]

Not overdue, we're still discussing this.

[isabelastisser]

Any updates, @shubham1206agra ? I can bump the slack discussions.

[shubham1206agra]

Any updates, @shubham1206agra ? I can bump the slack discussions.

@isabelastisser Please do that.

[melvin-bot]

@isabelastisser, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[isabelastisser]

@shubham1206agra, sorry for the delay in responding! I can't find the discussion in Slack. Can you please share the link so I can comment? Thanks!

[shubham1206agra]

https://expensify.slack.com/archives/C01GTK53T8Q/p1715341571609419

[isabelastisser]

Thanks, I just commented!

https://expensify.slack.com/archives/C01GTK53T8Q/p1716988518353269?thread_ts=1715341571.609419&cid=C01GTK53T8Q

[isabelastisser]

waiting for answer in slack.

[isabelastisser]

Hey @shubham1206agra, please confirm if you can still reproduce this, and if son, let's pick a proposal then. Thanks!

[melvin-bot]

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

[isabelastisser]

Bump @shubham1206agra on my comment above.

[shubham1206agra]

I can still repro the bug.
But looks like @bernhardoj has better idea over #42645 (comment)
So lets hold this issue there.

[isabelastisser]

@shubham1206agra any updates here? Thanks!

[shubham1206agra]

@isabelastisser, this seems to be fixed for me in production. You may close this issue.