[HOLD for payment 2023-07-14] [$1000] Placeholder not moving back to it's place after selecting a contact

[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. Click on FAB
2. Click on "Split bill"
3. Type an email and select the contact that shows up in the below suggestion
4. Notice the placeholder text "Name, email or phone number"

Expected Result:

The placeholder text should return back to it's place after user selects on a contact

Actual Result:

Placeholder stays in typing state and doesn't return back

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.16.3
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: Any additional supporting documentation

2023-05-16.21.25.47.mp4

Recording.650.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684261779675679

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0196a89d054bb9c9e0
• Upwork Job ID: 1660771960364109824
• Last Price Increase: 2023-06-12

[melvin-bot]

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

[PrashantMangukiya]

Proposal

Posting proposal early as per new guidelines

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

Placeholder not moving back to it's place after selecting a contact. i.e. Placeholder not focus after selecting contact.

What is the root cause of that problem?

During Split Bill it it rendering participant selection list via this

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js

Lines 218 to 233 in 78ec531

	<OptionsSelector
	canSelectMultipleOptions
	sections={sections}
	selectedOptions={this.props.participants}
	value={this.state.searchTerm}
	onSelectRow={this.toggleOption}
	onChangeText={this.updateOptionsWithSearchTerm}
	headerMessage={headerMessage}
	boldStyle
	shouldShowConfirmButton
	confirmButtonText={this.props.translate('common.next')}
	onConfirmSelection={this.finalizeParticipants}
	textInputLabel={this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
	safeAreaPaddingBottomStyle={this.props.safeAreaPaddingBottomStyle}
	shouldShowOptions={isOptionsDataReady}
	/>

OptionsSelector has shouldFocusOnSelectRow prop is there with default value false. It can be used to set focus to text input after an option is selected.

App/src/components/OptionsSelector/optionsSelectorPropTypes.js

Lines 71 to 72 in 78ec531

	/** Whether to focus the textinput after an option is selected */
	shouldFocusOnSelectRow: PropTypes.bool,

Within MoneyRequestParticipantsSplitSelector.js we are not passing shouldFocusOnSelectRow props to OptionSelector at line 218, so it will not set focus to text input after option is selected. This is the root cause of the problem.

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

Within MoneyRequestParticipantsSplitSelector.js we have to pass shouldFocusOnSelectRow to OptionSelector (starts from line 218) as shown code below. So it will focus to text input after option is selected.

 <OptionsSelector 
     ....
     shouldFocusOnSelectRow        // *** Add this
 /> 

This will solve the issue as shown in result video.

What alternative solutions did you explore? (Optional)

None

Result

SplitBill-Contact-select.mp4

[strepanier03]

I am able to recreate this behavior, but something odd also happened.

When testing, I recreated the behavior right away. But then I stepped away and left my screen open on the contact select screen where this issue was happening.

When I came back 5-10 minutes later, the behavior didn't happen any longer. The placeholder went back to it's place every time and very quickly. Now, I can't recreate the issue again.

I'm going to pause until tomorrow and then move this forward then if I can recreate.

[strepanier03]

Tested this again this morning and I the behavior hasn't returned.

@kavimuru, can you still recreate this behavior? I can no longer recreate it. How about you @Nathan-Mulugeta??

[Nathan-Mulugeta]

Yes @strepanier03, I am still able to reproduce this. Here are some things to notice. After clicking split bill from the FAB menu instead of selecting the contact lists from the recent list, type out a new email to add to the split bill money request. After typing the email in the input field select the email that shows up. Then you can notice the place holder doesn't return back to its place. I will attach a video demonestrating this.

2023-05-19.22.13.38.mp4

[bernhardoj]

Proposal

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

Text input label does not deactivate (move back as the placeholder) when we clear the input value. To reproduce it, make sure type something first.

What is the root cause of that problem?

Actually, we already have the code that SHOULD deactivate the label when we clear the input value here:

App/src/components/TextInput/BaseTextInput.js

Lines 71 to 89 in 7378626

	// Activate or deactivate the label when value is changed programmatically from outside
	const inputValue = _.isUndefined(this.props.value) ? this.input.value : this.props.value;
	if ((_.isUndefined(inputValue) || this.state.value === inputValue) && _.isEqual(prevProps.selection, this.props.selection)) {
	return;
	}
	// eslint-disable-next-line react/no-did-update-set-state
	this.setState({value: inputValue, selection: this.props.selection});
	// In some cases, When the value prop is empty, it is not properly updated on the TextInput due to its uncontrolled nature, thus manually clearing the TextInput.
	if (inputValue === '') {
	this.input.clear();
	}
	if (inputValue) {
	this.activateLabel();
	} else if (!this.state.isFocused) {
	this.deactivateLabel();
	}

As you can see, we update the value state with inputValue and call deactivateLabel if inputValue is empty. Then, inside deactivateLabel, we check if the state of value is empty or not.

App/src/components/TextInput/BaseTextInput.js

Lines 164 to 171 in 7378626

	deactivateLabel() {
	if (this.props.forceActiveLabel || this.state.value.length !== 0 || this.props.prefixCharacter) {
	return;
	}
	this.animateLabel(styleConst.INACTIVE_LABEL_TRANSLATE_Y, styleConst.INACTIVE_LABEL_SCALE);
	this.isLabelActive = false;
	}

If it's empty, then we proceed to deactivate the label. The problem is, the value state is not updated yet inside deactivateLabel, which is basically a race condition.

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

Call the deactivateLabel inside the setState callback.

More specific, move this code to be inside the setState callback.

App/src/components/TextInput/BaseTextInput.js

Lines 85 to 89 in 7378626

	if (inputValue) {
	this.activateLabel();
	} else if (!this.state.isFocused) {
	this.deactivateLabel();
	}

I guess we also can change inputValue to this.state.value instead.

Result

Screen.Recording.2023-05-20.at.20.12.56.mov

[melvin-bot]

@strepanier03 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0196a89d054bb9c9e0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[strepanier03]

Thank you for the extra details @Nathan-Mulugeta! Moved this on.

[dukenv0307]

Proposal

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

Placeholder stays in typing state and doesn't return back after selecting a contact.

What is the root cause of that problem?

There're 2 problems:

1. First, the expected result might not be enough. Here's what happens when a contact is selected in Android/iOS:

RPReplay_Final1684858056.MP4

As we can see, on native platforms, when a contact is selected, the search field will remain focused. The same should happen for web as well for consistency, while currently for web the search input will be blurred. Also there's no reason we should blur the input after selecting a contact, because the user might want to search and add a new contact immediately.

Also worthy to note that the search field will remain focused is different from the search field will be blurred then focused again which is what the shouldFocusOnSelectRow is for. the search field will be blurred then focused again will make the input blink when selecting contact, also it's different UX compared to Android/iOS.

So the expectation should be "the search input should remain focused on Web after selecting a contact" (currently it's like that for Android/iOS).

The root cause is that currently we're not supporting this behavior.

2. 1. should be enough to fix this issue, but there's a small issue in here

   App/src/components/TextInput/BaseTextInput.js

   Line 88 in 1aa30d1

   this.deactivateLabel();

   which might cause problems in the future. In there the deactivateLabel is called if the inputValue becomes empty, inside deactivateLabel itself we're using the state.value here

   App/src/components/TextInput/BaseTextInput.js

   Line 165 in 1aa30d1

   if (this.props.forceActiveLabel || this.state.value.length !== 0 || this.props.prefixCharacter) {

   . It causes race condition where inputValue becomes empty but the state.value doesn't update to empty yet inside deactivateLabel, causing the label not to be deactivated and moved back to its place properly.

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

We need to listen to mouse down event of the option, and preventDefault if the TextInput is currently focused, so that it will not blur the TextInput on selecting option.

We already use the same approach to make sure the Composer is not blurred when pressing the send button here.

Steps:
a. Pass a new onRowMouseDown to <OptionsList here

onRowMouseDown={(e) => {
    if (!e || !this.textInput.isFocused()) {
        return;
    }
    
    e.preventDefault();
}}

b. Pass onRowMouseDown={this.props.onRowMouseDown} to OptionRow, then in OptionRow here

App/src/components/OptionRow.js

Line 151 in 1ac9964

<TouchableOpacity

, assign the onRowMouseDown to the onMouseDown:

onMouseDown={this.props.onRowMouseDown}

2. To fix this we should make sure the deactivateLabel call here happens inside this setState callback

What alternative solutions did you explore? (Optional)

In 1, we can optionally introduce a variable to OptionsSelector to control if we want to disable the behavior "the search input should remain focused on Web after selecting a contact" in some cases

[strepanier03]

@sobitneupane - Friendly bump for the proposal submitted yesterday. Thank you!

[sobitneupane]

I believe the behavior of OptionsSelector in split bill page should be similar to that of OptionSelector in New group page.

Screen.Recording.2023-05-25.at.16.24.17.mov

In New group page,

• Text Input refocuses on select row with input text selected.

@strepanier03 @tylerkaraszewski Do we want similar behavior in Split bill page as well?

[dukenv0307]

@sobitneupane @tylerkaraszewski just to note both Split bill page and New group page behavior after selecting the option in desktop are inconsistent with that same page in mobile. In mobile the input will not be blurred (blinked) after the option is selected. We can make all those pages consistent with each other and also platforms to be consistent as well.

[melvin-bot]

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

[strepanier03]

@dukenv0307 - Looks like Sobit left a comment on your proposal.

[dukenv0307]

I think we are eligible for the timeline bonus because this issue is on hold from Jun 29 to Jul 4

[isabelastisser]

@sobitneupane, to confirm, the PR didn't cause a regression, and this issue is ready for payment, correct?

@arosiclair I think we can process the bonus because the PR merged shortly after it was no longer on hold. Do you agree?

[sobitneupane]

@isabelastisser Yup. The PR didn't cause a regression. The PR has not hit the production yet. So, not ready to issue payment.

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

• Fix: Remain focus when selecting option #20736

If no regressions arise, payment will be issued on 2023-07-14. 🎊

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:

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

[arosiclair]

@arosiclair I think we can process the bonus because the PR merged shortly after it was no longer on hold. Do you agree?

That makes sense to me 👍

[isabelastisser]

I was OOO yesterday and will take care of this by EOD.

[isabelastisser]

@sobitneupane please complete the BugZero checklist.

I will process the payments in Upwork now.

[sobitneupane]

Sorry for the delay @isabelastisser. I will complete the checklist by tomorrow. I will be paid through newDot.

[isabelastisser]

The payments were made in Upwork. Waiting for @sobitneupane to complete the BZ checklist, then we are all set.

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

Rather than a bug, it was an inconsistency issue. The PR raised make MoneyRequestParticipantsSplitSelector page consistent with NewGroupPage.

[sobitneupane]

Regression Test Proposal

• Click on FAB
• Click on "Split bill"
• Type an email and select the contact that shows up in the below suggestion
• Verify that the search input is highlighted/selected after user selects/deselects

Do we agree 👍 or 👎

[isabelastisser]

[@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

https://github.com/Expensify/Expensify/issues/302474

all set!

[sobitneupane]

#19231 (comment)

Requested payment on newDot.

[JmillsExpensify]

@isabelastisser Can you please summarize the appropriate individual payments for all parties involved in this issue? This is holding up @sobitneupane's NewDot payments. More information on this compliance process in Slack.

[isabelastisser]

Payments summary:

Issue reported by: @Nathan-Mulugeta $250
C+ review: @sobitneupane $1000 review, $500 urgency bonus
Issue fixed by: @dukenv0307, $1000 for accepted proposal, $500 urgency bonus

[JmillsExpensify]

Reviewed details for @sobitneupane. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.