[HOLD for payment 2023-04-10] [$1000] Emoji picker moves to top left side when using Tab key

[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. Open app
2. go to any chat
3. Hover over any message
4. Use tab key to "add reaction" icon

Expected Result:

emoji picker should not move to top left side

Actual Result:

emoji picker moves to top left side

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:

Screen.Recording.2023-03-22.at.7.20.34.PM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e7e5fc421067468c
• Upwork Job ID: 1640667623552278528
• Last Price Increase: 2023-03-28

[MelvinBot]

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

@JmillsExpensify Huh... This is 4 days overdue. Who can take care of this?

[JmillsExpensify]

We don't officially support keyboard navigation, though I agree that this is an issue and I can reproduce it. @stitesExpensify and @hannojg thoughts on prioritizing a fix for this here, vs closing it and coming back to keyboard navigation throughout the app? I tend to favor closing it, though I thought I'd check with either of you first.

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[esh-g]

@JmillsExpensify I investigated this and this is definitely a one line fix as we are not following the recommended approach for handling refs in functional components. Fixing this would also help in adhering to the conventions laid out by react. I didn't post a proposal as the help Wanted label isn't present, but let me know if I should do so. 😇

[JmillsExpensify]

I'll go ahead and apply the external label so you can get ahead on that one. I'd still like input from Brandon/Hanno when they get a chance though!

[MelvinBot]

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

[MelvinBot]

Current assignee @JmillsExpensify 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 @madmax330 (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[esh-g]

Proposal

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

The emojiPicker shows on the top left corner of the web page when pressing the add reaction icon with enter key.

What is the root cause of that problem?

The root cause is that the anchor ref.current becomes null while executing the EmojiPickerAction.showEmojiPicker method.

App/src/components/Reactions/MiniQuickEmojiReactions.js

Lines 51 to 63 in 2bd8b48

	const MiniQuickEmojiReactions = (props) => {
	const ref = React.createRef();
	const openEmojiPicker = () => {
	props.onPressOpenPicker();
	EmojiPickerAction.showEmojiPicker(
	props.onEmojiPickerClosed,
	(emojiCode, emojiObject) => {
	props.onEmojiSelected(emojiObject);
	},
	ref.current,
	);
	};

This happens because we are executing props.onEmojiPickerClosed on line 55 before EmojiPickerAction.showEmojiPicker, which causes the component to re-render. And when pressing the enter key, the re-render happens twice.
Due to the re-render, the ref gets redefined every time that happens so it is null when the next line executes.

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

We can solve this problem by using the useRef hook instead of react.createRef which is the recommended way to use refs in functional components.
This hooks is meant exactly for this type of circumstance where we don't want the ref to get re-defined on every re-render.

After applying

Screen.Recording.2023-03-22.at.11.06.57.PM.mov

Additional Details: There are only two files in the codebase where we are using React.createRef in functional Components. One is in MiniQuickEmojiReactions.js as listed above and the other is in AddReactionBubble.js

What alternative solutions did you explore? (Optional)

We can also

• Use a class based component approach and use refs that way
• Store the ref temporarily in a variable before executing props.onEmojiPickerClosed

Both these options have been tested as well.

[sobitneupane]

Proposal from @esh-g looks good to me.

🎀👀🎀 C+ reviewed
cc: @madmax330

[madmax330]

I agree @sobitneupane

@JmillsExpensify you can assign the job to @esh-g on upwork

[MelvinBot]

📣 @esh-g You have been assigned to this job by @JmillsExpensify!
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 📖

[JmillsExpensify]

Sounds good! @esh-g is assigned to this GH issue. In addition, please apply to the Upwork job and we'll already have that in place once the regression period has passed.

[esh-g]

Thanks for assigning me! The PR will be ready shortly.
Applied on Upwork with profile: https://www.upwork.com/freelancers/~0186b1d7cc69656f22

[JmillsExpensify]

Great! I sent you the initial offer. We can apply the 50% bonus on final payout, if that ends up applying.

[esh-g]

PR is ready! 🚀

cc @madmax330 @sobitneupane @JmillsExpensify

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

• Emoji Reaction createRef -> useRef #16676

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

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

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:

• [@sobitneupane / @madmax330] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane / @madmax330] 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 / @madmax330] 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:
• [@JmillsExpensify] 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.
• [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[esh-g]

@JmillsExpensify Is this issue due? I hope that there were no regressions...

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

#15210

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

#15210 (comment)

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

https://expensify.slack.com/archives/C049HHMV9SM/p1681283318136189

[sobitneupane]

Regression Test Proposal

• Click on the "+ reaction" option while hovering on any report
• Verify that the emoji picker is correctly positioned.
• Navigate to the "+ reaction" option using tab and press enter
• Verify that the emoji picker is correctly positioned.
• After adding a reaction, click on the AddReactionBubble that appears right of the added reaction.
• Verify that it is positioned correctly.

Do we agree 👍 or 👎

[stitesExpensify]

Looks good to me! Just a couple small changes to make it more clear

• Click on the + "add reaction" icon while hovering a message on any chat
• Verify that the emoji picker is correctly positioned.
• Navigate to the + "add reaction" option using tab and press enter
• Verify that the emoji picker is correctly positioned.
• After adding a reaction, click on the add reaction bubble that appears to the right of the added reaction.
• Verify that it is positioned correctly.

[MelvinBot]

@JmillsExpensify, @madmax330, @sobitneupane, @esh-g Huh... This is 4 days overdue. Who can take care of this?

[JmillsExpensify]

Love those updates. I'm going make just a couple more to mimic the formatting we use in TestRail.

• Click on the + "add reaction" icon while hovering a message on any chat
• Verify that the emoji picker is correctly positioned.
• Navigate to the + "add reaction" option using tab and press enter
• Verify that the emoji picker is correctly positioned.
• After adding a reaction, click on the add reaction bubble that appears to the right of the added reaction.
• Verify that it is positioned correctly.

[JmillsExpensify]

Alright, in terms of settling up for this issue, it looks like we have the following:

• Issue reporter: @gadhiyamanan $250
• Contributor: @esh-g $1,500 (incl. 50% bonus)
• Contributor+: @sobitneupane $1,500 (incl. 50% bonus)

I've sent contracts to the issue reporter as well as contributor plus. I'll pay bonuses out when issuing payment, which by the way, @esh-g I've just done that very step for you.

[gadhiyamanan]

@JmillsExpensify accepted, thanks!

[JmillsExpensify]

Thanks, all paid out.

[sobitneupane]

@JmillsExpensify Accepted the offer.

[JmillsExpensify]

Awesome, payment sent. Closing this one out!