[HOLD for payment 2023-05-16] [$1000] The Emoji suggestion is flickered when adding attachment

[kavimuru]

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 any chat
2. Entering :smi and observe the emoji suggestion is appear
3. Click + button on the left of composer
4. Click on Add Attachment and observe that the emoji suggestion is appear on a moment
5. Close attachment and observe the composer is focus but the emoji suggestion is not appeared

Expected Result:

The emoji suggestion is not flickered and it should be displayed when the composer is focus

Actual Result:

The Emoji suggestion is flickered when adding attachment and it isn’t displayed when the composer is focus

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.95-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: Any additional supporting documentation

Screen.Recording.2023-04-05.at.09.39.19.mov

Recording.141.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b73cd2c4c5df346d
• Upwork Job ID: 1643787501583937536
• Last Price Increase: 2023-04-27

[MelvinBot]

Triggered auto assignment to @arielgreen (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]

Job added to Upwork: https://www.upwork.com/jobs/~01b73cd2c4c5df346d

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[jjcoffee]

Proposal

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

The emoji picker is appearing in between selecting the attachment menu item and the file selector opening.

What is the root cause of that problem?

We are inadvertently returning focus to the composer when the attachment menu item is selected. This is because we are forcing focus here:

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

Lines 232 to 238 in 313c575

	// We want to focus or refocus the input when a modal has been closed and the underlying screen is focused.
	// We avoid doing this on native platforms since the software keyboard popping
	// open creates a jarring and broken UX.
	if (this.willBlurTextInputOnTapOutside && this.props.isFocused
	&& prevProps.modal.isVisible && !this.props.modal.isVisible) {
	this.focus();
	}

The issue is we are only checking for modal.isVisible, but the file selector is not a modal so we are always going to return focus to the composer input when the file selector opens.

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

I suggest we add a new state, isFilePickerOpen, which we set to true when the file picker is opened and which then will be used to block the emoji suggestions from showing. We then set it back to false when either the window receives focus (i.e. on cancel) or the AttachmentModal is closed. Unfortunately, when the file picker is dismissed the order of events that fire (i.e. onChange and window focus) is inconsistent between browsers/platforms, so we need a workaround (I have a few options below) to ensure it works everywhere.

First we need to update this section to add the new state and an event listener for the window regaining focus.

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

Lines 783 to 790 in 313c575

	menuItems={[...this.getMoneyRequestOptions(reportParticipants),
	{
	icon: Expensicons.Paperclip,
	text: this.props.translate('reportActionCompose.addAttachment'),
	onSelected: () => {
	openPicker({
	onPicked: displayFileInModal,
	});

onSelected: () => {
    this.setState({isFilePickerOpen: true});
    window.addEventListener('focus', this.filePickerClosed);
    openPicker({
        onPicked: displayFileInModal,
    });
}

When the window regains focus we want to force focus onto the composer:

filePickerClosed() {
    this.focus();
    window.removeEventListener('focus', this.filePickerClosed);
}

By forcing focus without setting isFilePickerOpen to false, we get the existing behaviour of the composer being focused but the emoji suggestions not showing. Current behaviour here:

emoji-flicker-issue-original-cancel-behaviour-2023-04-12_15.01.41.mp4

We can then set isFilePickerOpen to false on a click event on the composer, i.e. onClick={() => this.setState({isFilePickerOpen: false})}, and then the emoji suggestions will appear.

To handle a file being selected, we can need to also reset the state in when the AttachmentModal is closed here:

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

Lines 716 to 719 in 5fadec9

	<AttachmentModal
	headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
	onConfirm={this.addAttachment}
	>

onModalHide={()=> {this.setState({isFilePickerOpen: false})}

Now we can add the !this.state.isFilePickerOpen check here to avoid the composer being focused whilst the file picker is open:

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

Lines 246 to 249 in ab9b7d9

	if (this.willBlurTextInputOnTapOutside && this.props.isFocused
	&& prevProps.modal.isVisible && !this.props.modal.isVisible) {
	this.focus();
	}

And finally we need to use the isFilePickerOpen state as an additional condition before showing the emoji suggestions, here:

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

Line 910 in 5fadec9

{!_.isEmpty(this.state.suggestedEmojis) && this.state.shouldShowSuggestionMenu && (

so that it becomes !_.isEmpty(this.state.suggestedEmojis) && this.state.shouldShowSuggestionMenu && !this.state.isFilePickerOpen &&

Demo:

emoji-flicker-issue-updated-fix-2023-04-12_16.14.17.mp4

What alternative solutions did you explore? (Optional)

Simpler alternative solution is to add some sort of state flag shouldBlockEmojiCalc (I haven't thought too hard about the name) here:

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

Lines 783 to 790 in 313c575

	menuItems={[...this.getMoneyRequestOptions(reportParticipants),
	{
	icon: Expensicons.Paperclip,
	text: this.props.translate('reportActionCompose.addAttachment'),
	onSelected: () => {
	openPicker({
	onPicked: displayFileInModal,
	});

onSelected: () => {
    this.setState({shouldBlockEmojiCalc: true});
    openPicker({
        onPicked: displayFileInModal,
    });
}

We can then add this as a check before going ahead with calculateEmojiSuggestion, which will block the suggestions from showing, and then reset the flag, which stops us needing to deal with the cancel button press (e.g. using a window focus event listener).

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

Lines 416 to 417 in 0276f42

	calculateEmojiSuggestion() {
	const leftString = this.state.value.substring(0, this.state.selection.end);

if (this.state.shouldBlockEmojiCalc) {
    this.setState({shouldBlockEmojiCalc:false});
    return;
}

[allroundexperts]

Proposal

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

The emoji selection suggestion is flickering when adding attachment

What is the root cause of that problem?

As correctly pointed out by @jjcoffee, the issue is that we're refocusing the composer once the popover closes. However, the fie selector isn't a popover. As such, the menu item opens for a short time before the file dialog opens.

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

Setting the state when the picker is open does not work because the state won't reset if the user will cancel the selection. To deal with this, we'll have to find a way to reset the state once the user cancels the selection. Also, I think adding more state variables in this file will just make the whole code more complex.
Instead, we can do something much simple to fix this. We have to replace this line with the following:

this.focus(true);

This will result in the focus being returned to the composer after a slight delay.

What alternative solutions did you explore? (Optional)

Alternatively, we can open the Picker before closing the Popover menu. This can be done by adding the following condition here:

onItemSelected={(item) => {
    if (item.text === this.props.translate('reportActionCompose.addAttachment')) {
        openPicker({
          onPicked: displayFileInModal
      });
    }
     this.setMenuVisibility(false);
}}

We'll also have to remove this callback because it gets triggered only when the menu closes.

Doing the above changes will also prevent the usage of state.

IF none of the above work, and our preference is to go with the state variable, then we'll have to add that state check on this line instead of the one mentioned in the proposal by @jjcoffee. We'll also have to modify the AttachmentPicker component, such that it calls a supplied callback whenever user cancels the upload as well. We'll then have to reset the state if a file is selected or the selection is cancelled.

[jjcoffee]

Updated proposal with extra details regarding handling cancelling the file picker.

@allroundexperts I don't think your first solution does anything as far as I can tell, since the delay is only set to 100ms, unless I'm missing something! I don't think your second solution does anything either, or maybe you can explain a bit more?

[Snehal-Techforce]

Proposal

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

The Emoji suggestion is flickered when adding attachment.

What is the root cause of that problem:

There are 2 issues which have the following root causes.

1. Emoji suggestion is flickered

A. When the user clicks on Add attachment, at that time emoji disables and opening of “Add attachment” picker modal events fires. Where for a fraction of time emoji displayed and disabled. Because the emoji component gets autofocus on blur and when the user clicks on “Add attachment” as soon as this event is called, it gets autofocus and due to the async event, it displays the emoji.

2. Emoji suggestion not visible on cancelation of attachment picker modal

A. Because of onBlur event of composer, suggestedEmoji array gets converted to an empty.

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

Line 833 in 64f4a78

this.resetSuggestedEmojis();

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

One new state “ isActionProcesed “ is required to be called to monitor progress of action. When “isActionProcessed” is true then emoji will not be displayed. So, by providing a timeout of 500 to 950 ms to “isActionProcessed” state needs to change. adding two additional conditions to display emoji suggestion modal, flickered Emoji suggestion issue can be fixed. ( image 1 & 2 )

To get emoji suggestions after cancellation to attachment modal, we have to remove the resetEmojisuggestion call from onBlur event. ( Image 3 )

What alternative solutions did you explore:
None

Expected result:

Resolved_Issue_16977.mp4

[sobitneupane]

say onFinishedPicking, which we can trigger when the window regains focus (which should reliably trigger on either cancel or opening a file)

@jjcoffee Can you please add the explanation on "which should reliably trigger on either cancel or opening a file". How you are planning to achieve this? What will be the changes in AttachmentPicker?

[sobitneupane]

@allroundexperts Thanks for your proposal.

The use of delays and setTimeout should be the last resort. So, I believe firstly, we should look for other solutions if any.

Your alternative solution does not look reliable. I can still reproduce the issue with the solution.

[sobitneupane]

@Snehal-Techforce Thanks for the proposal.

The use of setTimeout is highly discouraged. Let's look for the solution that doesn't use delays.

[jjcoffee]

@sobitneupane Sorry for not being clearer! I meant that we can add an event listener on the window to check when it receives focus, as this will reliably trigger when the file picker is closed (either via selecting a file, or via cancelling). As an example, my quick and dirty version was to update

App/src/components/AttachmentPicker/index.js

Lines 56 to 59 in b96c516

	openPicker: ({onPicked}) => {
	this.onPicked = onPicked;
	this.fileInput.click();
	},

to the following:

openPicker: ({onPicked, onFinishedPicking}) => {
    this.onPicked = onPicked;
    this.onFinishedPicking = () => {
        onFinishedPicking();
        window.removeEventListener('focus', this.onFinishedPicking);
    };
    window.addEventListener('focus', this.onFinishedPicking);
    this.fileInput.click();
}

Then we add the callback here

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

Lines 787 to 790 in b96c516

	onSelected: () => {
	openPicker({
	onPicked: displayFileInModal,
	});

onFinishedPicking: () => {
    this.setState({isFileSelectorOpen: false});
    this.focus();
}

[sobitneupane]

@jjcoffee Thanks for the update. There is one problem with the implementation. You can notice in the following video that emoji suggestion is shown for a brief moment after selecting an image which is not expected.

Screen.Recording.2023-04-11.at.14.03.48.mov

[jjcoffee]

@sobitneupane Ah good catch! On my device the modal appears instantly, so I wasn't getting the same issue. The solution would be to update AttachmentPicker so that we remove the window focus event listener when the onChange event fires. This effectively means onFinishedPicking will only run on cancellation (so I would rename it to something like onCancel). So the updated code in AttachmentPicker would look like:

openPicker: ({onPicked, onFinishedPicking}) => {
    this.onPicked = (file) => {
        onPicked(file);
        window.removeEventListener('focus', this.onFinishedPicking);
    };
    this.onFinishedPicking = () => {
        onFinishedPicking();
        window.removeEventListener('focus', this.onFinishedPicking);
    };
    window.addEventListener('focus', this.onFinishedPicking);
    this.fileInput.click();
}

(We could also remove the window focus event listener in the onChange event itself, but I thought it's a bit more readable to have all the window event stuff together. Happy to change if you disagree, of course!)

[sobitneupane]

The solution would be to update AttachmentPicker so that we remove the window focus event listener when the onChange event fires.

@jjcoffee I don't think it will work. Even if we remove the event listener on the onChange event, the call back function will be called.

[jjcoffee]

@sobitneupane I have tested by logging when the focus() function call (in ReportActionCompose) happens and it works as expected. The onChange event fires first, so when we remove the window focus event listener there onFinishedPicking will never fire, since nothing else is calling it. Unless I'm misunderstanding your point?

[sobitneupane]

@jjcoffee For me, onFinishedPicking is being called before onChange Event fires.

Also, Can you please update your original proposal incorporating the suggested changes.

[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.3.12-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Prevent emoji suggestions flicker on file picker opening on desktop web #18227

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

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 @tienifr
• Contributor that fixed the issue @jjcoffee
• Contributor+ that helped on the issue and/or PR @sobitneupane

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

[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:

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[arielgreen]

@tienifr @sobitneupane @jjcoffee offers sent in Upwork

[jjcoffee]

@arielgreen Accepted, thanks! Just a heads up this one's also due the 50% timeliness bonus (assigned Fri 28 April, merged Tues 2nd May).

[arielgreen]

Not overdue, regression period.

[AndrewGable]

Same, just waiting for payment.

[arielgreen]

Calculating payments:

• 4/28 ssue assigned to @jjcoffee
• 5/2 PR merged

2 business days = 50% bonus

$250 to @tienifr
$1000 + 50% = $1500 to @jjcoffee
$1000 + 50% = $1500 to @sobitneupane

All payments have been issued.

[arielgreen]

@sobitneupane can you please complete the checklist so we can close this issue?

[sobitneupane]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#14686

• [@sobitneupane] 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:

#14686 (comment)

• [@sobitneupane] 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:

It is just an edge case and can easily be missed while implementing a big feature. So, I don't think any update in the PR review checklist will help to catch such case.

[sobitneupane]

Regression Test Proposal

1. Type :smi to begin showing emoji suggestions
2. Click the + button on the left of the composer and click on Add Attachment
3. Ensure that the emoji suggestion doesn't flicker(especially between clicking of Add Attachment and opening of file picker) and remain hidden until the file picker is dismissed (either by cancelling, or selecting and opening a file)

Do we agree 👍 or 👎