[HOLD for payment 2024-02-07] [$2000] Chat - Whitespace is not added after an emoji while inserting via native keyboard

[kbecciv]

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. Open a chat
2. Insert a couple of emojis via emoji picker available in composer and send the message.
3. Now add a bunch of emojis via the emoji picker in native keyboard and send the message

Expected Result:

As per the issue #11100 and slack discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1662746279291419 a white space is expected after each emoji.

Actual Result:

Whitespace is added only if emoji is added via emoji picker in composer and not while added using native keyboard.

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.3.44-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
Notes/Photos/Videos: Any additional supporting documentation

emoji.webm

RPReplay_Final1690382962.1.MP4

Expensify/Expensify Issue URL:
Issue reported by: @aswin-s
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690359926207719

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0188d7c4a5e13254b6
• Upwork Job ID: 1684628795620294656
• Last Price Increase: 2023-10-20
• Automatic offers:
• aswin-s | Contributor | 27373872
• aswin-s | Reporter | 27373874

[melvin-bot]

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

[melvin-bot]

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

[samh-nl]

Proposal

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

Whitespace is not added after an emoji while inserting via native keyboard

What is the root cause of that problem?

No logic has been implemented for automatically adding whitespace after inserting an emoji via the native keyboard.

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

In ReportActionCompose, we perform these changes:

1. Change to an onChange handler:

- onChangeText={(comment) => this.updateComment(comment, true)}
+ onChange={this.updateCommentAndProcessEmoji}

2. The new function would be a wrapper around this.updateComment. It detects if the event is an emoji insertion and appends a trailing space (default behavior of this.replaceSelectionWithText). Subsequently, it passes on the processed comment.

updateCommentAndProcessEmoji(event) {
	if (event.nativeEvent.data && event.nativeEvent.isComposing && EmojiUtils.containsOnlyEmojis(event.nativeEvent.data)) {
                this.replaceSelectionWithText(event.nativeEvent.data);
	} else {
	        this.updateComment(event.nativeEvent.text, true);
        }
}

3. We add a shouldDebounceSaveComment (default false) argument to this.replaceSelectionWithText to pass to this.updateComment.

This change would also be applied to ReportActionItemMessageEdit.

What alternative solutions did you explore? (Optional)

N/A

[daordonez11]

Proposal

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

Experience of inserting emoji in chat is not unified!

What is the root cause of that problem?

So I will open a new discussion in slack. @mallenexpensify proposed the space but this is only true when emoji is written with :emoji, when the emoji is picked from the picker menu in slack it is written without trail space. We would have to write custom code to "process" the written emoji and set a space in the onChange of the composer to unify but my proposal is to set the experience same as slack!

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

I would remove the trail from onEmojiSelected in ReportActionCompose and ReportActionItemMessageEdit since the implementation of :emoji already includes the space to unify the experience with slack and leave the EmojiPickerMenu to behave as the native one. Same as slack.

Video of solution working same as slack with native menu and EmojiPickerMenu:

working.as.slack.mp4

What alternative solutions did you explore? (Optional)

Update the onChange handler of Composer component, use a regex to validate if there is an emoji and include the space after it is included via native picker. Still I think altering native behavior wouldn't be expected but if it is desired we can implement the custom code!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0188d7c4a5e13254b6

[melvin-bot]

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

[melvin-bot]

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

[mollfpr]

@daordonez11 I still prefer consistency in our emoji addition, and having a space is what we have now. So we should fix it by adding an emoji from the native keyboard.

To detect the insertion of an emoji, we must rely on onChange. Here, we should check whether nativeEvent.data is an emoji.

@samh-nl What's your plan for this? Will we replace the current this.updateComment with the used onChange from the composer?

[daordonez11]

Oh yeah I agree @mollfpr it was discussed in this thread https://expensify.slack.com/archives/C01GTK53T8Q/p1690483337635379 definitely the other proposal is the way to go. I'll check if there's a better proposal to be done.

[CortneyOfstad]

I'm heading OoO until 8/14, so reassigning BZ to keep an eye on this while I'm out 👍

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[mollfpr]

What's your plan for this? Will we replace the current this.updateComment with the used onChange from the composer?

Friendly bump @samh-nl

[CortneyOfstad]

@mollfpr bump ^^^ Thanks!

[CortneyOfstad]

@mollfpr bump again — TIA!

[mollfpr]

Sorry for the delay 🙏

I'm starting to work on this issue again and testing the new PR.

[melvin-bot]

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

[melvin-bot]

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

• feat: append whitespace after inserting emoji #30955

If no regressions arise, payment will be issued on 2024-02-07. 🎊

For reference, here are some details about the assignees on this issue:

• @aswin-s requires payment automatic offer (Contributor)
• @mollfpr requires payment (Needs manual offer from BZ)
• @aswin-s requires payment automatic offer (Contributor)

[melvin-bot]

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:

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

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:
[@mollfpr] 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:

New feature.

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

1. Open any chat
2. Enter an emoji via a native keyboard, emoji picker, or short code (eg- ❤️)
3. Verify that the emoji gets appended by a white space.
4. Verify that emojis can be removed by tapping backspace.
5. Verify that the cursor is positioned properly after inserting the emoji.
6. Verify that emoji can be inserted in the midst of text
7. Verify that an emoji can be copy pasted
8. Verify that complex emoji like '👨‍👩‍👧‍👦' can be added and removed.
9. Verify that while pasting text, white space is added only when emoji is pasted. Whitespace shouldn't be added if the pasted text has any other characters other than emoji, including whitespace.
10. Verify that emojis with different skin tones can be inserted

---

@CortneyOfstad Could you give the payment summary for manual request? Thank you!

[CortneyOfstad]

@aswin-s — I sent you both offers as the reporter and the contributor. Please let me know once you accept those and I'll get them paid ASAP.

@mollfpr payment summary is below!

[aswin-s]

Payment Summary

@aswin-s (reporter) — to be paid $50 via Upwork

@aswin-s (contributor) — to be paid $2000 via Upwork

@mollfpr (C+) — to be paid $2000 via NewDot

@CortneyOfstad The issue was reported before the bounty for issues were reduced to 50$.

[CortneyOfstad]

Regression test listed here 👍

[CortneyOfstad]

Sorry about that @aswin-s! I adjusted the payment to be $250, and will update the payment summary above. You should have received both payments successfully, totaling $2,250 👍

[situchan]

We had to revert first PR due to regressions: #31006 #30950 #30983
2nd PR also caused regression but we closed it as minor.

Hope this info won't affect payment (no other contributors were involved so no overpaid) but worth mentioning.

[CortneyOfstad]

Shoot — that does reduce the payment and it was already completed 🙃 Going to post in Open-Source on how to get this corrected.

[CortneyOfstad]

Alright, @aswin-s I requested a refund of 50% ($1000) as the contributor. Please let me know as soon as you complete this. Thank you!

[aswin-s]

@CortneyOfstad Let me know if I can do anything from my end

[aswin-s]

Alright, @aswin-s I requested a refund of 50% ($1000) as the contributor. Please let me know as soon as you complete this. Thank you!

Thank you

[CortneyOfstad]

Thank you so much @aswin-s — I greatly appreciate it!

[CortneyOfstad]

Alright — here is the FINAL payment summary 😂

Payment Summary

@aswin-s (reporter) — paid $250 via Upwork ✅
@aswin-s (contributor) — paid $1000 via Upwork ✅ (50% cut due to regression)
@mollfpr (C+) — to be paid $1000 via NewDot (50% cut due to regression)

[aswin-s]

@CortneyOfstad @situchan Quick question. The regression was caught during staging itself and fix was raised the very next day. So payment would be deducted even if developer fixes it even before it hits PROD? Just asking so that I can be more careful next time.

[CortneyOfstad]

@aswin-s Thanks for asking! Each error in proposed PRs will carry different weights based on the type of error and the impact it would have, so in some cases (like this one) yes, it will cause a reduction in payment.

[situchan]

There are some exceptions. i.e. regressions penalties won't apply if already discussed as out of scope in PR.
And too much efforts working on PR.
i.e. Ideal Nav PR caused so many regressions but it was super large and reviewers won't get payment deducted.
But usually penalty is applied as long as bug is caught after PR is merged, whether in main branch, staging, or production

[JmillsExpensify]

$1,000 approved for @mollfpr based on this summary.