[HOLD for payment 2023-09-13] [$1000] Emoji - When scrolling with the down arrows, the emoji selection disappears

[lanitochka17]

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 https://staging.new.expensify.com/
2. Open any conversation
3. Open the emoji window
4. Press the down arrow to scroll downward

Expected Result:

When scrolling with the down arrows, the emoji highlighting should remain in the user's field of view

Actual Result:

When scrolling with the down arrows, the emoji highlighting disappears, it wilts again after clicking on the other arrows

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome
• MacOS / Desktop

Version Number: 1.3.53.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6161613_Recording_emoji.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018ba7fbfbb4ccc435
• Upwork Job ID: 1692235536154705920
• Last Price Increase: 2023-08-31
• Automatic offers:
• DrLoopFall | Contributor | 26458681

[melvin-bot]

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

[DrLoopFall]

Proposal

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

When scrolling with the down arrows, the emoji selection disappears

What is the root cause of that problem?

Currently, we're using CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT for scrolling the highlighted emoji, which is set to 300 pixels, but the actual height is 256 pixels, which leads to an incorrect calculation of the scroll offset.

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Line 400 in ed2c287

targetOffset = offsetAtEmojiBottom - CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT;

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

Actually the scrollToHighlightedIndex is not needed as we already focus the selected emoji, which will automatically scroll to the emoji, it looks like scrollToHighlightedIndex was added as a workaround to fix the issue where the top row emoji was not scrolled properly due to using sticky headers.

We can completely remove scrollToHighlightedIndex and add scrollPaddingTop to the emoji list to fix the issue.

Add scrollPaddingTop in emoji list's style.

{scrollPaddingTop: isFiltered ? 0 : CONST.EMOJI_PICKER_ITEM_HEIGHT}

To scroll one row at a time change this and this

- this.ref.focus();
+ this.ref.focus({preventScroll: true});
+ this.ref.scrollIntoView({block: 'nearest'})

What alternative solutions did you explore? (Optional)

Changing the CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT to 256 pixels in src/CONST.js would fix the issue.

-    NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 300,
+    NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 256,

Or we can change CONST.EMOJI_PICKER_SIZE.HEIGHT to 460 pixels.

Alternate Solution 2

Use the actual height of the emoji picker list, for calculating the scroll offset.

[dummy-1111]

Proposal

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

In EmojiPicker, the emoji highlight disappears when scroll with the down arrow

What is the root cause of that problem?

In order to scroll to the highlighted emoji, we're comparing the highlighted emoji position with CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT now as below

App/src/components/EmojiPicker/EmojiPickerMenu/index.js

Line 399 in ed2c287

if (offsetAtEmojiBottom - this.currentScrollOffset >= CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT) {

This value is set to 300 now

App/src/CONST.js

Line 805 in ed2c287

NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 300,

But the height of EmojiPicker's list isn't always same with this value

The reasons are:

1. In wide-screen devices, EmojiPicker's height is set to 416 as you can see below
   

   App/src/CONST.js

   Line 798 in ed2c287

   HEIGHT: 416,

This includes search input(height: 72), category bar(height: 32), EmojiPicker list, and skintone list(height: 56). EmojiPicker list has style flexShrink: 1, flexGrow: 1 and so its height becomes 256

2. In small-screen devices, we don't explicityl set the height of the EmojiPicker as you can see below
   

   App/src/styles/StyleUtils.js

   Lines 593 to 597 in ed2c287

   	if (isSmallScreenWidth) {
   	return {
   	width: CONST.SMALL_EMOJI_PICKER_SIZE.WIDTH,
   	};
   	}

So the the height of the EmojiPicker list has the same value as CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT

3. We have maxHeight style for the EmojiPicker list below
   

   App/src/styles/StyleUtils.js

   Line 1191 in ed2c287

   maxHeight: windowHeight - dimensions,

This also affect the height of the EmojiPicker list

These are the root causes of this issue

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

We need to know the current height of the EmojiPicker list. In order to do this,

1. Set NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT to 256

    NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 256,

This ensures the height of the EmojiPicker list is same as this value when maxHeight is larger than this.

2. Update the below line to store the correct height to member variable
   

   App/src/components/EmojiPicker/EmojiPickerMenu/index.js

   Lines 528 to 529 in ed2c287

   	const height = !listStyle.maxHeight || listStyle.height < listStyle.maxHeight ? listStyle.height : listStyle.maxHeight;
   	const overflowLimit = Math.floor(height / CONST.EMOJI_PICKER_ITEM_HEIGHT) * 8;

        this.emojiListHeight = !listStyle.maxHeight || listStyle.height < listStyle.maxHeight ? listStyle.height : listStyle.maxHeight;
        const overflowLimit = Math.floor(this.emojiListHeight / CONST.EMOJI_PICKER_ITEM_HEIGHT) * 8;

3. Use the stored height value when scrolling
   

   App/src/components/EmojiPicker/EmojiPickerMenu/index.js

   Lines 399 to 400 in ed2c287

   	if (offsetAtEmojiBottom - this.currentScrollOffset >= CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT) {
   	targetOffset = offsetAtEmojiBottom - CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT;

        if (offsetAtEmojiBottom - this.currentScrollOffset >= this.emojiListHeight) {
            targetOffset = offsetAtEmojiBottom - this.emojiListHeight;
        }

This works perfect for all cases

Result

24471.mp4

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018ba7fbfbb4ccc435

[melvin-bot]

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

[melvin-bot]

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

[kevinksullivan]

Was OOO last week, pushing forward

[Santhosh-Sellavel]

Thanks, @DrLoopFall, this seems like a regression to me. Any Idea where this got broken?

AFAIK It was working well few months or weeks ago

[DrLoopFall]

Hi @Santhosh-Sellavel,
Looks like it's caused by #18221 (f734e1c)

Also, I think we need to remove CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT, so it sizes automatically with the emoji popup because now the emoji picker is resizable, so the emoji picker list height is not constant.

[redpanda-bit]

a) Looks like the emoji picker scrolls on its own without needing the scrollToHighlightedIndex implementation. Maybe removing it fixes this issue.
b) I saw a significant improvement from using a throttle or debounce on the scrollToHighlightedIndex. Might be a race condition.

[TTaro]

Proposal

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

When navigating through the list using the arrow keys and holding down the key, the selected emoji disappears.

What is the root cause of that problem?

If the arrow key is held down, multiple keyboard events are generated, leading to race issues when calculating the highlighted emoji.

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

1 Add a new property in the constructor
IsKeyDownInProgress = false

2 In keyDownHandler we can avoid any calculation if IsKeyDownInProgress is true and if keyBoardEvent="ArrowDown" If not we continue and set IsKeyDownInProgress = true

3 As last step of keyDownHandler we change IsKeyDownInProgress = false , so next event can progress

[melvin-bot]

@kevinksullivan, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[kevinksullivan]

Next step is for @Santhosh-Sellavel to review the comment here #24471 (comment)

[melvin-bot]

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

[kevinksullivan]

Hi @Santhosh-Sellavel can you provide an ETA on when you'll be able to review this?. If it's not till next week I'm going to have to reassign.

[melvin-bot]

@kevinksullivan @Santhosh-Sellavel 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!

[melvin-bot]

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

[ArekChr]

@s-alves10 I've been trying to find the simplest way to fix the issue. I think @DrLoopFall was the first to understand and solve the root problem. Regarding your proposal, it works, and that's appreciated. However, I'm in favour of removing the scrolling logic by replacing scrollToHighlightedIndex with this.ref.scrollIntoView({block: 'nearest'}). It seems less like a workaround and more like a fix to the problem which will be less susceptible for regressions.

[melvin-bot]

📣 @DrLoopFall 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[melvin-bot]

@kevinksullivan @ArekChr @jasperhuangg @DrLoopFall this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[DrLoopFall]

@ArekChr @jasperhuangg
The PR #26633 is ready for review

[melvin-bot]

🎯 ⚡️ Woah @ArekChr / @DrLoopFall, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @DrLoopFall got assigned: 2023-09-01 20:42:02 Z
• when the PR got merged: 2023-09-05 18:01:51 UTC

On to the next one 🚀

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

• Fix scrolling emoji picker with arrows #26633

If no regressions arise, payment will be issued on 2023-09-13. 🎊

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

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

• @ArekChr does not require payment (Contractor)
• @DrLoopFall requires payment offer (Contributor)

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:

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

[kevinksullivan]

Hey @ArekChr please get to the checklist above when you have a chance!

[kevinksullivan]

@DrLoopFall are you Afsar P on upwork? If yes, please accept the offer! And if not, pelase submit an application to the job

https://www.upwork.com/jobs/~018ba7fbfbb4ccc435

[DrLoopFall]

@kevinksullivan Yes, I've accepted the offer.

[ArekChr]

• Link to the PR: Mobile emoji picker now has search input #18221
• Link to comment: Mobile emoji picker now has search input #18221 (comment)
• Link to discussion: n/a
• Determine if we should create a regression test for this bug: Yes

Regression Test Proposal

1. Open https://staging.new.expensify.com/
2. Open emoji selector in any ocnversation
3. Press the down arrow to scroll downward
4. Verify if the focus doesn't lose after passing through the emojis categories selection header

Do we agree 👍 or 👎

[kevinksullivan]

All set with payments, closing out