Do not select participant in IOU confirmation list

[luacmartins]

Details

Prevents the focusedIndex being set to 0 when the IOUConfimationList component updates

Fixed Issues

$ #14441

Tests

1. Open the App in 2 sessions logged in to the same account
2. In session A, navigate to + > Request money/Send money > Enter Amount > Select participant > Confirmation page
3. In session B, navigate to Settings > Preferences and change the selected language
4. In session A, verify that the selected participant is not highlighted

• Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests.

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

chrome.mov

Mobile Web - Safari

safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

[melvin-bot]

@parasharrajat @aldo-expensify One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[parasharrajat]

Will be reviewing shortly.

[parasharrajat]

It is taking time as I am trying to understand the focused index logic of OptionsList.

[luacmartins]

@parasharrajat if it helps, the only place that passes the shouldTextInputAppearBelowOptions prop is the IOUConfirmationList page, so this shouldn't affect any other pages.

[parasharrajat]

That is correct. What is the difference between newOptions and selectedOptions?

[luacmartins]

newOptions shows all available options in the page, whereas selectedOptions only shows the option we "selected" in cases where we can select multiple participants like in Group chat or Split bill.

So in the context of the IOUConfirmPage it will be the same for SplitBill since we can select multiple participants, but for other flows e.g. Request money selected participants will be an empty array since we can't "select" multiple participants.

You can see them in the images below:

Split bill

Request money

[aldo-expensify]

@luacmartins the change works and seems somewhat consistent with what we do with the index in this other line:

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 44 in 066018a

focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,

I don't fully understand why this code makes sense, can you add a comment explaining?

Did you consider another option like "disable highlights if the options are disabled"? In this case, you can't click on the options, they are just there for display, prop isDisabled is set to true. I think it could make sense and be more intuitive just to disable the highlights:

                optionIsFocused={!this.props.disableFocusOptions
                    && !this.props.isDisabled
                    && !section.isDisabled
                    && this.props.focusedIndex === (index + section.indexOffset)}

in

App/src/components/OptionsList/BaseOptionsList.js

Lines 158 to 170 in 066018a

	<OptionRow
	option={item}
	showTitleTooltip={this.props.showTitleTooltip}
	hoverStyle={this.props.optionHoveredStyle}
	optionIsFocused={!this.props.disableFocusOptions
	&& this.props.focusedIndex === (index + section.indexOffset)}
	onSelectRow={this.props.onSelectRow}
	isSelected={Boolean(_.find(this.props.selectedOptions, option => option.login === item.login))}
	showSelectedState={this.props.canSelectMultipleOptions}
	boldStyle={this.props.boldStyle}
	isDisabled={this.props.isDisabled || section.isDisabled}
	shouldHaveOptionSeparator={index > 0 && this.props.shouldHaveOptionSeparator}
	/>

Or another options is to pass disableFocusOptions as true for this usage.

[luacmartins]

Did you consider another option like "disable highlights if the options are disabled"?

Funny you asked, because that was the first solution that I tried and it works! However, I felt like it was kind of a workaround to me since what caused the issue was focusing on that option in the first place instead of having the input focused by default. I don't feel strongly though, so I can use the disable the highlights if the option is disabled approach if we think that's more intuitive.

[luacmartins]

Updated with the "disable highlight if option is disabled" solution!

[aldo-expensify]

Did you consider another option like "disable highlights if the options are disabled"?

Funny you asked, because that was the first solution that I tried and it works! However, I felt like it was kind of a workaround to me since what caused the issue was focusing on that option in the first place instead of having the input focused by default. I don't feel strongly though, so I can use the disable the highlights if the option is disabled approach if we think that's more intuitive.

Was the initial solution to change the focus index to 1, since that is the index of the input if it was in the option list? Was the focus index = 0 making it highlight incorrectly?

if the answer is yes to both, I feel like moving the index to 1 could also be seen as a workaround. It doesn't make a lot of sense to me to talk about a valid index for the input if the input doesn't really belong to the option list.

Having said that, the stuff about disabling the highlight for disabled option lists also don't convince me 100%, but I feel it is a bit easier to understand to me... unless you added a comment to the index calculation explaining that ternary.

PD: I'm just divided about what is the best, I don't mean to criticize the approach/current solution or past solution. Just trying to communicate my thoughts in case they are of some use.

[luacmartins]

Was the initial solution to change the focus index to 1, since that is the index of the input if it was in the option list? Was the focus index = 0 making it highlight incorrectly?

Yea, this was it. I agree with what you said above and I don't feel strongly about any particular solution and I also agree that disabling the focus highlight is more intuitive so let's go with that! As always, thanks for the review!

[luacmartins]

Bump @parasharrajat!

[parasharrajat]

I will test it sometime. I really don't remember the focusedIndex main design but I will try to test it best of my knowledge.

[luacmartins]

@parasharrajat we changed the solution and are now just disabling focus if the input is disabled.

[aldo-expensify]

Ping @parasharrajat

[parasharrajat]

Sorry for the delay, Checking now.

[parasharrajat]

I completely agree that this does not make sense. It needs a comment.

focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0, 

Are we setting to focusedIndex to nothing as there won't be any item in the list with index === allOptions.length when text input is below the list, otherwise to the first item in the list? Another question is why?

I am trying to understand the root cause of this issue which is still unclear to me. I am fine with disabling the highlight when the option is disabled which fixes this issue. But the real problem is why changing the language(change detection cycle) caused the focus. I think we should also fix that it might come back for other lists where options are not disabled and the selection list is empty.

Can someone help me understand that?

[luacmartins]

Here's what I think that does:

1. We can pass the shouldTextInputAppearBelowOptions to render a TextInput component below the options list.
2. If we pass that prop, we set the focusedIndex to allOptions.length so that the TextInput is focused, instead of the first option in the OptionsList

We only seem to have that logic as a default for the state though. When we change languages and the component re-renders, we hit this logic instead, which does not account for the shouldTextInputAppearBelowOptions prop and sets the focusedIndex to 0

[parasharrajat]

If we pass that prop, we set the focusedIndex to allOptions.length so that the TextInput is focused, instead of the first option in the OptionsList

How is this working? I mean where are we focusing the TextInput?

[luacmartins]

We do it here, because we also pass autoFocus along with shouldTextInputAppearBelowOptions.

[parasharrajat]

It means there is no role of focusedIndex in setting the focus to TextInput.

[luacmartins]

Correct! It just prevents the option from being focused, since we focus the TextInput instead.

[parasharrajat]

Screenshots

🔲 iOS / native

screen-2023-02-01_06.53.21.mp4

🔲 iOS / Safari

screen-2023-02-01_06.58.50.mp4

🔲 MacOS / Desktop

screen-2023-02-01_06.51.02.mp4

🔲 MacOS / Chrome

screen-2023-02-01_06.33.13.mp4

🔲 Android / Chrome

screen-2023-02-01_06.34.32.mp4

🔲 Android / native

screen-2023-02-01_06.38.58.mp4

[parasharrajat]

LGTM. Fixes the issue.

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @aldo-expensify

🎀 👀 🎀 C+ reviewed

[parasharrajat]

@luacmartins So do you think we should also fix the root cause as explained in #14502 (comment)?

This means also adding logic to fix the focusedIndex in componentDidUpdate.

[luacmartins]

Haha I think that's what my first solution tried to address, but TBH I think we should get rid of the shouldTextInputAppearBelowOptions altogether since it's confusing and just disable focusing on the OptionsList. I'll leave that for another PR though. Thanks for the reviews!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	708.735 ms → 743.545 ms (+34.810 ms, +4.9%)
Open Search Page TTI	609.265 ms → 621.037 ms (+11.772 ms, +1.9%)
App start runJsBundle	196.250 ms → 204.839 ms (+8.589 ms, +4.4%)
App start nativeLaunch	20.000 ms → 20.100 ms (+0.100 ms, +0.5%)
App start regularAppStart	0.015 ms → 0.016 ms (+0.001 ms, +7.6%)

Show details

Name	Duration
App start TTI	Baseline Mean: 708.735 ms Stdev: 41.056 ms (5.8%) Runs: 646.5257450002246 664.1360120000318 667.2672460000031 667.8922069999389 669.8495849999599 671.4976820000447 673.7066239998676 677.0778600000776 678.7988829999231 679.6882810001262 681.5663029998541 683.20119300019 685.3997300001793 685.8845870001242 688.8142710002139 696.4727509999648 698.3629600000568 706.1737330001779 709.722145000007 710.7167710000649 712.3739339997992 715.6733050001785 717.668169000186 728.8869869997725 737.751747999806 756.1781930001453 757.6879170001484 757.9631579997949 769.34283100022 778.0188429998234 794.0757059999742 811.1287609999999 Current Mean: 743.545 ms Stdev: 31.506 ms (4.2%) Runs: 689.2129219998606 699.4444960001856 703.8465800001286 707.9036090001464 712.3796040001325 715.2909039999358 716.6586850001477 722.1857300000265 723.6457779998891 726.2179200001992 727.528460000176 730.4623619997874 733.507563999854 736.8922419999726 738.0949699999765 738.1129800002091 740.6191230001859 742.8690109997988 743.5828350000083 745.0823659999296 746.1566090001725 746.7215289999731 748.0807139999233 750.3257129997946 758.2904210002162 770.0325400000438 770.9315809998661 781.6039249999449 788.7260070000775 811.3653350002132 811.5533110001124 816.1106639998034
Open Search Page TTI	Baseline Mean: 609.265 ms Stdev: 13.108 ms (2.2%) Runs: 581.8817950002849 589.4807130000554 590.3723550001159 592.3412279998884 593.6854249997996 597.7209070003591 600.5882569998503 600.6571450000629 602.3336180001497 603.2713629999198 603.547730000224 606.3771979999729 606.406697999686 607.4733890001662 608.1654059998691 609.428588999901 609.6432289998047 610.141601999756 611.2817800003104 612.9603269998915 614.1217040000483 614.965739000123 615.1761480001733 615.7724609998986 618.097087000031 619.35990400007 621.2232260000892 624.6249600001611 627.1110439999029 636.2655850001611 642.7421470000409 Current Mean: 621.037 ms Stdev: 15.685 ms (2.5%) Runs: 590.6986899999902 598.2480469997972 601.8592130001634 602.0325119998306 602.2681479998864 603.8648680001497 605.7668050001375 607.703125 610.275391000323 610.7670489996672 612.7775880000554 613.7687180000357 615.4217530000024 615.8911549998447 617.4925539996475 619.3140460001305 620.6774089997634 621.015340000391 622.9148769997992 624.3849280001596 627.3585609998554 627.4581710002385 627.4667569999583 630.3597409999929 632.3275150000118 632.9331879997626 635.0531819998287 636.1955160000362 637.943889000453 638.5548910000362 640.250976999756 645.3905850001611 665.7792969997972
App start runJsBundle	Baseline Mean: 196.250 ms Stdev: 21.310 ms (10.9%) Runs: 166 169 173 174 175 176 178 178 178 181 187 188 189 190 190 192 192 192 192 194 200 200 206 208 211 216 216 220 223 223 248 255 Current Mean: 204.839 ms Stdev: 14.826 ms (7.2%) Runs: 175 184 185 185 187 189 193 193 195 195 196 200 201 203 204 205 206 207 210 211 212 212 213 213 216 219 222 225 225 230 239
App start nativeLaunch	Baseline Mean: 20.000 ms Stdev: 2.550 ms (12.7%) Runs: 17 17 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 21 22 22 22 22 23 23 24 25 28 Current Mean: 20.100 ms Stdev: 2.256 ms (11.2%) Runs: 17 17 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 21 21 21 22 22 22 24 24 25 26
App start regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (6.2%) Runs: 0.012817000038921833 0.013264999724924564 0.0139979999512434 0.0139979999512434 0.014037999790161848 0.014160000253468752 0.014282000251114368 0.014322999864816666 0.01436399994418025 0.014405000023543835 0.014566999860107899 0.014566999860107899 0.01460800040513277 0.0147299999371171 0.014851999934762716 0.014851999934762716 0.01509599993005395 0.015178000088781118 0.015339999925345182 0.015583999920636415 0.015584999695420265 0.015666000079363585 0.015705999918282032 0.01570700015872717 0.015910000074654818 0.015910000074654818 0.016030999831855297 0.016032000072300434 0.01615400006994605 0.016561000142246485 0.016764999832957983 Current Mean: 0.016 ms Stdev: 0.001 ms (4.7%) Runs: 0.014282000251114368 0.015137000009417534 0.015176999848335981 0.015217999927699566 0.01525900000706315 0.015340999700129032 0.015583999920636415 0.015585000161081553 0.015585000161081553 0.015625 0.015787999611347914 0.015868999995291233 0.01599099999293685 0.01599099999293685 0.01599099999293685 0.016032000072300434 0.01627599960193038 0.01643799990415573 0.016479999758303165 0.016561000142246485 0.016600999981164932 0.016642000060528517 0.01672299997881055 0.0167239997535944 0.016805000137537718 0.016846000216901302 0.016846000216901302 0.0170889999717474 0.01725299982354045 0.01725299982354045 0.017578000202775

[OSBotify]

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.2.64-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅