[$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 [email protected] 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): [email protected] 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?

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/01839aaf-6a53-4682-a3c6-03a91b06d71a

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-04-08

[melvin-bot[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[bot]]

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

[melvin-bot[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?

https://github.com/Expensify/App/blob/dd30d137b485482bb8e4c61700b58d2e03683124/src/components/ConfirmContent.tsx#L140-L181

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. https://github.com/Expensify/App/blob/dd30d137b485482bb8e4c61700b58d2e03683124/src/pages/home/report/ReportActionItemMessageEdit.tsx#L456

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.

https://github.com/Expensify/App/assets/27822551/73444e4b-1079-466d-8e39-d71e302b5046

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: https://github.com/Expensify/App/blob/89b9cef1f4d6ee61473181431fc76d78d7d1bc11/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L278-L279 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[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 https://github.com/Expensify/App/issues/33201

[isabelastisser]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@isabelastisser @shubham1206agra this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

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

[isabelastisser]

Bump @shubham1206agra for an update! I will DM you for visibility.

[shubham1206agra]

@Krishna2323 and @BhuvaneshPatil Can you both provide me with test branches? I want to test both solutions first.

[shubham1206agra]

@nkdengineer Your proposal lacks why the focus sticks on the Cancel button.

[nkdengineer]

@shubham1206agra I mentioned why the focus sticks on the Cancel button in the below section:

[shubham1206agra]

Yeah, but it doesn't explain why the focus sticks.

[nkdengineer]

@shubham1206agra focusLastDescendant is function to focus on the last descendant component of the modal, in this case the last descendant is the "Cancel" button. Calling focusLastDescendant leads to the focus sticks on the Cancel button.

Feel free to provide feedback if my response is unclear. Thanks

[shubham1206agra]

You still didn't understood what I am asking. I understand how cancel button gets focused. But why am I unable to change the focus by up/down arrow keys?

[nkdengineer]

change the focus by up/down arrow keys?

Sorry because misunderstood your thought. I think we do not have this feature. We just can change the focus by using the Tab key.

[shubham1206agra]

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.

Then why did you say this? If the feature was not implemented.

[Krishna2323]

@shubham1206agra, test branch here

[BhuvaneshPatil]

@shubham1206agra Branch