[$4,000] Pressing down key on delete workspace popup focuses on the cancel button and then pressing enter key still deletes the workspace - reported by @adeel0202

[mvtglobally]

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. Go to settings
2. Go to any workspace
3. Click on the More icon
4. Select Delete Workspace option
5. Press down key and observe focus going to cancel button
6. Press enter key when the focus is on the cancel button

Expected Result:

When the focus is on the cancel button then pressing the enter key should just close the popup, or pressing the down key should not focus on the cancel button.

Actual Result:

Pressing the down key focuses on the cancel button and then pressing enter key deletes the workspace

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.1.41-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/156307909-c14700ef-ee45-45c7-8da8-24cf06422d97.mov

Expensify/Expensify Issue URL: Issue reported by: @adeel0202 Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644946895276159

Job Post: https://www.upwork.com/jobs/~011442245749e3e1fd

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @mateocole (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[MelvinBot]

@mateocole Huh... This is 4 days overdue. Who can take care of this?

[MelvinBot]

@mateocole Still overdue 6 days?! Let's take care of this!

[mateocole]

Hmm yeah looks odd stylistically. Having an engineer review

[MelvinBot]

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[alex-mechler]

Certainly looks like a 🐛. Sending external since this is all front end

[MelvinBot]

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[kevinksullivan]

Job posted. @adeel0202 feel free to apply for the reporting bonus!

https://www.upwork.com/jobs/~01f9c098c1c6835ed2

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

[MelvinBot]

Triggered auto assignment to @stitesExpensify (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[adeel0202]

@kevinksullivan, applied on Upwork for reporting bonus, thanks.

[imcvampire]

A solution for fixing this problem is tracking the selected button in React and overriding the default behaviour of the enter key in React based on the current selected button.

[Santhosh-Sellavel]

Hi @imcvampire, First, check out our contributing guidelines to understand our process.

And also go through these closed issues here for reference.

When you are ready make a proper proposal with the code changes that will address this issue. Thanks!

[mollfpr]

Proposal

Edit: If withNavigationFocus is implemented in Button like proposal in here, we can add pressOnEnter to the button cancel instead. For now, the only solution I can think of is the proposal below. https://github.com/Expensify/App/blob/34e2d3eacfb80356b1906869d87cb0f43b012287/src/components/ConfirmContent.js#L78-L82

Remove pressOnEnter from the button confirm will prevent creating Enter key down listener. https://github.com/Expensify/App/blob/34e2d3eacfb80356b1906869d87cb0f43b012287/src/components/ConfirmContent.js#L69-L74

- pressOnEnter

Chrome

https://user-images.githubusercontent.com/25520267/158017842-c1f1f0c7-e4fd-45d9-a11b-ea3bca8c8fd3.mov

Desktop

https://user-images.githubusercontent.com/25520267/158017861-671e1511-afed-4c22-9f00-8d69e9cb1e23.mov

[mollfpr]

Proposal

This issue appears because onPress function of Delete button is called because we pressed the Enter Key on Cancel button. https://github.com/Expensify/App/blob/c0b318188856d17249ae3d4c62182d31fb2f56bc/src/components/Button.js#L125-L131

Add some checks to make that the element target is equal to the focused current element. This will prevent callback from Delete button are called when when pressed Enter Key on Cancel button and vice versa.

        // Setup and attach keypress handler for pressing the button with Enter key
        this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
+            /** To make sure that button call this event are the active one */
+            const isButtonFocused = e.target === document.activeElement;

-            if (this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
+            if (!isButtonFocused || this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
                return;
            }
            this.props.onPress();
        }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true, false, this.props.enterKeyEventListenerPriority, false);

Also we pass pressOnEnter props from Cancel button to make sure the onPress function will be firing on event listener.

            <Button
                style={[styles.mt3]}
                onPress={props.onCancel}
                text={props.cancelText || props.translate('common.no')}
                pressOnEnter
            />

https://user-images.githubusercontent.com/25520267/158997680-b948d99b-3738-4f70-a1ea-75f1580ce8f0.mov

[mollfpr]

@Santhosh-Sellavel By the way it's better if you give some feedback for the contributor proposal than emoji 😉

[Santhosh-Sellavel]

Proposal

Edit: If withNavigationFocus is implemented in Button like proposal in here, we can add pressOnEnter to the button cancel instead. For now, the only solution I can think of is the proposal below.

https://github.com/Expensify/App/blob/34e2d3eacfb80356b1906869d87cb0f43b012287/src/components/ConfirmContent.js#L78-L82

Remove pressOnEnter from the button confirm will prevent creating Enter key down listener.

https://github.com/Expensify/App/blob/34e2d3eacfb80356b1906869d87cb0f43b012287/src/components/ConfirmContent.js#L69-L74

- pressOnEnter

Chrome

Screen.Recording.2022-03-12.at.18.49.28.mov

Desktop

Screen.Recording.2022-03-12.at.19.21.25.mov

@mollfpr

This was your proposal, here you suggested a one-line change it would break existing behavior. So I straight away gave a 👎 .

Do not rush proposals (Even I did some times), because we would end up giving a half-cooked solution Try to go through the problem statement until you fully understand it properly or ask questions to know the expected result for the issue. Then start working on the solution and propose a full-fledged solution instead of workarounds. Also, your solution should not break any existing behavior.

[Santhosh-Sellavel]

Proposal

This issue appears because onPress function of Delete button is called because we pressed the Enter Key on Cancel button.

https://github.com/Expensify/App/blob/c0b318188856d17249ae3d4c62182d31fb2f56bc/src/components/Button.js#L125-L131

Add some checks to make that the element target is equal to the focused current element. This will prevent callback from Delete button are called when when pressed Enter Key on Cancel button and vice versa.

        // Setup and attach keypress handler for pressing the button with Enter key
        this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
+            /** To make sure that button call this event are the active one */
+            const isButtonFocused = e.target === document.activeElement;

-            if (this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
+            if (!isButtonFocused || this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
                return;
            }
            this.props.onPress();
        }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true, false, this.props.enterKeyEventListenerPriority, false);

Also we pass pressOnEnter props from Cancel button to make sure the onPress function will be firing on event listener.

            <Button
                style={[styles.mt3]}
                onPress={props.onCancel}
                text={props.cancelText || props.translate('common.no')}
                pressOnEnter
            />

Screen.Recording.2022-03-18.at.18.25.21.mov

@mollfpr Here,

After a delete popup is shown, pressing enter should straight away perform a delete action. Focus is not necessarily required to perform the Delete action.

cc: @stitesExpensify

[mollfpr]

@Santhosh-Sellavel Okay, I see it now. Thanks!

[mollfpr]

Proposal

1. Update Button component to use forwardRef so we can get the ref from Pressable component and can compare it later inside ConfirmContent component.

export default React.forwardRef((props, ref) => (
    // eslint-disable-next-line react/jsx-props-no-spreading
    <Button {...props} forwardedRef={ref} />
));

            <Pressable
                ref={(el) => {
                    if (!this.props.forwardedRef) {
                        return;
                    }

                    this.props.forwardedRef(el);
                }}

4. Use class component instead of function component in ConfirmContent
5. Set the ref to confirm Button and cancel Button. Also we need to remove pressOnEnter props in confirm Button. We gonna set Enter key listener inside ConfirmContent component.

                <Button
                    ref={el => this.confirmButton = el}
                    success={this.props.success}
                    danger={this.props.danger}
                    style={[styles.mt4]}
                    onPress={this.props.onConfirm}
                    text={this.props.confirmText || this.props.translate('common.yes')}
                />
                {this.props.shouldShowCancelButton && (
                    <Button
                        ref={el => this.cancelButton = el}
                        style={[styles.mt3]}
                        onPress={this.props.onCancel}
                        text={this.props.cancelText || this.props.translate('common.no')}
                    />
                )}

6. Add document.activeElement.blur(); inside componentDidMount, this will keep the focus to body element rather than to the last button inside modal. We also add timeout because if we not using timeout after we blur the activeElement it's not gonna set it to body. (I cannot debug this, since after document.activeElement.blur() it's returning body element but after we press a keyboard (except Tab, metaKey, shiftKey, etc) the focus going to the last button which is cancel button.)

        /** Hacky way to prevent focusing Cancel Button when modal first show */
        setTimeout(() => {
            document.activeElement.blur();
        }, 100);

7. Create Enter Key listener. We check if the element target are not the buttons we run the deletion.

        const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
        this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
            if (!this.cancelButton || this.cancelButton === e.target || this.confirmButton === e.target) {
                return;
            }

            this.props.onConfirm();
        }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true, false, 0, true);

    componentWillUnmount() {
        // Cleanup event listeners
        if (!this.unsubscribe) {
            return;
        }
        this.unsubscribe();
    }

Changes in https://github.com/Expensify/App/blob/0b0a51ed0035d4e211e96c7cebfed83ce0063105/src/components/Button.js and https://github.com/Expensify/App/blob/0b0a51ed0035d4e211e96c7cebfed83ce0063105/src/components/ConfirmContent.js

Delete action with Enter

https://user-images.githubusercontent.com/25520267/159151121-3aa44fe1-07c0-4d8b-8f58-c1520543f18b.mov

Cancel button pressed

https://user-images.githubusercontent.com/25520267/159151161-a58b752a-24ab-4ae7-8e46-b94311d8abbc.mov

Confirm button pressed

https://user-images.githubusercontent.com/25520267/159151181-ae616b4f-85de-4269-85b3-38a1df12062a.mov

[Santhosh-Sellavel]

Will check this by monday!

[Santhosh-Sellavel]

I feel the proposal here is over-engineered.

@stitesExpensify I would like to hear your thoughts too on this one, thanks!

Waiting for better proposals!

[mollfpr]

As I said before the above solution is gonna resolve another issue which happens when the modal first shows and we press EnterKey, the Cancel button will be focused immediately.

https://user-images.githubusercontent.com/25520267/160265204-aa11bdf0-1bbf-4224-9b8e-60c82f5643d3.mov

---

The main issue here is we have 2 buttons and 1 of them has pressOnEnter pass which will make an event listener when EnterKey is pressed. But in the listener, there's no check which buttons pressing the EnterKey so if we press EnterKey on Cancel it's will also run the function from the listener which we don't mean to do. So that's why I think is better we handle this inside the modal that controls how many buttons are inside it.

[stitesExpensify]

@mollfpr do you know if that's a common pattern in other RN projects? I've got to imagine that this is something other people have run into where multiple rendered buttons have pressOnEnter

[mollfpr]

@stitesExpensify I didn’t know that.

Yeah because when the event listener is called, it didn't check whether the ‘EnterKey’ pressed from a button or not.

If both buttons use ‘pressOnEnter’ then both ‘onPress’ functions will be called whenever ‘EnterKey’ is pressed.

[stitesExpensify]

Okay, so I think the best solution would be to find an example online of how other people handle this situation, and we'll use that solution.

[stitesExpensify]

@mollfpr any update here?

[stitesExpensify]

@mollfpr bump

[mollfpr]

Sorry @stitesExpensify , I’m not finding anything new, I’ll try to take a look at this issue again later.

[kevinksullivan]

Job was no longer accepting new proposals so paid out @adeel0202 for reporting and reposted a new job with the price doubled.

https://www.upwork.com/jobs/~011442245749e3e1fd