[HOLD #16078][$2000] suggestion for Emoji remains even after sending message

[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. Go to any chat
2. Type for any emiji ( do not select suggested emoji)
3. Send message

Expected Result:

Suggested emoji should be removed after sending the message

Actual Result:

Emoiji suggestions shows

Workaround:

unknown

Platforms:

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

• [ ] Android / native
• [x] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.2.92-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:

https://user-images.githubusercontent.com/43996225/224875615-cecd9a1d-1424-46f2-b08e-51a0f88da5a2.mp4

Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678718820449989

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01eccf0dd16e54ab9d
• Upwork Job ID: 1643225880592248832
• Last Price Increase: 2023-04-24

[MonilBhavsar]

@miljakljajic can we please double the bounty here to get some more eyes.

[MelvinBot]

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

[s77rt]

Not overdue. Still waiting for proposals. I think we should double $

[MonilBhavsar]

Agree, I'll double the bounty cc @miljakljajic

[MelvinBot]

Upwork job price has been updated to $2000

[koko57]

Hi I'm Agata from Callstack - expert contributor group - maybe we could help with this one?

[s77rt]

@koko57 Sure! @MonilBhavsar Would you please assign @koko57 as per the process?

[JediWattson]

Proposal

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

in Chrome, onBlur in the composer is not called when clicking the send button

What is the root cause of that problem?

Apparently in Chrome there's some precedence with the composer and send button where clicking the button doesn't blur the composer input. This causes an issue because onBlur is required to be called when submitting since it's the only way to reset the emojis.

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

in the submitForm function I would add a check and call to resetSuggestedEmojis such as:

if (this.state.shouldShowSuggestionMenu) {
    this.resetSuggestedEmojis();
}

this would ensure the reset is called onSubmit and prevent multiple calls when shouldShowSuggestionMenu is already false.

What alternative solutions did you explore? (Optional)

n/a

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[MelvinBot]

📣 @JediWattson! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[JediWattson]

Contributor details Your Expensify account email: robertjd333@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01c62599ed4a878d8e

[MonilBhavsar]

@s77rt sure. Could you please go through the latest proposal and confirm if it does really fix this issue?

[alitoshmatov]

https://github.com/Expensify/App/issues/15934#issuecomment-1520996432 This solution is proposed many times, and considered as a workaround hack.

[MelvinBot]

📣 @koko57 You have been assigned to this job by @MonilBhavsar! Please apply to this job in Upwork 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 📖

[s77rt]

@JediWattson Thanks for the proposal. The RCA is is not correct. We intentionally keep the input focused (by preventing default mouse down event).

[s77rt]

@MonilBhavsar It does fix the issue, but not fixing the root cause.

[MonilBhavsar]

Thanks! Got it. Wanted to make sure If I was not missing anything

[koko57]

Thanks for assigning me - yesterday, while working on another issue that also touched resetting emoji suggestion, I tried to investigate if this one could be fixed with my changes. Unfortunately, it's much more complex, so the issue will be investigated by our team tomorrow.

[bionanek]

Hi there! I'm Jakub from Callstack - expert contributor group - I am going to be taking this task over from @koko57 as she is already assigned to another issue.

[MelvinBot]

📣 @bionanek! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[s77rt]

@MonilBhavsar Please assign @bionanek instead.

[MelvinBot]

📣 @bionanek You have been assigned to this job by @MonilBhavsar! Please apply to this job in Upwork 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 📖

[bionanek]

Hi there! Just a quick update from me: I was investigating this issue on different platforms and it did seem like it's a React-Native-Web issue to me at first, specifically with TextInput's selectionChange event , because the onSelectionChange method was not being called after the emoji was inserted into the composer and I thought that the root might be somewhere around programmatically updating the value prop (which is what emoji replacement is). But then I have noticed that if I updated the state with some random string value it did work properly. Then when I thought maybe it's the issue with emoji itself rather than text input, so I changed the line responsible for replacing text with emoji in EmojiUtils -> replaceEmojis function like so

if (isSmallScreenWidth && i === emojiData.length - 1 && text.endsWith(emojiData[i])) {
    emojiReplacement += ' ';
}
newText = newText.replace(emojiData[i], 'test123');

and this actually make the suggestion box disappear properly. So it seems that the issue must be somewhere around inserting emoji itself.

Due to the bank holiday here in Poland I will be able to continue investigation and prepare the proposal on Tuesday.

[s77rt]

@bionanek Thanks for the investigation but I don't think this is related to how the emoji is inserted. The bug is reproducible with a simple text and even on ReactJS. Please check https://github.com/Expensify/App/issues/15934#issuecomment-1510454077 (and other comments above).

[s77rt]

Not overdue. Waiting for a proposal.

[miljakljajic]

Waiting

[MonilBhavsar]

@bionanek any update here?

[bionanek]

@MonilBhavsar sorry for late update.

I was doing some testing around React, React Native and React Native Web in this matter, I was also doing some digging in their source codes but I can agree that the issue seems to be under what e.preventDefault() must be doing under the hood - not resetting the selection state. Just as @alitoshmatov explained in his proposal

I've noticed that on older version of Chromium there was also an issue not only when you'd click on the button and fire e.preventDefault() but when you'd just programmatically replace a content of a text box with an empty string without loosing focus. So perhaps this issue might get fixed in the upcoming updates gradually. I've tested it on Android Chrome v101.0.4951.74 and v113.0.5672.76. This was also the reason for my previous assumptions which where incorrect due to the different Chrome version I was using.

For now, I think the best solution would be to introduce a simple workaround (one form many already proposed here on this issue) as this issue seems just not significant enough to spend more resources on it and get back to it in the future if needed.

[s77rt]

@bionanek Thanks for the update, but this does not answer my concerns on why the "Fill" button works. Please refer to the previous comments such as https://github.com/Expensify/App/issues/15934#issuecomment-1510454077.

[bionanek]

hi - I'm currently debugging React code searching for the issue with onSelect implementation, I will get back with more info on Monday!

[MonilBhavsar]

Not overdue, we're discussing and making sure we all agree to the root cause of the issue

[bionanek]

Hello everyone, I've been debugging React to see if this issue comes from it, to be more specific - SelectEventPlugin, DOMPluginEventSystem and ReactDOMEventListener. I've used the exact same code as in the mentioned example and I've noticed that in case of clearing the input by pressing the Clear button, or highlighting a text by dragging the mouse over it and then hitting 'backspace' button React doesn't even receive the selectionchange DOM event.

That makes me think that the bug comes from Chromium itself, and there are currently 2 issues opened in Chromium bugs repository related to deleting text and selectionchange event being problematic when deleting text.

• Issue 1405597: selectionchange not fired when deleting text in a contenteditable

• Issue 725890: selectionchange event not firing after backspace

In React, SelectEventPlugin is constructing select event when, for example, onmouseup event gets fired when you click on "Fill" button. But when you do this React doesn't yet receive the selectionStart and selectionEnd values as we expect them to be (7-7), but it receives values 0-0.

See logs from my local React debug session:

As you can see the selectionchange event forces React to compare new selection value with the old one and it results in no changes It's the log that says: constructSelectEvent, last selection was null or equal to current selection, returning

Right after that, the selectionchange DOM event gets fired and we get updated selection values. These are my logs right after the ones you can see on the screenshot above:

And here, selection values get compared again, but now React sees differences and it fires the construction of selection event and the it can dispatch it.

Now, when we hit the "Clear" button

You can see that selectionchanged event is never received by React and so the selection values are never updated and the comparison with the old value results in no changes. Because of that React never dispatched the onSelect event.

This is the most I could get from this issue, and if you go through Chromium repository and search for selectionchange related issues you can see that there are some random looking unexpected behaviours and they are quite a low priority, some of them are opened for a long time now.

Because of that I would suggest not to rely on onSelect too much as it can cause more potential side-effects in the future.

[s77rt]

@bionanek Thanks a lot for the explanation. I have been looking into this and I agree with your analysis; Chrome does not emit selectionchange event if you delete (press backspace) or clear the text programmatically (thus we don't constructSelectEvent).

https://user-images.githubusercontent.com/16493223/236942130-8804287e-9f95-415b-aead-fca20949675d.mp4

I only have one slight note:

But when you do this React doesn't yet receive the selectionStart and selectionEnd values as we expect them to be (7-7), but it receives values 0-0.

This is actually the expected value since the moueup event fires before the click event (where we clear the value). I think the mouseup event is irrelevant (same goes for mousedown).

And one question :grin:: What do you propose to fix the issue?

[bionanek]

Hi @s77rt !

Sorry, but yesterday I had quite a busy day and couldn't follow up on this one.

I only have one slight note:

But when you do this React doesn't yet receive the selectionStart and selectionEnd values as we expect them to be (7-7), but it receives values 0-0.

This is actually the expected value since the moueup event fires before the click event (where we clear the value). I think the mouseup event is irrelevant (same goes for mousedown).

Thanks for claryfying on this one. After a long debugging session sometimes brain gets confused :D

And one question 😁: What do you propose to fix the issue?

Since we identified this as a Chromium bug I would suggest to not depend on the onSelectionChanged call, and instead call this.calculateEmojiSuggestion(); within updateComment(comment, shouldDebounceSaveComment) function. This approach gives us 2 benefits:

• more reliable behaviour, since updateComment is invoked by Composer's onChangeText function
• if we call this.calculateEmojiSuggestion(); from updateComment it gets called fewer times, avoiding unnecessary calls of this function

One more thing to add - I would remove a call to resetSuggestedEmojis from onBlur callback and instead refactor setIsFocused function to either call calculateEmojiSuggestion if shouldHighlight is true or resetSuggestedEmojis if it's false. This way we will get an emoji suggestion box after we re-focus the input in which there's a piece of emoji code or hide the box on blur.

Like so:

setIsFocused(shouldHighlight) {
    this.setState({isFocused: shouldHighlight});
    if (shouldHighlight) {
        this.calculateEmojiSuggestion();
        return;
    }

    this.resetSuggestedEmojis();
}

[s77rt]

@bionanek Thanks for the update. I don't think we can get ride of onSelectionChanged because the emoji suggestion list is calculated based on cursor position as well and not just on the text. e.g. type hi :wave test :smile notice the suggestion list getting updated based on cursor position.

I think we will just reset the suggestion list on clear as suggested by @dukenv0307 https://github.com/Expensify/App/issues/15934#issuecomment-1495986845 But I'm not sure how the compensation would work in this case.

@bionanek Do you think there is a better approach here?

[bionanek]

Hey @s77rt, I wasn't clear yesterday - what I meant is to not depend on onSelectionChanged when it comes to calling this.calculateEmojiSuggestion().

But of course we need to leave it to just update the selection state that, as you mentioned, is required to calculate the emoji suggestions. Resetting suggestion list on clear will not work in case when user selects the input content and delete it all or cut it out.

So again - my suggestion is to leave onSelectChanged and make it only update the selection value, and call the calculateEmojiSuggestion in updateComment as I described above as it's called on every text change.

I've tested my proposal locally and it seems to be working fine, no issues with emoji suggestions calculations.

[bionanek]

Here you can see the result of the implementation I'm proposing

Here's web: https://github.com/Expensify/App/assets/16389499/a3aeb474-72c9-4120-bf18-766c6c357e32

Here's mobile android just to showcase that it's not braking the behaviour: https://github.com/Expensify/App/assets/16389499/af2f088b-827e-4b5a-b812-0fab7fdff89f

[s77rt]

@bionanek I understood what you meant and I think we still do need to depend onSelectionChange to calculateEmojiSuggestion. Otherwise the selection won't get updated on when changing cursor position:

with onSelectionChange	without onSelectionChange

---

I see your point on the failing cases of onClear. Those only fail on Chrome right? (due to selection bug). I think we may just ignore those cases and consider them as an edge case especially that they are primary related to a browser bug. Calculating the emoji suggestion on comment update seems a lot of work.

I will ask on Slack for more :eyes: on this.

[s77rt]

Asked on Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1683900085329619

[MonilBhavsar]

@bionanek are you in #expensify-open-source? Sorry I couldn't find you. We're discussing and could try to match with slack's way of handling this issue.

[MonilBhavsar]

Melvin, please see my above comment 😕

[thiagobrez]

Hey @MonilBhavsar! I'm Thiago from Callstack - expert contributor group - and I'll be taking this over from Jakub as he is already assigned to something else :)

[MonilBhavsar]

@thiagobrez thank you! I'll assign you to the issue.

[melvin-bot[bot]]

📣 @thiagobrez You have been assigned to this job by @MonilBhavsar! Please apply to this job in Upwork 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 📖

[thiagobrez]

Following on the slack thread, suggestion to reset emojis when onClear works well (also for @mentions) and it's simple enough.

I'll investigate the other suggestion which is making the suggestion menu a Modal that disables other actions (such as sending the message) until you dismiss it

[thiagobrez]

Update: I'm able to use the Modal component to render the emoji suggestions with a backdrop disallowing the user to do other actions while the modal is open. Looks promising, but still has some conflicts I need to solve before I can come up with a proposal

[thiagobrez]

Update: Got to something reasonable using the Modal component. Posted in the slack discussion the pros and cons

[thiagobrez]

Proposal

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

On web, when clicking to send a message, if the emoji or mention suggestion view is open, the message will go through but the suggestions will remain opened (not cleared).

What is the root cause of that problem?

This is due to a bug only on Chrome, that is not firing the selectionchange event if the textinput is cleared programatically or in some edge cases (e.g. using Cut text). We rely heavily on the selectionchange event because suggestions change as you move the cursor.

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

There was a lot of discussions both in this issue and in the slack thread. We decided to go with an approach similar to Slack, which is making the suggestion view a Modal with invisible backdrop, so the composer actions won't be clickable until you close the modal.

components/AutoCompleteSuggestions/BaseAutoCompleteSuggestions.js:

• Change View to a Modal, not fullscreen, with backdrop, backdropColor transparent, and a new type of COMPOSER_SUGGESTIONS

components/Modal/BaseModal.js:

• Add backdropColor prop and hasBackdrop prop

components/Modal/index.web.js:

• Add mousedown listener so we're able to use the already existing prop shouldCloseOnOutsideClick on web too

pages/home/report/ReportActionCompose.js:

• Remove a code block that automatically focus the input when modals are closed (lines 255-257). This was causing the input to flicker when closing the new suggestions Modal. Also, with this new improvement being worked on (automatically focusing input when typing any letter), this code block will not be needed anymore
• In the onSelectionChange callback, only calculate the new emoji and mention suggestions if the new selection is different from the previous one. This prevents unnecessary state updates and re-calculating the suggestions when the focus went back to the input when closing the Modal

What alternative solutions did you explore? (Optional)

• Calling resetSuggestions() in Composer onClear. This works and it's very simple, but as discussed on Slack, the first proposal is more solid.

https://github.com/Expensify/App/assets/26878038/72f9b4d1-8146-46ac-bb07-d81cfdbe1187

[s77rt]

@thiagobrez Thanks for proposal. Your RCA is correct. The general idea of the suggested fix makes sense. But can you please explain why we would need to add the mousedown event listener? Isn't coverScreen={false} with hasBackdrop={true} enough for this? Also we need to keep the functionality that refocus text input on modal close. The linked PR may not get merged based on this comment. Any alternative way to prevent the flicker?

[thiagobrez]

@s77rt Thanks!

But can you please explain why we would need to add the mousedown event listener? Isn't coverScreen={false} with hasBackdrop={true} enough for this?

I'm using coverScreen={false} and hasBackdrop={true}, but it's not enough because the Modal will only close when the backdrop is pressed. And in this case, the backdrop only exists in the very bottom of the screen up until where the Modal is placed, so clicking anywhere else in the rest of the application would not close the Modal.

This was probably never noticed before because most of Modals use the default fullscreen={true}

After further investigation, I just found out that the shouldCloseOnOutsideClick implementation only existed in Modal/index.js (which I suggested copying to Modal/index.web.js), and this file is not being loaded at all, because we have all the other platform extensions being used:

• Modal/index.ios.js
• Modal/index.android.js
• Modal/index.web.js
• Modal/index.js <-- never being loaded, probably a leftover

So, in short, my suggestion is:

• Delete Modal/index.js
• Rename Modal/index.web.js to make it the new index.js
• Copy the mousedown listener to the new index.js, as I previously suggested in the Proposal

[thiagobrez]

Also we need to keep the functionality that refocus text input on modal close. The linked PR may not get merged based on this https://github.com/Expensify/App/pull/17649#issuecomment-1533116613. Any alternative way to prevent the flicker?

I think we can keep the functionality 👍🏻 . It's not really a flicker, It's just something I thought it was better UX. But looking at it again now, I think it works well

With refocus	Without refocus