[HOLD for Payment 2024-09-03] [$250] Chat - Unable to deselect user using The Tab key

[izarutskaya]

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: v9.0.19-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: https://expensify.testrail.io/index.php?/tests/view/4847797
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to FAB > Start Chat
2. Using the tab key and Enter key select add to group multiple results from the contact lists
3. use the arrow and tab key and try to deselect the selected users

Expected Result:

Able to deselect users using tab key

Actual Result:

Unable to deselect user using The Tab key. User is navigated to 1:1 conversation

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

Bug6570676_1723537229718.bandicam_2024-08-13_10-42-07-312.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018da70c1cbe5cf980
• Upwork Job ID: 1823397082345036137
• Last Price Increase: 2024-08-13
• Automatic offers:
• ZhenjaHorbach | Contributor | 103590029
• Krishna2323 | Contributor | 103591698

Issue Owner

Current Issue Owner: @alitoshmatov

[melvin-bot]

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-08-13 10:33:23 UTC.

Proposal

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

Chat - Unable to deselect user using The Tab key

What is the root cause of that problem?

The role is set to CONST.ACCESSIBILITY_ROLE.CHECKBOX. It should be CONST.ROLE.BUTTON like here & here.

App/src/pages/NewChatPage.tsx

Lines 256 to 262 in 86da342

	<PressableWithFeedback
	onPress={() => toggleOption(item)}
	disabled={item.isDisabled}
	role={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
	accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
	style={[styles.flexRow, styles.alignItemsCenter, styles.ml3]}
	>

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

Update the role to role={CONST.ROLE.BUTTON} and accessibilityLabel to accessibilityLabel={item.text ?? ''}.

If needed, we can replace all the use of CONST.ACCESSIBILITY_ROLE in the app with CONST.ROLE.

What alternative solutions did you explore? (Optional)

We can export the KeyboardShortcutComponent from Button/index.tsx and use it inside PressableWithFeedback like we do in Button/index.tsx.

What alternative solutions did you explore? (Optional 2)

We can simply create a handler for onKeyDown prop and prevent default when key is Enter. I think this is the simplest and most straight forward solution. We don't need to include KeyboardShortcutComponent because we don't want a pressable item to capture Enter key events even if it is not focused.

    const onKeyDownHandler = (event: React.KeyboardEvent) => {
        if (event.key === 'Enter') {
            event.preventDefault();
        }
        onKeyDown?.(event);
    };

OR:

We can modify onPressHandler to prevent default when the event is an instance of KeyboardEvent and key is Enter

const onPressHandler = useCallback(
   (event?: GestureResponderEvent | KeyboardEvent) => {
       if (event instanceof KeyboardEvent && event.key === 'Enter') {
           event.preventDefault();
       }
       if (isDisabled || !interactive) {...

What alternative solutions did you explore? (Optional 3)

Add onKeyDown={(e) => e.preventDefault()}.

We can also check for the key pressed if needed.

onKeyDown={(event) => {
    if (event.key !== 'Enter') {
        return;
    }
    event.preventDefault();
}}

[nyomanjyotisa]

Proposal

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

Unable to deselect user using Enter when the focus is already changed using Tab key to the selected user icon

What is the root cause of that problem?

We don't handle on enter key press here

App/src/pages/NewChatPage.tsx

Lines 254 to 266 in 86da342

	if (item.isSelected) {
	return (
	<PressableWithFeedback
	onPress={() => toggleOption(item)}
	disabled={item.isDisabled}
	role={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
	accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
	style={[styles.flexRow, styles.alignItemsCenter, styles.ml3]}
	>
	<SelectCircle isChecked={item.isSelected} />
	</PressableWithFeedback>
	);
	}

While on "Add to group" we use button component for it, so it already have a on key press handle

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

create KeyboardShortcutComponent to toggle option when it is focused and enter key pressed and use it here

function KeyboardShortcutComponent() {
                const isFocused = useIsFocused();

                const keyboardShortcutCallback = useCallback(
                    (event?: GestureResponderEvent | KeyboardEvent) => {
                        event?.preventDefault();
                        toggleOption(item)
                    },
                    [],
                );
            
                const config = useMemo(
                    () => ({
                        isActive: isFocused,
                        shouldPreventDefault: false,
                    }),
                    [isFocused],
                );
            
                useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, keyboardShortcutCallback, config);
            
                return null;
            }

            if (item.isSelected) {
                return (
                    <>
                        <KeyboardShortcutComponent/>
                        <PressableWithFeedback
                            onPress={() => toggleOption(item)}
                            disabled={item.isDisabled}
                            role={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
                            accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
                            style={[styles.flexRow, styles.alignItemsCenter, styles.ml3]}
                        >
                            <SelectCircle isChecked={item.isSelected} />
                        </PressableWithFeedback>
                    </>
                );
            }

What alternative solutions did you explore? (Optional)

[Krishna2323]

Proposal Updated

• Added alternative.

Note: The proposal above came just few seconds before I update the my proposal the minutes are the same. Just noting it down for C+.

[Krishna2323]

Proposal Updated

• Added alternative 2

[Krishna2323]

Proposal Updated

• Updated main solution and RCA, and moved the previous content in the main solution to alternative 3 section.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018da70c1cbe5cf980

[melvin-bot]

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

[sakluger]

@alitoshmatov can you take a look at the proposals we have so far?

[melvin-bot]

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

[Krishna2323]

@alitoshmatov, friendly bump for a review.

[melvin-bot]

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[sakluger]

@ZhenjaHorbach thanks for handling C+ for this one!

[ZhenjaHorbach]

@Krishna2323 @nyomanjyotisa
Thank you for your proposals !

As far as I can see 'ACCESSIBILITY_ROLE' is deprecated
And since we have several buttons which do the same in listItems that have role like CONST.ROLE.BUTTON
I agree with @Krishna2323 proposal that we need to update the role for consistency

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[dangrous]

Sounds good to me! Let's attempt to swap out all the ACCESSIBILITY_ROLE instances for ROLE, like you said - but if it's too many and hard to test, then maybe we can do it later.

[melvin-bot]

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[Krishna2323]

@ZhenjaHorbach, PR ready for review ^

[dangrous]

I think this is likely on prod and the automation didn't fire (since two deploys had that happen)... We should confirm and then update the payment date accordingly!

[dangrous]

@sakluger I'm like 99% certain this is on prod and we're out of the 7 day window here - do you mind trying to help confirm that, and hopefully be able to issue payment?

[sakluger]

I confirmed that this is live on prod! Moving to daily and handling payment.

[sakluger]

Summarizing payment on this issue:

Contributor: @ZhenjaHorbach $250, paid via Upwork
Contributor+: @Krishna2323 $250, paid via Upwork

@ZhenjaHorbach do you think we need regression test steps here? If so, could you please leave proposed test steps?

[ZhenjaHorbach]

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:

• [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:

Unfortunately, I did not find a suitable PR
Plus this PR is more related to code refactoring and ACCESSIBILITY_ROLE removal

• [ @ZhenjaHorbach] 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:

NA

• [@ZhenjaHorbach] 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:

NA

• [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
• [@ZhenjaHorbach] 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.

Regression Test Proposal

• Go to FAB > Start Chat
• Using the tab key and Enter key select add to group multiple results from the contact lists
• Use the arrow and tab key and try to deselect the selected users
• Verify user can be deselected using tab key

Do we agree 👍 or 👎

[ZhenjaHorbach]

@sakluger
Done !

[sakluger]

Thanks!