Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2022-04-12] [$250] Android - Emoji- Emoji is selected only on second tap #8110

Closed
kbecciv opened this issue Mar 12, 2022 · 23 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Mar 12, 2022

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


Action Performed:

  1. Launch the app
  2. Log in with account and select user
  3. On Input box - tap on emoji icon
  4. Tap on any emojis

Expected Result:

Emoji is selected on first tap

Actual Result:

Emojie is selected only on second tap

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.1.42.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): hateemshafi00@gmail.com

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5487377_20220311_234129.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause (Exploratory)

Slack conversation:

Job Post: https://www.upwork.com/jobs/~01d711814db7c31f61

View all open jobs on GitHub

@MelvinBot
Copy link

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

@johnmlee101
Copy link
Contributor

K reproduced. Opening this up to external. I'm guessing it has to do with focus of the picker

@johnmlee101 johnmlee101 added the External Added to denote the issue can be worked on by a contributor label Mar 14, 2022
@MelvinBot
Copy link

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

@kevinksullivan kevinksullivan changed the title Android - Emoji- Emojie is selected only on second tap Android - Emoji- Emoji is selected only on second tap Mar 14, 2022
@kevinksullivan kevinksullivan self-assigned this Mar 14, 2022
@botify botify removed the Daily KSv2 label Mar 14, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Mar 14, 2022
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 14, 2022
@MelvinBot
Copy link

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

@mollfpr
Copy link
Contributor

mollfpr commented Mar 16, 2022

Proposal

The issue is happen because the keyboard is still visible after the modal show, show the first press is for dismissing the keyboard then the second press is for selecting emoji.

My solution is to dismiss the keyboard when emoji button is pressed to open emoji modal.

<Pressable
ref={el => emojiPopoverAnchor = el}
style={({hovered, pressed}) => ([
styles.chatItemEmojiButton,
StyleUtils.getButtonBackgroundColorStyle(getButtonState(hovered, pressed)),
])}
disabled={props.isDisabled}
onPress={() => EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor)}

-            onPress={() => EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor)}

+            onPress={() => {
+                /* Dismiss the keyboard before opening the modal, this will prevent double press when selecting emoji */
+                Keyboard.dismiss();
+                EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor);
+            }}

Also we need to add the timeout time from 50 to 100 to make room for keyboard opening.

setTimeout(() => this.textInput.focus(), 50);

-                setTimeout(() => this.textInput.focus(), 50);
+                setTimeout(() => this.textInput.focus(), 100);

Result

Screen.Recording.2022-03-16.at.22.42.52.mov

@tgolen
Copy link
Contributor

tgolen commented Mar 16, 2022

@mollfpr Thank you for the proposal! It looks mostly good to me, but I did have a question.

The issue is happen because the keyboard is still visible after the modal show

From watching your video, it doesn't look to me like the keyboard is open before the modal opens, so then why does the keyboard need to be dismissed?

@mollfpr
Copy link
Contributor

mollfpr commented Mar 16, 2022

@tgolen Ahh I forget about that. I should use this code instead. I use hoc withKeyboardState to have an isShown to check whether the keyboard is open or not. If open then dismiss it.

                /* Dismiss the keyboard before opening the modal, this will prevent double press when selecting emoji */
                if (props.isShown) {
                    Keyboard.dismiss();
                }

                EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor);

@tgolen
Copy link
Contributor

tgolen commented Mar 16, 2022

Ah, OK. That sounds good. Thank you!

I'm not too excited about changing the setTimeout() but it seems like that hack is still going to be there for a while, so I don't have any other suggestions.

🟢 to hire @mollfpr with that proposal @kevinksullivan

@kevinksullivan
Copy link
Contributor

Hired @mollfpr , go ahead and accept whenever you get a chance!

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Mar 18, 2022

@tgolen
Thanks for taking this quicker!
Seems PR is ready , let's assign @mollfpr this issue!

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Mar 18, 2022

@tgolen Solution does not work for me!
cc: @mollfpr

Screen Recording

Screen_Recording_20220319-025959_New.Expensify.mp4

@mollfpr
Copy link
Contributor

mollfpr commented Mar 18, 2022

@Santhosh-Sellavel what Android version do you use?

@Santhosh-Sellavel
Copy link
Collaborator

@mollfpr I am using a Android 12
Samsung device.

@mollfpr
Copy link
Contributor

mollfpr commented Mar 19, 2022

@Santhosh-Sellavel @tgolen I found the culprit.

We need to make the text input unfocus. There are two approaches to do this.

  1. Dismiss the keyboard without checking is shown or not, in this way the text input will be blurred.
                /* Dismiss the keyboard before opening the modal, this will prevent double press when selecting emoji */
-                if (props.isShown) {
                    Keyboard.dismiss();
-                }
  1. Passing the text input ref to EmojiPickerButton and call blur() when the modal will show.
                /** Unfocusing the text input to prevent double press when select a emoji */
                if (props.textInput) {
                    props.textInput.blur();
                }

Before either above solution applied

Screen.Recording.2022-03-19.at.10.02.25.mov

With Keyboard.dismiss()

Screen.Recording.2022-03-19.at.10.05.24.mov

With text input blur

Screen.Recording.2022-03-19.at.10.05.47.mov

Those above solutions are great, but I'm thinking probably the EmojiPicker will be used not only in ReportActionCompose but maybe other components will also use this and maybe without EmojiPickerButton which trigger the modal in composing component. So the best solution I came up with is calling Keyboard.dismiss() in EmojiPicker whenever is emoji picker visible or not.

componentDidMount() {
this.emojiPopoverDimensionListener = Dimensions.addEventListener('change', this.measureEmojiPopoverAnchorPosition);
}

    // eslint-disable-next-line rulesdir/prefer-early-return
    componentDidUpdate(prevProps, prevState) {
        if (prevState.isEmojiPickerVisible !== this.state.isEmojiPickerVisible) {
            Keyboard.dismiss();
        }
    }

Result

Screen.Recording.2022-03-19.at.10.24.49.mov

@mollfpr
Copy link
Contributor

mollfpr commented Mar 19, 2022

@tgolen @Santhosh-Sellavel updated PR.

@tgolen
Copy link
Contributor

tgolen commented Mar 30, 2022

PR is merged!

@MelvinBot MelvinBot removed the Overdue label Mar 30, 2022
@mountiny mountiny changed the title Android - Emoji- Emoji is selected only on second tap [$250] Android - Emoji- Emoji is selected only on second tap Mar 31, 2022
@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 31, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 5, 2022
@botify botify changed the title [$250] Android - Emoji- Emoji is selected only on second tap [HOLD for payment 2022-04-12] [$250] Android - Emoji- Emoji is selected only on second tap Apr 5, 2022
@botify
Copy link

botify commented Apr 5, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.49-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-04-12. 🎊

@Santhosh-Sellavel
Copy link
Collaborator

Applied for C+ @kevinksullivan

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 11, 2022
@kevinksullivan
Copy link
Contributor

Hired @Santhosh-Sellavel .

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@kevinksullivan
Copy link
Contributor

Paid. Thanks again @Santhosh-Sellavel @mollfpr !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants