[HOLD for payment 2024-02-07] [HOLD for payment 2024-02-07] [$500][Wave 9][Low] Mweb - BA - When user selects "State", the drop down list gets scrolled

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.6-1
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Tap profile icon
3. Tap Workspaces
4. Tap on a Workspace
5. Tap Bank account
6. Tap connect manually
7. Enter all fields and tap next
8. Select a state
9. Tap on state again and select any other state from the list

Expected Result:

When user selects "State", the drop down list must not be scrolled

Actual Result:

When user selects "State", the drop down list gets scrolled

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6296573_1701377547280.usethu.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b6d7a0421d65ccee
• Upwork Job ID: 1730336841339785216
• Last Price Increase: 2023-12-26

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01b6d7a0421d65ccee

[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

[melvin-bot]

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

[junsuparkswe]

I don't understand the issue since there is no drop down menu for selecting state. In the video, the scrolling occurs due to the user scrolling up, right?

[barros001]

This is not a bug, it's intentional. All lists always scroll to the selected item, if any.

App/src/components/SelectionList/BaseSelectionList.js

Line 472 in 4fc20d3

onLayout={scrollToFocusedIndexOnFirstRender}

App/src/components/SelectionList/BaseSelectionList.js

Lines 317 to 333 in 4fc20d3

	const scrollToFocusedIndexOnFirstRender = useCallback(
	({nativeEvent}) => {
	if (shouldUseDynamicMaxToRenderPerBatch) {
	const listHeight = lodashGet(nativeEvent, 'layout.height', 0);
	const itemHeight = lodashGet(nativeEvent, 'layout.y', 0);
	setMaxToRenderPerBatch((Math.ceil(listHeight / itemHeight) || 0) + CONST.MAX_TO_RENDER_PER_BATCH.DEFAULT);
	}
	if (!firstLayoutRef.current) {
	return;
	}
	scrollToIndex(focusedIndex, false);
	firstLayoutRef.current = false;
	},
	[focusedIndex, scrollToIndex, shouldUseDynamicMaxToRenderPerBatch],
	);

[SSRV25]

I don't think this is a bug or problem as it is useful as of UX perspective.

[bernhardoj]

Proposal

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

Every time we close a state selection page, the page is scrolled to the top.

What is the root cause of that problem?

We have a logic to highlight and scroll to the top every time the sections data changes.

App/src/components/SelectionList/BaseSelectionList.js

Lines 358 to 369 in 6301426

	useEffect(() => {
	// do not change focus on the first render, as it should focus on the selected item
	if (firstLayoutRef.current) {
	return;
	}
	// set the focus on the first item when the sections list is changed
	if (sections.length > 0) {
	updateAndScrollToFocusedIndex(0);
	}
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [sections]);

When we close the state selection page, the sections data is actually not changed, but the problem is we always pass a new instance of sections to the component.

App/src/components/StatePicker/StateSelectorModal.js

Lines 95 to 99 in 6301426

	<SelectionList
	headerMessage={headerMessage}
	textInputLabel={label || translate('common.state')}
	textInputValue={searchValue}
	sections={[{data: searchResults, indexOffset: 0}]}

State selection page itself is a modal, so when we close it, the isVisible state is changed to false which will re-render the state selection modal component.

App/src/components/StatePicker/StateSelectorModal.js

Lines 76 to 78 in 6301426

	<Modal
	type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
	isVisible={isVisible}

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

Memoize the sections to prevent passing a new instance of it to the list component.

const sections = useMemo(() => [{data: searchResults, indexOffset: 0}], [searchResults]);

[Christinadobrzyn]

I agree, I don't think this is a bug - there's a search at the top of the page when selecting a state so the user could search for a different state if they don't see it in the drop-down. Why would it help to have the list start over once you've selected a state? Closing without action.

[bernhardoj]

@Christinadobrzyn hi, the bug here is that every time the user closes the State selection page (either by pressing back or selecting a new state), the list is scrolled to the top (0:14 - 0:16).

This doesn't happen on a country selection page for example.

[Christinadobrzyn]

Hi @bernhardoj thanks for the information. I'm still not sure what the bug is. Here's the video of what I see.

Maybe the "issue" is that there are states listed above the one selected?

2023-12-05_09-46-33.mp4

Where do you see a country selection that I can test?

[bernhardoj]

@Christinadobrzyn you can try to reduce the playback speed to 0.25x and play it back in 0:31 to 0:32. After selecting a new state, the state list is scrolled to the top. I think it's easier to notice this on android as shown in the OP video.

Here is a recording of the slowed recording above.

Screen.Recording.2023-12-06.at.03.05.32.mov

For the country selection page, you can visit staging.new.expensify.com/settings/profile/personal-details/address.

[bernhardoj]

@Christinadobrzyn can you check this again, please?

[Christinadobrzyn]

Ah okay, I do see that it looks like the state scroll to the top once a state is selected - thanks for pointing that out @bernhardoj

However, I don't think that is something we need to focus on fixing. Do you see any reason why that would impact the use of the product? It's really hard to even notice and I don't think it deters from the functionality but I'm not an engineer so maybe it's doing something else on the backend?

@eVoloshchak or @bernhardoj why should this be fixed?

[bernhardoj]

@Christinadobrzyn yes, it doesn't impact the use of the product, but personally, I always think we should always fix a flaw in the code.

[Christinadobrzyn]

Okay sounds good - asking the team for their thoughts! https://expensify.slack.com/archives/C01SKUP7QR0/p1702322692823169

[melvin-bot]

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

[Christinadobrzyn]

I think this could be part of Wave 6 so I'm just checking with the team to see what our workflow should be to associate the bug with a wave - https://expensify.slack.com/archives/C01SKUP7QR0/p1702404266022039?thread_ts=1701355181.491179&cid=C01SKUP7QR0

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

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

[melvin-bot]

Current assignee @Christinadobrzyn is eligible for the Bug assigner, not assigning anyone new.

[eVoloshchak]

@neonbhai, looks like this has caused a regression (#35374)
How can we resolve it and fix this issue?

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @eVoloshchak owed $250 (50% penalty because of regression) via NewDot
• ROLE: @neonbhai paid $250 (50% penalty because of regression) via Upwork (https://www.upwork.com/jobs/~0137e20c9aacb99228)

BugZero Checklist (@Christinadobrzyn)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/jobs/~0137e20c9aacb99228)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[neonbhai]

Hi, seems like baseSelectionList has multiple sources of truth if multiple options can be selected. I tested two solutions but it seems would need a more complicated refactor of this core component.

One thing that seems to work is that Pushing State Selector to a Page

State Selector seems to be a modal right now. It looks like a page but behaves like a modal

This brings many bugs:

Bug 1

Back button takes us 2 steps back:

Screen.Recording.2024-02-07.at.3.06.58.AM.mov

Bug 2

Side Panel does not close when tapping outside

Screen.Recording.2024-02-07.at.3.06.13.AM.mov

Bug 3

Inconsistent reloading behavior on State (user goes a step back)

Screen.Recording.2024-02-07.at.3.22.17.AM.mov

All other pages like these (Year Picker, Currency Selector, Status Clear After Pages) have been made a route (and conversely don't show this behavior)

I'll suggest we add a route to the page before proceeding with a fix

[eVoloshchak]

One thing that seems to work is that Pushing State Selector to a Page

Those 3 bugs look pretty bad, so if can resolve those while resolving this issue - that would be great.
But I think first we need to figure out why Pushing State Selector to a Page does resolve our issue

We need to know the root cause of this. Applying a workaround would work in this case, but the bug could potentially come back in case we refactor those pages (use different navigation lib, use modals, etc.) in the future
cc: @neonbhai

[Christinadobrzyn]

just checking in here, are we working on a regression? Or can I pay this out? @eVoloshchak @neonbhai

[neonbhai]

still working on this!

[neonbhai]

Updating potential solutions by EOD!

[rlinoz]

Hey @neonbhai were you able to move forward with this?

[rlinoz]

Bumping ^
@neonbhai

[neonbhai]

This issue seems to be no longer reproducible on latest main:

Screen.Recording.2024-02-19.at.2.45.31.AM.mov

@eVoloshchak please help double check this

[rlinoz]

Can't reproduce in staging either.

[Christinadobrzyn]

Ah okay, if it's not reproducible, I assume that means we can pay this out?

Here's the payment summary here - it notes the 50% regression penalty - let me know if there's something different with that.

Also does this need a regression test @eVoloshchak?

[eVoloshchak]

Ah okay, if it's not reproducible, I assume that means we can pay this out?

@Christinadobrzyn, this was fixed, but not by this PR (since it was reverted)
I don't think we're eligible for any payment in this case

[Christinadobrzyn]

Ah okay, thank you for confirming @eVoloshchak! I will close this but let me know if I'm missing anything!