[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 contributors@expensify.com 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 https://github.com/Expensify/App/issues/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?

• [x] Android / native
• [ ] Android / Chrome
• [x] 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

https://github.com/Expensify/App/assets/93399543/b833037d-c850-44d0-805c-e8d6fd5e78fd

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[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] 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:

https://github.com/Expensify/App/assets/13544910/cc484e2f-2b73-497f-ab50-d6192d2a33d0

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[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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[bot]]

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

[melvin-bot[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[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

[samh-nl]

@mollfpr Thanks and yes that would be the general approach, please see the edited proposal.

I've been trying the implementation but face a strange behavior in RN. If the input value is modified during a composition event (as is the case when inserting an emoji via the native keyboard), the input event is sent again leading to a duplicate emoji.

[mollfpr]

Thanks @samh-nl. I'll try your updated proposal.

[melvin-bot[bot]]

@CortneyOfstad @mollfpr @zanyrenney this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[zanyrenney]

Any update on the proposal review?

[melvin-bot[bot]]

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

[ShogunFire]

Proposal

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

When we add an emoji with the native keyboard no space is added

What is the root cause of that problem?

There is no code to handle space when we add a native emoji

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

We can add a space after the last emoji if it is at the end of the text, we also need to check that the text is longer than before otherwise the space will be added even when we remove it and we need to check that the cursor is at the end

https://github.com/Expensify/App/assets/19537677/aa5aaec5-4388-4765-9473-d6cbfbc75774

Here is my commit if you want to have a look: https://github.com/Expensify/App/compare/main...ShogunFire:App:addSpaceAfterNativeEmoji

What alternative solutions did you explore? (Optional)

[ShogunFire]

@mollfpr I made a proposal without using onChange because I am wondering if using onChange will be regression free

[mollfpr]

Sorry for the delay 🙏

After consideration, we shouldn't change the onChangeText to avoid regression.

@ShogunFire I'm not on board with adding a new package just for this case. I think we can check the emoji from the last char of the text from updateComment.

[ShogunFire]

@mollfpr I can do that quite easily but the problem with that method is that if there is an emoji at the end, we will add a space, now if the user wants to suppress the last part of the comment, let's say the emoji, he will suppress the space first which will trigger updateComment and add the space again, the user would be stuck. Would you like my proposal better if I didn't use any library (basically recreating a function to see what changed) ?

[mollfpr]

@ShogunFire I like to see how it goes. Let's put it in your alternative solution.

[ShogunFire]

Proposal

Updated

[ShogunFire]

This is very simplified version, I actually just made a diff of the emojis, we can check more things if necessary for example:

• Check that only one emoji was added
• Count the occurences of each emoji to know if one that existed before has more occurences now
• Check the index of the new emoji so that we can add a space to the right one if there was already a similar one existing

Tell me if it's better

[samh-nl]

@ShogunFire To be sure it works in practice, I would test the solution (if you haven't already), to check that RN does not give the same behavior that I encountered with composition events.

I made a proposal without using onChange because I am wondering if using onChange will be regression free

After consideration, we shouldn't change the onChangeText to avoid regression.

onChangeText is simply a wrapper for onChange. I'm not suggesting to re-evaluate my proposal, because it still has the problem I mentioned that I haven't solved, but I don't see how onChange inherently could lead to a regression. We use it elsewhere too, but perhaps I have misunderstood what you mean.

[CortneyOfstad]

@zanyrenney I'm back from OoO, so I can take this back over — thanks for holding down the fort while I was gone!

[CortneyOfstad]

@mollfpr any thoughts on the comments above from @samh-nl and @ShogunFire?

[mollfpr]

Sorry for the delay. I'll get the review done in the morning.

[melvin-bot[bot]]

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

[CortneyOfstad]

@mollfpr just checking where we're at? TIA!

[mollfpr]

Sorry, I forgot to post the update.

@ShogunFire I'm curious why we need to loop over the diff char?

@samh-nl Is the problem you encounter can be solved? Or it's a regression from chancing the onChangeText?

[ShogunFire]

Hello @mollfpr Are you talking about the alternative proposal or the original one ?

For the original one I think that was pretty clear

This will return an array of objects, to verify that the only thing that was added was an emoji, we can go through that array and verify that only one of the object was added and that this object value is an emoji.

Have you also checked the alternative proposal without using libs ?

[ShogunFire]

Actually it's not that clear ahah, if the two strings look like this

String1 = "abc" String2 = "abbc"

array returned by diffChars looks like this ["ab", "b" added, "c"]

[mollfpr]

@ShogunFire The alternative solution. I'm afraid a nested loop every time the user typing will affect the performance.

[zanyrenney]

Unassigning as per - https://github.com/Expensify/App/issues/23658#issuecomment-1677609191

[ShogunFire]

@mollfpr I understand but it only loops through the emojis so that should not be too much. I don't think there is a way to know the differences between two texts without nested loops. If we can make onChange works without regression I agree this is a better solution.

[ShogunFire]

If I resolve the regression can we share the bounty @samh-nl ? :P

[samh-nl]

This adds the space in the right place and doesn't cause the double emoji behavior. There are two issues though:

• You notice a very short delay in the adding of the space.
• The cursor is moved to the end, I should be able to solve this, but the 1st point I don't think is.

updateCommentAndProcessEmoji(event) {
  this.updateComment(event.nativeEvent.text, true);

  if (event.nativeEvent.data && event.nativeEvent.isComposing && EmojiUtils.containsOnlyEmojis(event.nativeEvent.data)) {
    InteractionManager.runAfterInteractions(() => {
      this.replaceSelectionWithText(' ', false);
    });
  }
}

If pasting emojis should also be prepended with a space like is discussed, the event.nativeEvent.isComposing condition can be removed from the if-statement above.

A different way would be to ignore the composition event and use the emoji from the 'duplicate' event that is followed, however this seems like a workaround and therefore not a good solution.

@ShogunFire Sure I would be open to that.

[ShogunFire]

Is the isComposing boolean true for the second call also ? From what I have seen we can ignore the calls while it is composing.

If isComposing is true for the second call, we can calculate it ourself. I found this issue in react native https://github.com/facebook/react/issues/3926

And there are a lot of solutions for example this one:

https://github.com/facebook/react/issues/3926#issuecomment-1200414788

[ShogunFire]

I will try that soon

[melvin-bot[bot]]

@CortneyOfstad @mollfpr this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @mollfpr is eligible for the Internal assigner, not assigning anyone new.

[CortneyOfstad]

@mollfpr any updates here? TIA!

[samh-nl]

@ShogunFire In the second event isComposing was false during my testing (you can't distinguish it from an emoji paste).

[mollfpr]

@CortneyOfstad We still don't have a satisfactory proposal, but the solution I am looking forward to is still under discussion by @samh-nl @ShogunFire

Could we make this issue external again?

InteractionManager.runAfterInteractions

@samh-nl Can we have the ready proposal now? Also, what is the delay purpose before adding the space?

[ShogunFire]

Then I think we can just ignore the call to onChange if isComposing is true, I am testing it, last question, when you were testing you were using android or ios ? I have a problem on android, event.nativeEvent.data is undefined, I haven't tried on ios

[ShogunFire]

I only have target, eventCount and text in nativeEvent

[samh-nl]

@ShogunFire Yes you're right, isComposing is only available via the native emoji keyboard on (mobile) web. A different approach I have in mind still requires InteractionManager.runAfterInteractions, but I'm not satisfied with this approach because using this has a small (but noticeable) delay before the space is added, so if anyone has a better idea, feel free.

@mollfpr It's to ensure it's processed outside of the event, which prevents problems with duplicate emojis being inserted (at least on web).