[HOLD for #16078][$1000] Pressing down arrow key for the 3rd time does not highlight the 1st one of the 2 emoji suggestions and console error clicking Enter 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. Type in :smiley
2. two suggestions will pop up, the first one will be highlighted
3. press down arrow key, the second suggestion will be correctly highlighted
4. press down arrow key

Expected Result:

Should highlight 1st one and hit enter will show the emoji in the compose box

Actual Result:

Now the highlight will be invisible, and pressing enter will not do anything throws an error into the console
pressing the down arrow key three more times will bring the highlight back to the first item (basically its behaving like if there were 5 suggestions in total

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:

Recording.140.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011af615711c2a4640
• Upwork Job ID: 1638683762211266560
• Last Price Increase: 2023-03-22

[MelvinBot]

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

[kadiealexander]

I could reproduce, however I didn't get any console errors and I could make the cursor go to the first option using 5 down button presses.

2023-03-23_12-25-00.mp4

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~011af615711c2a4640

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[allroundexperts]

Proposal

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

The down arrow key does not correctly focus the suggested emojis.

What is the root cause of that problem?

The root cause of the issue is this line. We're getting an incorrect emojiRowCount value because of using Math.max function for the actual number of suggestions and the default number to show (5 for large screen). In this particular case, the actual number of emoji count is 2. However, Math.max results in the count to be 5.

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

We can just replace this expression to use Math.min instead.

A more detailed and readable way of doing the same thing would be to replace this with:

let emojiRowCount = 0;

    if (isEmojiPickerLarge) {
        emojiRowCount = numRows <= CONST.EMOJI_SUGGESTER.MAX_AMOUNT_OF_ITEMS ? numRows : CONST.EMOJI_SUGGESTER.MAX_AMOUNT_OF_ITEMS;
    } else {
        emojiRowCount = numRows <= CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS ? numRows : CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS;
    }

What alternative solutions did you explore? (Optional)

None

[Prince-Mendiratta]

Proposal

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

When there are less than 3 items suggested, there seem to be ghost values for the emoji suggestions that are not highlighted and throw console error when Enter key is clicked.

What is the root cause of that problem?

In the getMaxArrowIndex, we set the value to necessarily be a maximum of 5 for large screens. Typing :smiley results in only 2 suggestions being there, but since the maximum has to be 5, it changes to 5, resulting in empty values.

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

Lines 126 to 128 in fa31301

	const emojiRowCount = isEmojiPickerLarge
	? Math.max(numRows, CONST.EMOJI_SUGGESTER.MAX_AMOUNT_OF_ITEMS)
	: Math.max(numRows, CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS);

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

By default the maximum amount of emojis that can be suggested is 5, as can be seen here:

App/src/libs/EmojiUtils.js

Line 223 in fa31301

function suggestEmojis(text, limit = 5) {

This is called here:

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

Line 397 in fa31301

const newSuggestedEmojis = EmojiUtils.suggestEmojis(leftString);

So that is implied and we only need to modify the condition to show a maximum of 3 emojis for smaller screens. A cleaner way to do this would be via this:

    let emojiRowCount = numRows;
    if (numRows > CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS) {
        emojiRowCount = !isEmojiPickerLarge && CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS;
    }

[fedirjh]

Proposal

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

The Up/Down arrow keys does not correctly focus the suggested emojis when suggestion list have fewer that 5 emojis

What is the root cause of that problem?

ArrowKeyFocusManager's maxIndex prop doesn’t reflect the actual size of emoji suggestion list.

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

We should pass actual size of emoji suggestion to ArrowKeyFocusManager here

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

Line 883 in eb6931d

maxIndex={getMaxArrowIndex(this.state.suggestedEmojis.length, this.state.isEmojiPickerLarge)}

         maxIndex={this.state.suggestedEmojis.length - 1}

[kaushiktd]

Please re-state the problem that we are trying to solve in this issue.
Pressing down the arrow key for the 3rd time does not highlight the 1st one of the 2 emoji suggestions and the console error clicking Enter key

What is the root cause of that problem?
There is an issue with getMaxArrowIndex function in below mentioned file:

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

Lines 126 to 128 in fa31301

	const emojiRowCount = isEmojiPickerLarge
	? Math.max(numRows, CONST.EMOJI_SUGGESTER.MAX_AMOUNT_OF_ITEMS)
	: Math.max(numRows, CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS);

In this function Math.max() function is used and this method returns the number with the highest value.
In this function you are using CONST.EMOJI_SUGGESTER.MAX_AMOUNT_OF_ITEMS that has a static value of 5, so if suggested emojis are 2 or 3 it will always return 5.

What changes do you think we should make in order to solve the problem?
You need to return the exact number of suggested values so please change getMaxArrowIndex function with the below line:

const emojiRowCount = numRows - 1; (-1 because array index starts from 0 always);

[Pujan92]

Proposal

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

Up/Down arrow key goes to 5 rows every time even though available suggestions are fewer which causes console error.

What is the root cause of that problem?

We are not setting the correct maxIndex here for ArrowKeyFocusManager.

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

Line 878 in 63b96d3

maxIndex={getMaxArrowIndex(this.state.suggestedEmojis.length, this.state.isEmojiPickerLarge)}

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

We can get rid of the getMaxArrowIndex method as it seems not required and use array slicing for smaller screens at the time we received newSuggestedEmojis. To achieve it we can do below changes

1. Slice the results to MIN_AMOUNT_OF_ITEMS for smaller screens here otherwise do not
   

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

   Line 400 in 63b96d3

   nextState.suggestedEmojis = newSuggestedEmojis;

nextState.suggestedEmojis = this.state.isEmojiPickerLarge
         ? newSuggestedEmojis
          : newSuggestedEmojis.slice(0, CONST.EMOJI_SUGGESTER.MIN_AMOUNT_OF_ITEMS);

2. Use suggestedEmojis.length for the maxIndex here
   

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

   Line 878 in 63b96d3

   maxIndex={getMaxArrowIndex(this.state.suggestedEmojis.length, this.state.isEmojiPickerLarge)}

   
   maxIndex={this.state.suggestedEmojis.length - 1}

3. Remove getMaxArrowIndex method

Conclusion

• For isEmojiPickerLarge it will assign newSuggestedEmojis as it is(which won't be more than 5 for sure)
• For not isEmojiPickerLarge it will slice the newSuggestedEmojis with max to MIN_AMOUNT_OF_ITEMS

[mollfpr]

Thank you guys for the proposal! Everyone has all the correct RCA, and I like proposal from @allroundexperts because it has the correct logic and straightforward solution. (Only change the Math.max to Math.min, we can keep the ternary)

🎀 👀 🎀 C+ reviewed!

cc @nkuoch

[Pujan92]

@mollfpr isn't this makes more sense to take out the extra code and simplify it?
Just tagging @perunt as the function(getMaxArrowIndex) added in their PR to know whether it is required or not(as per my proposal)

[mollfpr]

@Pujan92 You have a point. Currently, the behavior on the smaller device is showing 5 emojis. With your proposal, we are only showing 3 emojis. We can wait for @perunt to clarify this.

Screen.Recording.2023-03-24.at.19.26.24.mov

---

@allroundexperts I do not recommend creating a PR before you are assigned to the issue. Let's wait @nkuoch first.

[allroundexperts]

@Pujan92 You have a point. Currently, the behavior on the smaller device is showing 5 emojis. With your proposal, we are only showing 3 emojis. We can wait for @perunt to clarify this.

@allroundexperts I do not recommend creating a PR before you are assigned to the issue. Let's wait @nkuoch first.

@mollfpr I created the PR on my fork. I'm not sure why it shows up here.

[mollfpr]

@allroundexperts Ohyea, sorry I didn't realize that. Probably because you mention the issue.

[perunt]

Hey guys! I'm working on implementing inline suggestions so that behavior will be changed, so it probably not make sense to make any changes here

[mollfpr]

Thanks @perunt, for clarifying this!

@nkuoch @kadiealexander Let's hold this and come back after @perunt implementation is done.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[kadiealexander]

@grgia and @mollfpr could you please assess the PR here #16708 and see if we're able to proceed here?

[grgia]

@kadiealexander the issue we're holding on is #16078 (still open) I think the title was a typo. I'll update it.

[kadiealexander]

🤦 Thanks @grgia!!

[kadiealexander]

Still on hold.

[kadiealexander]

Same as above.

[kadiealexander]

Still on hold.

[kadiealexander]

The issue this was on hold for has made it to production! @puneetlath or @kavimuru would you mind re-testing this and ensuring it's not still a bug? I am no longer able to reproduce it.

[puneetlath]

I'm also no longer able to reproduce.

[kadiealexander]

Woohoo! Closing this one :)