[HOLD for payment 2023-01-19] [$1000] Chat - New Group - The search box overlays the selected user

[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. Go to https://staging.new.expensify.com/
2. Click on the green + button
3. Click on the "New group" option
4. Start typing a search term
5. Select user
6. Then remove from search box

Expected Result:

When selecting users, they are displayed on the top of the list with a green check mark on the right.

Actual Result:

The user who has already been selected in the group is hiding behind the search box.

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

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5885194_Recording__210.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/~012de37194a5fa31d2
• Upwork Job ID: 1611089102897319936
• Last Price Increase: 2023-01-05

[melvin-bot]

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[NicMendonca]

Bug0 Triage Checklist

Note: see this SO for more information.

• The "bug" is actually a bug - confirmed here: https://expensify.slack.com/archives/C01SKUP7QR0/p1672939604697969
• The bug is not a duplicate report of an existing GH.
  • If it is, close the GH and add any novel details to the original GH instead
• The bug is reproducible, following the reproduction steps.
  • If the GH doesn’t have steps to reliably reproduce the bug and you figure it out, then please add them.
  • If you’re unable to reproduce the bug, add the Needs reproduction label.
  • Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.
• The GH template is filled out as fully as possible
  • The GH body and title are clear (ie. could an external contributor understand it and work on it?)
• The GH was created by an Expensify employee or a QA tester
• If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification
• If there's a link to Slack, check the discussion to see if we decided not to fix it
• Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012de37194a5fa31d2

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[syedsaroshfarrukhdot]

I believe this issue is related to auto scrolling of the option list as per the comment here we are looking to redesign the UI/UX of options list it should be resolved with we can put it on hold till then i guess.

[allroundexperts]

Proposal

The issue is being caused by scrolling to the first non selected option whenever the search term changes.

Fix

Following needs to be changed:

diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index 8e2668be2..671968e16 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -110,14 +110,7 @@ class BaseOptionsSelector extends Component {
             // If we just toggled an option on a multi-selection page, scroll to top
             if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
                 this.scrollToIndex(0);
-                return;
             }
-
-            // Otherwise, scroll to the focused index (as long as it's in range)
-            if (this.state.allOptions.length <= this.state.focusedIndex) {
-                return;
-            }
-            this.scrollToIndex(this.state.focusedIndex);
         });
     }
 

Result

Screen.Recording.2023-01-06.at.1.49.51.AM.mov

[Ollyws]

Proposal

When the textInput is cleared we scroll to the focused index, if we want to scroll to the top when we clear the textbox we can change this line:

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 110 to 114 in c2c7e70

	// If we just toggled an option on a multi-selection page, scroll to top
	if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
	this.scrollToIndex(0);
	return;
	}

- if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
+ if (this.props.selectedOptions.length !== prevProps.selectedOptions.length || this.props.value === '') {

[mollfpr]

Thank you guys for the proposal!

@NicMendonca as per comment here, do we want to hold this for the redesign?

cc @mountiny

[tienifr]

Proposal

Problem

We are scrolling to the focused index and the focused index is assigned with this.props.selectedOptions.length

Solution 1

We shouldn't scroll to the focused index and whenever this.props.selectedOptions.length is changed or this.props.sections[1].data (recent section) changed or this.props.sections[2].data (contact section) changed, we will scroll to top. Because when sections are changed (when text search is changed), the user should see new first options
In /Expensify/App/src/components/OptionsSelector/BaseOptionsSelector.js

componentDidUpdate(prevProps) {
        if (_.isEqual(this.props.sections, prevProps.sections)) {
            return;
        }

        const newOptions = this.flattenSections();
        const newFocusedIndex = this.props.selectedOptions.length;
        this.setState({
            allOptions: newOptions,
            focusedIndex: newFocusedIndex,
        }, () => {
            // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
-                this.scrollToIndex(0);
-                return;
-            }

-            Otherwise, scroll to the focused index (as long as it's in range)
-            if (this.state.allOptions.length <= this.state.focusedIndex) {
-                return;
-            }
-             this.scrollToIndex(this.state.focusedIndex);
+         if (
+                this.props.selectedOptions.length !== prevProps.selectedOptions.length
+                || _.isEqual(this.props.sections[1].data, prevProps.sections[1].data)
+                || _.isEqual(this.props.sections[2].data, prevProps.sections[2].data)) {
+                this.scrollToIndex(0);
+            }
+       });
    }

Solution 2

We shouldn't scroll to the focused index and whenever this.props.selectedOptions.length is changed or the real text search value is updated, we will scroll to top
In /Expensify/App/src/components/OptionsSelector/BaseOptionsSelector.js

componentDidUpdate(prevProps) {
        if (_.isEqual(this.props.sections, prevProps.sections)) {
            return;
        }

        const newOptions = this.flattenSections();
        const newFocusedIndex = this.props.selectedOptions.length;
        this.setState({
            allOptions: newOptions,
            focusedIndex: newFocusedIndex,
        }, () => {
            // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
+            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length || this.props.value !== prevProps.value) {
                this.scrollToIndex(0);
                return;
            }

-            Otherwise, scroll to the focused index (as long as it's in range)
-            if (this.state.allOptions.length <= this.state.focusedIndex) {
-                return;
-            }
-             this.scrollToIndex(this.state.focusedIndex);
    }

Result

Screen.Recording.2023-01-06.at.11.10.47.mp4

[mountiny]

@flodnv @NicMendonca @mollfpr Hmm, I would actually argue this is not a bug, I think think that once you are ready to search for more people it is fine we are showing the recent chats. We allow up to 8 people to be selected for a group chat, so then you would only see the selected people when you want to select more, maybe from the recents field, which does not seem ideal UX.

I would say this is not a bug, maybe we should open a discussion in #expensify-open-source to align on this and create a regression test accordingly.

Or indeed fix this but endure that it works as we want.

[flodnv]

Agreed, discussing here: https://expensify.slack.com/archives/C049HHMV9SM/p1672996492671499

[abdulrahuman5196]

Proposal
IMO, Along with the current clear text scroll bug, in this current bug code place, I see we scroll to index 0, but set the focusedIndex state(highlighted row index) as the newFocusedIndex in the code. (We are scrolling to show different row but highlighting a different row).
If we only just scroll to index 0 when clear the text also to particularly solve this issue, we would still highlight a different row (Row might even be hidden), which will create a new bug in this UI (Which is not happening now as currently it scroll to first unselected row and highlight the same).

For eg, Assume we have a list of 10 people, assuming view can only show 5 rows(different screen sizes can show higher or lower count). We have already selected 5 rows and we clear the search, now we scroll to index 0 as per requirement but set the highlighted row to be 6th row which would be hidden as per below code. Even if you press 'enter' now it only select the 6th row which is hidden from view.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 103 to 115 in b081ed1

	const newOptions = this.flattenSections();
	const newFocusedIndex = this.props.selectedOptions.length;
	// eslint-disable-next-line react/no-did-update-set-state
	this.setState({
	allOptions: newOptions,
	focusedIndex: newFocusedIndex,
	}, () => {
	// If we just toggled an option on a multi-selection page, scroll to top
	if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
	this.scrollToIndex(0);
	return;
	}

This is weird IMO as we are highlighting a different row but scrolling to a different row, if we just scroll to index 0 to solve this issue

I have also posted a separate bug in the bug channel now to report the current bug. As we set the highlighted row to be different, there is always a possibility the highlighted row will be hidden when we scroll to 0 row which will also happen if we just fix clear text issue - https://expensify.slack.com/archives/C049HHMV9SM/p1673109991457219.

So I am proposing below solutions to solve this clear text issue without causing a new highlight issue bug.

Current behaviour:

What row should we highlight and scrolled after a user is selected?

Highlight = first row in unselected section, scrollTo = 0

What row should we highlight and scrolled when we unselected a user?

Highlight = first row in unselected section, scrollTo = 0

What row should we highlight and scrolled when user types in search?

Highlight = first row in unselected section, scrollTo = No scroll

What row should we highlight and scrolled when user clears search?

Highlight = first row in unselected section, scrollTo = first row in unselected section

What position should we scroll the highlighted row to (‘viewPosition’ key)?

Default

---

Proposed Behaviour 1 (Best IMO):

What row should we highlight and scrolled after a user is selected?

Highlight = last selected row, scrollTo = last selected row

What row should we highlight and scrolled when we unselected a user?

Highlight = 0, scrollTo = 0

What row should we highlight and scrolled when user types in search?

Highlight = first row in unselected section, scrollTo = first row in unselected section

What row should we highlight and scrolled when user clears search?

Highlight = 0, scrollTo = 0

What position should we scroll the highlighted row to (‘viewPosition’ key)?

Default

--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -96,23 +96,22 @@ class BaseOptionsSelector extends Component {
     }
 
     componentDidUpdate(prevProps) {
-        if (_.isEqual(this.props.sections, prevProps.sections)) {
+        if (_.isEqual(this.props.sections, prevProps.sections) && _.isEqual(this.props.value, prevProps.value)) {
             return;
         }
 
         const newOptions = this.flattenSections();
-        const newFocusedIndex = this.props.selectedOptions.length;
+        let newFocusedIndex = this.props.selectedOptions.length;
+        if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
+            newFocusedIndex = prevProps.selectedOptions.length > this.props.selectedOptions.length ? 0 : this.props.selectedOptions.length - 1
+        } else if (_.isEmpty(this.props.value) && (this.props.value !== prevProps.value)) {
+            newFocusedIndex = 0;
+        }
         // eslint-disable-next-line react/no-did-update-set-state
         this.setState({
             allOptions: newOptions,
             focusedIndex: newFocusedIndex,
         }, () => {
-            // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
-                this.scrollToIndex(0);
-                return;
-            }
-
             // Otherwise, scroll to the focused index (as long as it's in range)
             if (this.state.allOptions.length <= this.state.focusedIndex) {
                 return;

Proposal.1.mp4

---

Proposal behaviour 2:

What row should we highlight and scrolled after a user is selected?

Highlight = 0, scrollTo = 0

What row should we highlight and scrolled when we unselected a user?

Highlight = 0, scrollTo = 0

What row should we highlight and scrolled when user types in search?

Highlight = first row in unselected section, scrollTo = first row in unselected section

What row should we highlight and scrolled when user clears search?

Highlight = 0, scrollTo = 0

What position should we scroll the highlighted row to (‘viewPosition’ key)?

Default

Code diff

--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -96,23 +96,21 @@ class BaseOptionsSelector extends Component {
     }
 
     componentDidUpdate(prevProps) {
-        if (_.isEqual(this.props.sections, prevProps.sections)) {
+        if (_.isEqual(this.props.sections, prevProps.sections) && _.isEqual(this.props.value, prevProps.value)) {
             return;
         }
 
         const newOptions = this.flattenSections();
-        const newFocusedIndex = this.props.selectedOptions.length;
+        let newFocusedIndex = this.props.selectedOptions.length;
+        if (this.props.selectedOptions.length !== prevProps.selectedOptions.length
+            || (_.isEmpty(this.props.value) && (this.props.value !== prevProps.value))) {
+            newFocusedIndex = 0;
+        }
         // eslint-disable-next-line react/no-did-update-set-state
         this.setState({
             allOptions: newOptions,
             focusedIndex: newFocusedIndex,
         }, () => {
-            // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
-                this.scrollToIndex(0);
-                return;
-            }
-
             // Otherwise, scroll to the focused index (as long as it's in range)
             if (this.state.allOptions.length <= this.state.focusedIndex) {
                 return;

Proposal.2.mp4

---

Proposal behaviour 3:

What row should we highlight and scrolled after a user is selected?

Highlight = first row in unselected section, scrollTo = first row in unselected section

What row should we highlight and scrolled when we unselected a user?

Highlight = first row in unselected section, scrollTo = first row in unselected section

What row should we highlight and scrolled when user types in search?

Highlight = first row in unselected section, scrollTo = first row in unselected section

What row should we highlight and scrolled when user clears search?

Highlight = 0, scrollTo = 0

What position should we scroll the highlighted row to (‘viewPosition’ key)?
Default

--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -96,23 +96,20 @@ class BaseOptionsSelector extends Component {
     }
 
     componentDidUpdate(prevProps) {
-        if (_.isEqual(this.props.sections, prevProps.sections)) {
+        if (_.isEqual(this.props.sections, prevProps.sections) && _.isEqual(this.props.value, prevProps.value)) {
             return;
         }
 
         const newOptions = this.flattenSections();
-        const newFocusedIndex = this.props.selectedOptions.length;
+        let newFocusedIndex = this.props.selectedOptions.length;
+        if ((_.isEmpty(this.props.value) && (this.props.value !== prevProps.value))) {
+            newFocusedIndex = 0;
+        }
         // eslint-disable-next-line react/no-did-update-set-state
         this.setState({
             allOptions: newOptions,
             focusedIndex: newFocusedIndex,
         }, () => {
-            // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
-                this.scrollToIndex(0);
-                return;
-            }
-
             // Otherwise, scroll to the focused index (as long as it's in range)
             if (this.state.allOptions.length <= this.state.focusedIndex) {
                 return;

Proposal.3.mp4

---

I think proposal 1 would have the best behaviour. But, If required we can also mix and match the above 3 to create different behaviour combination.

---

Proposal with just clear text scroll fix

Code diff

--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -108,7 +108,8 @@ class BaseOptionsSelector extends Component {
             focusedIndex: newFocusedIndex,
         }, () => {
             // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
+            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length
+                || (_.isEmpty(this.props.value) && (this.props.value !== prevProps.value))) {
                 this.scrollToIndex(0);
                 return;
             }

Proposal.mp4

[mollfpr]

@Ollyws proposal's looking good to me. The expected result will be to scroll to the top so the selected user is visible whenever the search box is cleared.

🎀 👀 🎀 C+ reviewed!

cc @flodnv @NicMendonca

[allroundexperts]

@mollfpr My proposal does the same thing. Have you considered it?

[abdulrahuman5196]

@mollfpr doesn't just scrolling to top without changing the highlight cause the issue I mentioned here - #14001 (comment)

Are we ok with that behaviour?

[mollfpr]

@allroundexperts Yes. In your proposal selecting an account and searching for another account by typing in the search box, the scroll position remains the same (on top). I don't think we want that.

---

For eg, Assume we have a list of 10 people, assuming view can only show 5 rows(different screen sizes can show higher or lower count). We have already selected 5 rows and we clear the search, now we scroll to index 0 as per requirement but set the highlighted row to be 6th row which would be hidden as per below code. Even if you press 'enter' now it only select the 6th row which is hidden from view.

Thanks for the detailed @abdulrahuman5196 but what you pointing out is another issue and we don't know yet if that will be an issue.

I don't mind after selecting an account the highlight option is the first row unselected option, this will be helpful if navigating through the options with the arrow key to go through the unselected options.

[allroundexperts]

@mollfpr Are you sure we don't want that behaviour? Because In my opinion, it makes more sense to keep the selected item sticky throughout.

[mollfpr]

Because In my opinion, it makes more sense to keep the selected item sticky throughout.

@allroundexperts For example, if we have 7 selected options already and searching for another account will only give a smaller space visibility to show unselected options, am I correct? If is that so, I think this will be an issue in the future or even regression.

[mollfpr]

Ahh sorry for that, I must be misspelling your name before. 😅

[Ollyws]

All yours @mollfpr !

[flodnv]

lol whoops thanks @mountiny

Thanks for the PR @Ollyws

[sophiepintoraetz]

@flodnv - can you confirm that Olly is to be paid $1000 + the PR merge bonus of 50% (i.e. $1500?)

[flodnv]

Yes please 👍

[mollfpr]

Will I get the bonus too? 😁

[flodnv]

Yep 👍

[sophiepintoraetz]

@Ollyws - contract for payment is with you
@mollfpr - you can apply for the job here for $1500 also.

[mollfpr]

@sophiepintoraetz I can't open the link.

[sophiepintoraetz]

Sorry, try this one!

[mollfpr]

@sophiepintoraetz Applied, thank you!

[mollfpr]

@sophiepintoraetz I think you should end the Upwork contract after the PR is in production and past the 7 days regression period 😅

Currently, the PR is still in staging but maybe later today or tomorrow will hit production.

[sophiepintoraetz]

Ah, I am on auto-pilot mode - wow - let's hope a regression doesn't pop up 😅

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.52-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix for: Chat - New Group - The search box overlays the selected user #14175

If no regressions arise, payment will be issued on 2023-01-19. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

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

• [@mollfpr / @flodnv] The PR that introduced the bug has been identified. Link to the PR:
• [@mollfpr / @flodnv] 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:
• [@mollfpr / @flodnv] 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:
• [@sophiepintoraetz] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[sophiepintoraetz]

@flodnv @mollfpr shall we close this GH out? Or is there an offending PR we need to source here?

[mollfpr]

@sophiepintoraetz @flodnv I don’t there’s a PR causing this regression. As mentioned in the Slack discussion, is more of a polish issue.