[HOLD for payment 2023-05-05] [$1000] Long pressing on emoji suggestions highlights the text/attachments behind the emoji list on mweb

[kavimuru]

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. First of all , open android app, go to any chat, type in :per (to get the suggestion list)
2. Now long press any of the emoji from the suggestion list and notice that the emoji gets placed on the chat box
3. Next, repeat the same steps on mweb chrome and notice that in mweb,the emoji doesn’t gets placed on the chat box instead it highlights the text or attachments (anything that is present behind the suggestion list)

Expected Result:

There should be consistency between the mweb and android/web where long pressing the emoji from the suggestion list should place the emoji on the chat box on mweb

Actual Result:

Long pressing the emoji in the emoji suggestion list selects the text/attachment (any text) behind the suggestion list on mweb chrome

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.88-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

emojierror.mp4

az_recorder_20230324_133659.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679662560223179

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0170cd69222271989f
• Upwork Job ID: 1640771245599219712
• Last Price Increase: 2023-04-18

[MelvinBot]

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

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

[garrettmknight]

Confirmed, agree that we should make this consistent and the expected result makes sense.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0170cd69222271989f

[MelvinBot]

Current assignee @garrettmknight is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[redstar504]

Proposal

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

On mobile web, long pressing on an emoji suggestion results in the report action context menu being opened after the emoji suggestion component is dismissed.

What is the root cause of that problem?

The root cause seems to be the order of the event loop. The press is being intercepted by the view below the emoji suggestions as soon as focus is lost as a result of the long press. First focus is lost, and then the press handler is firing.

It does not happen with normal presses because there is no responder for that event on the underlying view, and it's also being intercepted using onMouseDown={() => e.preventDefault()} which does not emit for a long press.

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

The easiest way I found to solve this problem is to simply defer the function call that hides the emoji suggestions until after the event loop completes. This can be very easily achieved by replacing:

App/src/pages/home/report/ReportActionCompose.js

Line 833 in f68a199

this.resetSuggestedEmojis();

with

setTimeout(() => this.resetSelectedEmojis(), 0)

What alternative solutions did you explore? (Optional)

[bernhardoj]

Proposal

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

Chat context menu shows when we long press an emoji suggestion.

What is the root cause of that problem?

When we long press the emoji suggestion, the suggestion list will be dismissed because the composer is blurred.

App/src/pages/home/report/ReportActionCompose.js

Lines 831 to 834 in 3a9ce6f

	onBlur={() => {
	this.setIsFocused(false);
	this.resetSuggestedEmojis();
	}}

The blur is the normal behavior on web. We can verify this by long press any element, for example the send button. When we long press an element, we can catch the event by listen to contextmenu, which is what we do for the chat item, but not with the emoji suggestion.

App/src/components/PressableWithSecondaryInteraction/index.js

Line 21 in 3a9ce6f

this.pressableRef.addEventListener('contextmenu', this.executeSecondaryInteractionOnContextMenu);

So, how the event is triggered even though we long press the emoji suggestion?

I don't have a reference to why it happens, but we can reproduce it with a simple html. Here is the codepen https://codepen.io/bernhardoj/pen/XWPwBJO to try it.

(video):

WhatsApp.Video.2023-03-31.at.09.51.53.mp4

I change the console log to alert to debug it easier. The codepen is trying to replicate our chat page. The red box is the emoji suggestion which will hide when the input is blurred. Execute the same steps and notice an alert with text CONTEXT is shown but not with SUGGESTION CONTEXT. I'm guessing because the red box becomes hidden, the oncontextmenu is fired on the element behind it that receives the touch.

The purpose of contextmenu event is for the right click event on web/desktop.

App/src/components/PressableWithSecondaryInteraction/index.js

Lines 28 to 31 in 3a9ce6f

	/**
	* @param {contextmenu} e - A right-click MouseEvent.
	* https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event
	*/

On native/mWeb, we already have a long press event.

App/src/components/PressableWithSecondaryInteraction/index.js

Lines 59 to 67 in 3a9ce6f

	onLongPress={(e) => {
	if (DeviceCapabilities.hasHoverSupport()) {
	return;
	}
	if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
	this.pressableRef.blur();
	}
	this.props.onSecondaryInteraction(e);
	}}

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

We can listen the contextmenu event only for desktop and web (i.e. isSmallScreenWidth is false).

1. Wrap with withWindowDimensions
2. If this.props.isSmallScreenWidth is false, don't add the listener.

Or is it better to use DeviceCapabilities.canUseTouchScreen ?

[rushatgabhane]

@bernhardoj it would be great if you can share the html on codepen :)

[rushatgabhane]

Anyway, both solutions feel like a hack to me because we don't know the exact root cause

[bernhardoj]

Added the codepen and video.

because the red box becomes hidden, the oncontextmenu is fired on the element behind it that receives the touch.

This is the root cause which you can validate with the codepen I provided.

[redstar504]

After digging into this a bit more, I have found that none of the other emoji components, or the context menu items, respond to a long press on mWeb. I think if we want to keep things consistent we should match the behavior of the other components and prevent a long press from actually closing the emoji suggestion dialog.

Long pressing on an emoji suggestion does not actually add the emoji, but just keeps the text you entered up to the point that it opened, so it does not make sense to close the menu when it happens. I have updated my proposal to reflect this solution.

[rushatgabhane]

Chrome mobile web behavior seems different in this regard to other platforms, which dismiss the long press after it has occurred for the visible element

is there some documentation for this? is this bug logged for chrome? if yes, then won't it fix itself after some chrome updates?

prevent a long press from actually closing the emoji suggestion dialog

@bernhardoj @redstar504 I think that is a hack.

[bernhardoj]

Here is my opinion on why disabling the contextmenu event for mobile web is not a "hack".

First of all, the current issue we are facing is currently the behavior of browsers (except for Safari mobile, I will explain more on this later) as I explained here #16485 (comment). Here is a sandbox https://codesandbox.io/s/context-menu-oyj370 to test that behavior on non-mobile web. Right click the red box and you will see the contextmenu event is triggered on the red box.

Now, why it does not happen on iOS Safari? That is because iOS Safari never fires contextmenu on long press. You can validate this too by focusing on main composer then long press on emoji picker button. On Android Chrome, the keyboard is dismissed because of the contextmenu event while on iOS Safari is not. There are several reports of iOS Safari doesn't fire the contextmenu event 1, 2, 3, 4.

If you read the 3rd source (the twitter one), they said

so you have to implement your own long-press detection and handling

which we already does with the Pressable, but the onLongPress event is overridden by the contextmenu event, so it never fires. (idk if this another browser bug or not, but imagine if both event fires)

App/src/components/PressableWithSecondaryInteraction/index.js

Lines 59 to 67 in 4ee21e9

	onLongPress={(e) => {
	if (DeviceCapabilities.hasHoverSupport()) {
	return;
	}
	if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
	this.pressableRef.blur();
	}
	this.props.onSecondaryInteraction(e);
	}}

That's why disabling the contextmenu event on mobile web makes sense as we already handle the long press with onLongPress.

[MelvinBot]

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

[garrettmknight]

Still working on this one @rushatgabhane can you review the updated proposal and if you still think it's a hack we'll double and find another proposal?

[ntdiary]

Hi, I‘ve pushed a commit to prevent the focus event. And now have a small question, which of the following is our expected behavior?

1. the emoji will not be placed in the main input until the touch is released.

release.mp4

2. support long press (onLongPress handler), which means that after pressing the emoji for 500 milliseconds, the emoji will be placed in the main input. (touch has not been released)

long-press.mp4

3. the emoji will not be placed in the main input regardless of whether the touch is released or pressing the emoji for 500 milliseconds.

no-long-press.mp4

Option 1 is the behavior of the current native app. Option 3 is the behavior of the current iOS / Safari platform. (production environment)

• If we choose option 1, we also need to use the pointerup event handler on the web, may be relatively complicated:
  • We can create a component to handle the pointerup event (e.g. EmojiRowItem/index.js)
  • Or we need to calc the index of the pressed emoji row, and call props.onSelect(index) in the pointerup hander of the EmojiSuggestions component.
• If we choose option 2, we need to add onLongPress={() => props.onSelect(index)} to the <Pressable>
• If we choose option 3, we need to add onLongPress={() => {}} to the <Pressable>
  

  App/src/components/EmojiSuggestions.js

  Lines 85 to 93 in 931e15f

  	<Pressable
  	style={({hovered}) => StyleUtils.getEmojiSuggestionItemStyle(
  	props.highlightedEmojiIndex,
  	CONST.EMOJI_SUGGESTER.ITEM_HEIGHT,
  	hovered,
  	index,
  	)}
  	onMouseDown={e => e.preventDefault()}
  	onPress={() => props.onSelect(index)}

Or we still keep option 1 for native app and use option 3 for mWeb platform uniformly? (This way we don't need to add onLongPress prop)

cc @rushatgabhane @tgolen

[tgolen]

What do you think is the best UX? Then, if we all agree on that, let's make it consistent on all platforms.

I actually don't have a good idea of what I would expect to happen here. I very rarely use longpress for anything, so I would probably expect nothing to happen (which I think is option 3).

[rushatgabhane]

keep it simple. do nothing on long press

[ntdiary]

My initial thought was that we just need to make sure there is no wrong interaction on long press( the context menu appears unexpectedly). As for whether or not emoji are selected on a long press is probably not a big deal, we can just leave as it is, after all, the main way to select an emoji is by tapping it.

[ntdiary]

keep it simple. do nothing on long press

Fine, I will add onLongPress={() => {}} in a later commit.

[MelvinBot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

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

• prevent focus event when long press in emoji suggestion component #17788

If no regressions arise, payment will be issued on 2023-05-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter - @priya-zha $250
• Contributor that fixed the issue - @ntdiary $1500
• Contributor+ that helped on the issue and/or PR - @rushatgabhane $1500

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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:

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

[rushatgabhane]

Checklist to please the bot :)

1. The PR that introduced the bug has been identified. Link to the PR: Static emoji autosuggestion  #14686

2. 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: Static emoji autosuggestion  #14686 (comment)

3. 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: Not required because this is an extreme edge case for mWeb android only.

4. Determine if we should create a regression test for this bug - Not required. This is an extreme edge case for mWeb android only.

5. 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 - N.A.

[melvin-bot]

@tgolen, @ntdiary, @michaelhaxhiu, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[michaelhaxhiu]

edit: Buddy check, seems there is no speed bonus applied to this PR right?

A speed bonus should apply for the C and C+.

cc @ntdiary and @rushatgabhane

• April 21 - job assigned to ntdiary here
• April 26 - PR merged here (within 3 business days)

[rushatgabhane]

@michaelhaxhiu looks like there is a timeline bonus as per https://ex-syt.glitch.me/

[michaelhaxhiu]

Ahh the ol "3 business days" caught me up! Thanks for that :)

[michaelhaxhiu]

Ok so, these are the payouts:

• External issue reporter - @priya-zha $250
• Contributor that fixed the issue - @ntdiary $1500
• Contributor+ that helped on the issue and/or PR - @rushatgabhane $1500

[michaelhaxhiu]

Just waiting on @rushatgabhane to accept the job and I can pay/close.

[michaelhaxhiu]

Paid all parties