[HOLD for payment 2023-06-29] Web - Split Bill - Money request confirmation list stays highlighted after press

[kbecciv]

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 a group chat
2. Start a Split Bill and proceed until the confirmation page
3. Unchecks the items in the list

Expected Result:

The item is not highlighted if we don't hover it or is not focused

Actual Result:

The item stays highlighted after press and also the first item that is not clickable is highlighted if we click it

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.3.26.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

Screen.Recording.2023-06-08.at.13.24.35.mov

Recording.3078.mp4

Expensify/Expensify Issue URL:

Issue reported by: @bernhardoj

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686202285251059

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0110ed1d7d5b0b56cb
• Upwork Job ID: 1670152822418042880
• Last Price Increase: 2023-06-17

[melvin-bot]

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

[Christinadobrzyn]

I can reproduce but I don't see why this is a bug - asking for more information https://expensify.slack.com/archives/C049HHMV9SM/p1686604097827569?thread_ts=1686202285.251059&cid=C049HHMV9SM

[dukenv0307]

Proposal

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

Money request confirmation list stays highlighted after press

What is the root cause of that problem?

We add focusStyle as this.props.hoverStyle in OptionRow that is optionHoveredStyle props we passed to OptionsSelector. When we select a row, it's focused, and focusStyle is applied.

App/src/components/OptionRow.js

Line 185 in 93957f0

focusStyle={this.props.hoverStyle}

In money request confirmation list we pass optionHoveredStyle to OptionsSelector, so that when a row is selected it stays highlighted with style backgroundColor: themeColors.hoverComponentBG, focusIndex change to the next option in OptionsSelector and then we can see both options are highlighted with different bg color.

App/src/components/MoneyRequestConfirmationList.js

Line 302 in 93957f0

optionHoveredStyle={canModifyParticipants ? styles.hoveredComponentBG : {}}

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

I saw that optionHoveredStyle has a default style as optionHoveredStyle: styles.hoveredComponentBG. So I think we should define a new prop in optionsSelectorPropTypes like optionFocusedStyle to prevent focusStyle in some select pages in which we can select multiple and this issue doesn't occur in these pages.

What alternative solutions did you explore? (Optional)

[bernhardoj]

It's a regression from #19612 because of the focusStyle to solve this issue. The linked issue does not happen in the previous implementation because the hovered style is applied directly to the Pressable instead of the children. @Skalakid may know what's the best to do here.

[Christinadobrzyn]

Reached out here for some help - #19612 (comment)

[Skalakid]

@bernhardoj @Christinadobrzyn I reverted my changes for testing and still, I got the same bug (even in PRs merged before #19612). However, I think I know where is the problem. It is caused because of not disabling focusing in this list. I will write the proposal

[bernhardoj]

Hmm, that's strange. I removed focusStyle and the issue is gone. To have us in the same page, the first row should not be highlighted (ss taken from video above).

[Skalakid]

Proposal

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

The money request confirmation list stays highlighted after press

What is the root cause of that problem?

There are two root causes of this problem. The first one is that we don't disable focusing on this list. The second one is that we have two places that specify focus styles for PressableWithFeedback

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

We should pass the disableFocusOptions prop to the <OptionsList> component. This is the parent component of the user list where the bug exists. By passing this option the optionIsFocused prop in OptionRow will be always false.
After we change this, we should remove focus styles from the style prop in PressableWithFeedback because we can just use focusStyle prop.
Next, we should make focusStyle disabled when optionIsFocused is false, so we can change it to this.props.optionIsFocused ? styles.sidebarLinkActive : null}.

After we do these changes, the issue should be fixed, however we will have to fix again this issue. To do this we should change a little bit PressableWithFeedback structure and move the styles from OpacityView to the parent component GenericPressable. These changes will resolve both issues

What alternative solutions did you explore? (Optional)

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.

[Skalakid]

Hmm, that's strange. I removed focusStyle and the issue is gone. To have us in the same page, the first row should not be highlighted (ss taken from video above).

Hmm maybe I misunderstand something, I thought that after we select/unselect something nothing should be highlighted. Something like this:

Nagranie.z.ekranu.2023-06-14.o.17.24.43.mov

So the last clicked element should be on focus? Because on the ss that you sent, I see two highlights

[bernhardoj]

Sorry if my previous comment is a bit confusing. The ss I sent show 2 highlighted rows.

The bottom one is the correct one. It highlights the row that can be selected by pressing enter in keyboard. I think in all cases, it will highlights the first unselected row.

The top one should not be highlighted. It is highlighted because of the focusStyle. Pressing anywhere will remove the highlight.

Screen.Recording.2023-06-14.at.23.22.58.mov

[Skalakid]

Oh okay, now I think I understand, we want to focus last unselected element to enable the user to select it by clicking enter but the other highlight is wrong. Hmm, I tested it and its really buggy so I thought that we don't want this 😅

Nagranie.z.ekranu.2023-06-14.o.17.43.41.mov

[bernhardoj]

I guess that's another bug to be solved haha. I think we should focus on the wrong highlight here.

[Skalakid]

Okay, so to fix this issue we have to move styles in PressableWithFeedback to the parent component, to not break #19612 (comment). Also, we need to remove focusStyles prop in OptionRow because focused element is different than highlighted element (the last unselected item). I can create PR with the fix for it :D

[Skalakid]

The bug with switching between items using arrows and enter button occurs after you change the focused item using the tab button. The focusedIndex from ArrowKeyFocusManager is not changing so we have two different items "focused". But I think it should be resolved in another issue

[techievivek]

Looks like @Skalakid is working on this issue. The assigned engineer on the PR is @Julesssss, and the assigned C+ is @s77rt, so I will fix the assignments here to keep everything in sync.

[Julesssss]

We need to make a small follow up change, so we'll hold the payment on that temporarily.

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

• fix: 20585 - Web - Split Bill - Money request confirmation list stays highlighted after press #20838

If no regressions arise, payment will be issued on 2023-06-29. 🎊

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

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

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

[s77rt]

• The PR that introduced the bug has been identified: 17013 - migrate OptionRow to PressableWithFeedback #19612
• The offending PR has been commented on: 17013 - migrate OptionRow to PressableWithFeedback #19612 (comment)
• A discussion in #expensify-bugs has been started: I doubt we have much to discuss, this could have been caught during testing
• Determine if we should create a regression test for this bug: No

[Christinadobrzyn]

Looks like no regressions - paying this out.

This is a regression, right so I would only pay the reporter? If that's not the case, will you please let me know if there's a speed bonus for this?

@Julesssss @s77rt

[s77rt]

This is a regression, right so I would only pay the reporter? If that's not the case, will you please let me know if there's a speed bonus for this?

Not a regression that was caught during regression period (i.e. not a regression from something that we worked on recently) so I think we should pay everyone here. No bonus though.

[Christinadobrzyn]

Thanks @s77rt!

Here's the Upwork job posting:

Internal: https://www.upwork.com/ab/applicants/1670152822418042880/job-details
External: https://www.upwork.com/jobs/~0110ed1d7d5b0b56cb

@s77rt as C+
@bernhardoj as reporter

[Julesssss]

Not overdue

[Christinadobrzyn]

Paid out based on #20585 (comment)

Closing