[HOLD for Payment 2024-09-6][$250] clicking enter on emoji selector for status results in leaving status pane

m-natarajan commented 2 months ago

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: 9.0.19-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 Expensify/Expensify Issue URL: Issue reported by: @dylanexpensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723475125409519

Action Performed:

Head to status
Click on emoji selector
Search for any emoji
When the emoji you want is auto-focused, hit enter

Expected Result:

emoji is selected and window for emoji search disappears

Actual Result:

emoji is not selected, and the RHP for status disappears

Workaround:

unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/user-attachments/assets/0772fb25-a1e5-4e17-8fe9-149ff453806c

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d3caa8f59dbe1758
Upwork Job ID: 1823486563046416835
Last Price Increase: 2024-08-13
Automatic offers:

rayane-djouah | Reviewer | 103586699
Krishna2323 | Contributor | 103586701

Issue Owner

Current Issue Owner: @mallenexpensify

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @mallenexpensify (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 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-12 23:02:25 UTC.

Proposal

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

clicking enter on emoji selector for status results in leaving status pane

What is the root cause of that problem?

Form is submitted because of enter press. https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L167-L176

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

We should check if the emoji picker is active or not and if it is we will disable press on enter.

disablePressOnEnter={EmojiPickerAction.isEmojiPickerVisible()}

What alternative solutions did you explore? (Optional)

We can focus on the message input box when the modal hides. https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L189

onModalHide={() => {
    inputRef.current?.focus();
}}

What alternative solutions did you explore? (Optional 2)

We can use useKeyboardShortcut to capture enter clicks and perform emoji selection.

    useKeyboardShortcut(
        CONST.KEYBOARD_SHORTCUTS.ENTER,
        () => {
            let indexToSelect = focusedIndex;
            if (highlightFirstEmoji) {
                indexToSelect = 0;
            }

            const item = filteredEmojis[indexToSelect];
            if (!item) {
                return;
            }
            if ('types' in item || 'name' in item) {
                const emoji = typeof preferredSkinTone === 'number' && item?.types?.[preferredSkinTone] ? item?.types?.[preferredSkinTone] : item.code;
                onEmojiSelected(emoji, item);
            }
        },
        {isActive: !isEnterWhileComposition(keyBoardEvent), shouldPreventDefault: true, shouldStopPropagation: true},
    );

https://github.com/Expensify/App/blob/1eef411e1d9e85145f160ee5610bb9cc9a8c04d7/src/components/EmojiPicker/EmojiPickerMenu/index.tsx#L145-L164

daledah commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-13 04:48:25 UTC.

Proposal

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

Emoji is not selected, and the RHP for status disappears

What is the root cause of that problem?

In FormProvider: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L167 the disablePressOnEnter param is false by default.

When we open emoji picker then press on Enter, the onSubmit function is triggered: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L173

so the RHP is closed.

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

When the emoji picker is showing, we shouldn't trigger updateStatus on Enter: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L98 by adding an early return logic:

            if(EmojiPickerAction.emojiPickerRef.current?.isEmojiPickerVisible){
                return
            }

bellow: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L99

What alternative solutions did you explore? (Optional)

We can create a new state:

const [isEmojiPickerVisible, setIsEmojiPickerVisible] = useState(false);

Then: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L189 will be:

                        onModalHide={() => setIsEmojiPickerVisible(false)}
                        onWillShow={() => setIsEmojiPickerVisible(true)}

Then: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/components/EmojiPicker/EmojiPickerButtonDropdown.tsx#L57 will be:
```
        onWillShow,
```
Then: https://github.com/Expensify/App/blob/adaf5cd8044179fd4cb90b37447eb2ba5d8c6054/src/pages/settings/Profile/CustomStatus/StatusPage.tsx#L167 will be:
```
            disablePressOnEnter={isEmojiPickerVisible}
```

daledah commented 2 months ago

Proposal updated

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01d3caa8f59dbe1758

melvin-bot[bot] commented 2 months ago

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

mallenexpensify commented 2 months ago

Good catch @dylanexpensify I was able to reproduce. @rayane-djouah , plz review the proposals above.

https://github.com/user-attachments/assets/74042833-4202-49af-8242-6cfdca176f6f

Krishna2323 commented 2 months ago

Proposal Updated

Minor fix

rayane-djouah commented 2 months ago

@Krishna2323 @daledah - Thank you for your proposals.

Could you clarify why, on this page for example: https://staging.new.expensify.com/search/filters/category (using SearchMultipleSelectionPicker), when you search for a category and press the Enter key, the focused category is correctly selected and the form is not submitted (even though we haven't disabled pressOnEnter for the button)? However, on the status page, if you search for an emoji and press Enter, the form is submitted (the form submit button is pressed) instead of selecting the focused emoji?

https://github.com/user-attachments/assets/28d6430f-c12b-4c03-9c78-32cb5783ea26

Krishna2323 commented 2 months ago

@rayane-djouah, I believe that's because on status page when we search for emoji, the emoji is just highlighted not focused. But on categories page the option is both focused and highlighted.

rayane-djouah commented 2 months ago

@rayane-djouah, I believe that's because on status page when we search for emoji, the emoji is just highlighted not focused. But on categories page the option is both focused and highlighted.

@Krishna2323 How can we make the emoji focused to be selected correctly on enter key press instead of submitting the form?

Krishna2323 commented 2 months ago

@rayane-djouah, I think my proposal has the simplest solution possible for this because if we try to focus emoji on text input change, the focus from the input will we removed on every character entered. You can try focusing on emoji by up/down arrow keys, you will notice the focus from input is removed.

Krishna2323 commented 2 months ago

Proposal Updated

Added alternative 2

cc: @rayane-djouah

Krishna2323 commented 2 months ago

@rayane-djouah, friendly bump for checking comments above ^

rayane-djouah commented 2 months ago

@Krishna2323 - I want to make sure we understand the root cause. Could you please explain why the keyDownHandler is not being called before the form button’s pressOnEnter event handler? Is this possibly related to how we are subscribing to it here?

Krishna2323 commented 2 months ago

Could you please explain why the keyDownHandler is not being called before the form button’s pressOnEnter event handler?

@rayane-djouah, keyDownHandler is called before button’s pressOnEnter but the problem is that both event listeners are called.

What I believe is happening is that in EmojiPickerMenu, we are directly adding the listener to the document, while in the Button component, we are using useKeyboardShortcut, which utilizes KeyCommand from react-native-key-command. Due to this, the listeners are applied differently, and as a result, keyBoardEvent.preventDefault(); and keyBoardEvent.stopPropagation(); do not work as expected.

https://github.com/Expensify/App/blob/578006fe4d2b7ba13155e795bcef541019aad8ea/src/components/Button/index.tsx#L149-L160 https://github.com/Expensify/App/blob/578006fe4d2b7ba13155e795bcef541019aad8ea/src/libs/KeyboardShortcut/index.ts#L2

https://github.com/Expensify/App/blob/578006fe4d2b7ba13155e795bcef541019aad8ea/src/libs/KeyboardShortcut/index.ts#L92

rayane-djouah commented 2 months ago

@Krishna2323's proposal looks good to me. I prefer using the 2nd alternative solution (using useKeyboardShortcut) as it fixes the root cause

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 months ago

📣 @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 📖

mallenexpensify commented 2 months ago

Deployed to production earlier today Wonder if automation isn't working here (maybe I shoulda waited til Monday before updating the labels)

rayane-djouah commented 2 months ago

[!NOTE] The production deploy automation failed: This should be on [HOLD for Payment 2024-09-6] according to https://github.com/Expensify/App/issues/48221 prod deploy checklist, confirmed in https://github.com/Expensify/App/pull/47761#issuecomment-2322121729.

cc @mallenexpensify

mallenexpensify commented 2 months ago

Thanks @rayane-djouah , and for linking the deploy checklist too

@rayane-djouah , you might know the answer here in the related Slack thread. https://expensify.slack.com/archives/C01GTK53T8Q/p1725320002440979?thread_ts=1725050431.962859&cid=C01GTK53T8Q

melvin-bot[bot] commented 2 months ago

@carlosmiceli, @mallenexpensify, @rayane-djouah, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mallenexpensify commented 2 months ago

Contributor: @Krishna2323 paid $250 via Upwork Contributor+: @rayane-djouah paid $250 via Upwork, can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01d3caa8f59dbe1758

@rayane-djouah , can you also fill out the BZ checklist plz? thx

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:

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

rayane-djouah commented 1 month ago

Checklist

[x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/34581
[x] 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: https://github.com/Expensify/App/pull/34581#issuecomment-2336804760
[x] 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: N/A
[x] Determine if we should create a regression test for this bug. Yes
[x] 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 Settings > Status
Click on the emoji selector
Search for any emoji
When the emoji you want is auto-focused, hit enter
Verify that the emoji is selected and the message input is focused

Do we agree 👍 or 👎

rayane-djouah commented 1 month ago

@mallenexpensify - Accepted the job and completed the checklist

mallenexpensify commented 1 month ago

Thanks @rayane-djouah , I updated the comment above to denote you've been paid.

Test Case is here

https://github.com/Expensify/Expensify/issues/427110

Expensify / App