[$500] Android - There is a vertical shake every time I tap on a member

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.24.0
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:

Issue found when executing PR #33584

Action Performed:

1. Log in with an expensifail account
2. Create a workspace
3. Invite new members
4. Tap on several members checkbox

Expected Result:

There shouldn't be any shaking

Actual Result:

There is a vertical shake every time I tap on a member

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

Bug6338144_1704917381544.az_recorder_20240110_194241.mp4

View all open jobs on GitHub

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017ee358f3a7b2d9bd

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

Proposal

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

There is a vertical shake every time we tap to select a member on invite to workspace page.

• the issue is present both Android and iOS native platforms

What is the root cause of that problem?

When selecting a member to invite, within BaseSelectionList.js the scrollToIndex() function is called twice, once in the selectRow() when selecting a member and a second time in useEffect() when selected members section section changes which resets focus and scrolls to selected member at index 0 (first selected in the list).

This causes the vertical scrolling shake animation between the first call of the scroll via selectRow() and the second one which resets the focus and scrolls to member at index 0 (first selected in the list) via the useEffect() when the selected members section updates.

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

App/src/components/SelectionList/BaseSelectionList.js

Lines 210 to 217 in 6aa9062

	if (!shouldUnfocusRow) {
	setFocusedIndex(selectedOptionsCount);
	}
	if (!item.isSelected) {
	// If we're selecting an item, scroll to it's position at the top, so we can see it
	scrollToIndex(Math.max(selectedOptionsCount - 1, 0), true);
	}

Change the second if's condition to (!item.isSelected && !shouldUnfocusRow), adding !shouldUnfocusRow since the onSelectRow={() => selectRow(item, true)} function is initialized with shouldUnfocusRow as true (second argument) which calls setFocusedIndex(-1) meaning: unfocus all members when selecting a member with click/press.

Since inside selectRow() when shouldUnfocusRow is true it already unfocuses all members and the useEffect() resets the focus and scrolls to the first selected member at the head of the list, there's no need for the scrollToIndex(Math.max(selectedOptionsCount - 1, 0), true) to also be called because it causes the vertical scrolling shake.

Videos (main solution)

Android: Native

Screen.Recording.2024-01-11.at.02.14.17.mov

iOS: Native

Screen.Recording.2024-01-11.at.02.42.24.mov

What alternative solutions did you explore? (Optional)

As per reviewer's comment and the fact that the issue is a regression of #30438, a solution that would maintain the intended fix of the PR that introduced the issue but at the same time achieving the expected result mentioned in reviewer's comment the alternative solution would be:

App/src/components/SelectionList/BaseSelectionList.js

Line 371 in a8acf44

if (isInitialSectionListRender) {

on the line above in the useEffect introduced by the mentioned PR, add an additional check with OR canSelectMultiple that will return (not call the scrolling of useEffect).

What this does is maintain the useEffect scrolling for the intended list (single select) but not call the scrolling for multiple select (our case) - this will act as if the useEffect doesn't exist for our case, maintaining the previous behaviour for our case just as before the useEffect was introduced.

Videos (alternative solution)

iOS: Native

Screen.Recording.2024-01-17.at.14.30.02.mov

[jliexpensify]

@lanitochka17 can you repro this on a non-Expensifail account? If it affects a normal account, then maybe we could consider fixing.

@eVoloshchak - curious to get your thoughts on this issue: I'm leaning towards closing it, since it's so minor.

[lanitochka17]

Issue reproducible on gmail account
Samsung Galaxy A50/Android11

0-02-01-6bf2a973ddcc879a97586d95f3e195a9bb2d882fa1f315c40908c576872ade40_3c0fdf12f4ad36b6.mp4

[jliexpensify]

Cool, thanks @lanitochka17 - I was having trouble repro-ing. The shake looks a bit more pronounced in your second video, let me bring this up with our team.

https://expensify.slack.com/archives/C066HJM2CAZ/p1705269003874469

[jliexpensify]

Bump @eVoloshchak for reviews, thanks!

[jjcoffee]

@ikevin127 Thanks for your proposal! Based on what you've said, this looks like it may be a regression from #30438? I'm also curious to know if you have any insight as to why this only occurs on native platforms?

I think the expected result here is actually that we should scroll to the selected member, rather than scrolling to the top of the page each time. I believe #30438 was meant to adjust the scroll behaviour for searches, not for selections.

[jjcoffee]

Just noticed that the PR author is working on a PR for the regression that looks like it may fix this issue. I've asked to confirm.

[jjcoffee]

Okay the contributor says they think it will fix this issue too, so @jliexpensify let's put this on hold for #34196 and retest after.

[jliexpensify]

Thanks @jjcoffee !

[zanyrenney]

How did retesting this go @jliexpensify ?

[jliexpensify]

@zanyrenney I tested on my own @expensifail.com and it looks like the problem has gone away (I originally didn't see it on my Gmail, like Applause reported):

screen-20240128-145738.mp4

@jjcoffee can you verify this?

[jjcoffee]

@jliexpensify Still seeing it on latest main (v1.4.33-4) actually! Can we ask Applause to retest? cc @lanitochka17

34300-android-native-2024-01-30_15.07.32.mp4

[jliexpensify]

Bump @lanitochka17 for a re-test!

[lanitochka17]

Issue reproducible on the build 1.4.38-0

az_recorder_20240208_180331.mp4

[jliexpensify]

Hmm @jjcoffee - correct me if I'm wrong, but @lanitochka17's video mirrors mine: and I don't see a shake?

The shake in your video is much more prominent.

[jjcoffee]

@jliexpensify I see a bit of a wobble in @lanitochka17's video too, so there's definitely some scroll-jumping going on, it's just more subtle. I'm testing on a high traffic account so the list of members available to invite (and scrollable height) is very long; that could potentially make a difference.

Edit: retested on a smaller account and see that the effect is more in line with your and @lanitochka17's videos.

[jliexpensify]

@jjcoffee ok great - so I guess my question is: will standard users have high-traffic accounts? i.e. is it something they'll experience?

If so, lets try and find a fix. If not, lets close this - thoughts?

[jjcoffee]

@jliexpensify I believe that there are users with high traffic accounts, since we have a checklist item to test using a high traffic account. Unfortunately I've no idea how many users would hit the "high traffic" threshold, but I don't think the list of users has to be super long to trigger this particular issue!

[jliexpensify]

@jjcoffee I'm going to ask here: https://expensify.slack.com/archives/C01SKUP7QR0/p1707772810258999

Will loop back with an update!

[jliexpensify]

Bumped again as I didn't get any responses :(

[jliexpensify]

@jjcoffee - We're adding this to the Performance project, so lets work on it. Thanks!

[jjcoffee]

@ikevin127 Would you be able to confirm that your proposal is still relevant after the PR we thought would fix this got merged?

[ikevin127]

Looking at the latest main branch testing videos below we can notice that the scrolling behaviour changed, it doesn't stick to the top anymore therefore the issue cannot be reproduced. Now it scrolls to the latest added item which is handled by the selectRow function.

What changed between the JS selectRow function and the TS one is that when migrated the shouldUnfocusRow argument / functionality was removed. This together with the changes from PR #34196 result in the current behaviour:

Android: Native

Screen.Recording.2024-02-23.at.12.59.01.mov

iOS: Native

RPReplay_Final1708686037.mov

This being said, my proposal is outdated and wouldn't fix the issue anymore because the scrolling behaviour also changed.

Note

If we're looking to get back to the previous scrolling behaviour I can update my proposal to change that and also make sure this issue stays fixed - please let me know if we want that.

[jjcoffee]

@ikevin127 Thanks a lot for retesting! I agree the scrolling behaviour has now changed and so this particular issue seems fixed. cc @jliexpensify in case you have thoughts?

[jliexpensify]

Thanks @jjcoffee and @ikevin127 - lets close this out then!