Fix wrong scrolling in Search list when navigating by keyboard #43490

Kicu · 2024-06-11T15:07:24Z

Details

Root cause

It turns out that SelectionList was not prepared to handle items of different sizes and/or items with extra padding. Inside BaseSelectionList there is an implementation of getItemLayout (https://reactnative.dev/docs/virtualizedlist#getitemlayout) function that returns sizes of items and it has a hardcoded value here: https://github.com/Expensify/App/blob/main/src/components/SelectionList/BaseSelectionList.tsx#L157-L159
I believe we have no other lists that have items different than 64 height and none of them use any paddings
in Search we have items with padding, which makes them bigger than 64, and also we have different sized items

Done:

I've added 2 new optional props to SelectionList interface:
- callback getter that returns specific item heights
- height of any extra header if it's provided
I memoized the function getHeight because it was called at least 3 times per item per render, which for me meant 200-500 calls - feels like a lot
added extra filter for ReportItems so that we do not return "widows" without any transactions, which screwed up focusing by keyboard (as there were null items with height 0)

Fixed Issues

$ #43362
PROPOSAL:

Tests

go to Search
use keyboard to navigate over search list items - Tab key, and also up and down keyboard keys
the focus and scroll should work correctly

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

rec-web-sorting-scroll.mp4

rec-web-mobile-sorting.mp4

MacOS: Desktop

filip-solecki

LGTM

melvin-bot · 2024-06-12T11:46:13Z

@fedirjh Please 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]

luacmartins · 2024-06-12T14:29:13Z

src/libs/SearchUtils.ts

@@ -155,7 +157,7 @@ function getReportSections(data: OnyxTypes.SearchResults['data']): ReportListIte
        }
    }

-    return Object.values(reportIDToTransactions);
+    return Object.values(reportIDToTransactions).filter((item) => item.transactions?.length !== 0);


We started filtering this on the backend, so we shouldn't have to do it in the frontend anymore.

Fair enough but I found such cases that fallen into this if: https://github.com/Expensify/App/blob/main/src/components/SelectionList/Search/ReportListItem.tsx#L82 or the other one.

At lest for me I can see empty list elements.

I put up a fix for this, so we should be able to remove the filter once my fix is deployed.

luacmartins · 2024-06-12T14:30:19Z

src/components/Search.tsx

+const transactionItemMobileHeight = 100;
+const reportItemTransactionHeight = 52;
+const listItemPadding = 12; // this is equivalent to 'mb3' on every transaction/report list item


NAB This feels brittle since if the height changes we would introduce the bug again. That being said, I'm not sure how to solve it this.

Exactly same for me. We could in some way extract the same variable from styles perhaps and use, but still you would have to change it in two places.

I agree that its brittle but I see no other simple solution. padding affects item height, and correct item height has to be pushed down to RN List

luacmartins · 2024-06-12T15:12:23Z

@fedirjh all yours

luacmartins · 2024-06-13T16:48:09Z

Bump @fedirjh for review

fedirjh · 2024-06-14T12:26:01Z

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: mWeb Chrome
- iOS: Native
- iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

CleanShot.2024-06-18.at.12.36.03.mp4

Android: mWeb Chrome

CleanShot.2024-06-18.at.12.43.23.mp4

iOS: Native

CleanShot.2024-06-18.at.12.04.52.mp4

iOS: mWeb Safari

CleanShot.2024-06-18.at.12.12.07.mp4

MacOS: Chrome / Safari

CleanShot.2024-06-18.at.11.01.55.mp4

MacOS: Desktop

CleanShot.2024-06-18.at.11.24.12.mp4

CleanShot.2024-06-18.at.11.24.51.mp4

fedirjh · 2024-06-14T13:17:40Z

@Kicu The navigation using the TAB key is not smooth; it feels there is a delay:

CleanShot.2024-06-14.at.14.03.15.mp4

src/components/Search.tsx

Kicu · 2024-06-17T06:30:38Z

@fedirjh do you mind checking if the same feeling when using TAB appears also on main, and if not then can we move forward?
The purpose of this PR was not to affect how moving looks, nor speed it up - it fixes a specific bug that made this navigation almost impossible (because the scroll would behave badly).
I have not really tried to improve the speed yet - that would be a different task I think

fedirjh · 2024-06-17T09:11:32Z

do you mind checking if the same feeling when using TAB appears also on main, and if not then can we move forward?

@Kicu Yes it seems this behavior is happening on main as well.

There is another bug with scrolling, it took several clicks to navigate between the first item and the second (3-4 clicks)

CleanShot.2024-06-17.at.09.59.11.mp4

Also, the scrolling behavior looks weird; sometimes it takes two or three clicks to navigate to the next item:

CleanShot.2024-06-17.at.10.01.31.mp4

Kicu · 2024-06-17T09:30:38Z

Yeah I'm pretty sure I understand what happens 👍 Unfortunately there are empty "invisible" items on this list, and I was not sure if its only for me or is this more common.

Now that I know I will filter them out and that should help.
@luacmartins please take a look at the videos here: #43490 (comment) - it seems like the case there there is a report item but no transactions is more common. I will filter them out in Frontend, but if you can maybe you should investigate why this happens.

Kicu · 2024-06-17T09:41:08Z

@fedirjh please re-check, the weird navigation behaviour should be fixed

fedirjh · 2024-06-17T10:06:42Z

please re-check, the weird navigation behaviour should be fixed

@Kicu Yep results look good so far. navigating between items is now fixed. For scrolling, there is still a little weirdness, the list sometimes scrolls up and down when you have a large item in the list: I think this is related to the fact that we have two scrolling behaviors implemented at the same time :

SectionList auto-scrolls when an item is focused:
scrollToIndex that scrolls to the index of the selected element, it's triggered through the useArrowKeyFocusManager

I think we probably should disable the SectionList auto-scrolls on item focus behavior. I am not sure it that's possible, though.

CleanShot.2024-06-17.at.10.56.13.mp4

Kicu · 2024-06-17T10:57:52Z

will look into this later today or tomorrow and ping you

luacmartins · 2024-06-17T15:45:53Z

@luacmartins please take a look at the videos here: #43490 (comment) - it seems like the case there there is a report item but no transactions is more common. I will filter them out in Frontend, but if you can maybe you should investigate why this happens.

I'll take a look since we should be filtering those in the backend.

luacmartins · 2024-06-17T17:28:12Z

I put up a PR to fix the report without transactions issue. You can remove the filtering in the frontend again since the fix for that issue is in the backend.

src/libs/SearchUtils.ts

Kicu · 2024-06-18T09:18:39Z

@fedirjh I looked at the scrolling behaviors in our SelectionList. In general I think you're right that we have both native SectionList scrolling and our own mechanisms, but without it the items focused via Tab key and items focused when using keyboard will desynchronize.
As for disabling the native scrolling index in SectionList I don't think its possible.

That being said I think this is a bigger problem and this behaviour that we see here is what happens everywhere in the app where SelectionList is being used. Check for example my video - items are the same height, but the scrolling behavior still happens on every keystroke.

rec-web-sorting.mp4

I think the only difference here is that Search is currently the only list in Expensify where items can have different heights. Other than that, this is simply how SelectionList behaves.
I'm reluctant to change core behaviors of SelectionList right now, perhaps we should plan this for future as an improvement.

CC @luacmartins
Everything else should be fixed.
Filtering of empty items is removed because it will be fixed by backend as requested.
This means that the bug with keyboard sometimes requiring more than 1 keystroke to navigate to next item is expected on this PR right now.

fedirjh

Over to you @luacmartins

luacmartins

LGTM

OSBotify · 2024-06-18T16:34:30Z

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

OSBotify · 2024-06-19T21:52:37Z

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

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

OSBotify · 2024-06-19T22:17:19Z

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

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

OSBotify · 2024-06-19T22:51:18Z

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

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

kbecciv · 2024-06-20T13:37:19Z

This PR is failing because of issue #43362

The issue is reproducible in: Web and Desktop

1718884561366.Fix_wrong_scrolling_in_Search_list_when_navigating_by_keyboard.mp4

1718886939047.43490_desktop.mp4

Kicu · 2024-06-20T13:54:29Z

@kbecciv Sorry but I'm not sure I understand. This PR was specifically fixing the issue that you mention: #43362
I tested it against this exact scenario and it worked.

Right now I re-tested current main branch and I'm not able to reproduce the behavior in your videos.
What we can see in your videos is exactly the state of the bug before this PR was merged.

OSBotify · 2024-06-21T21:21:56Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.0-9 🚀

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

Fix wrong scrolling in Search list when navigating by keyboard

9da4042

luacmartins self-requested a review June 11, 2024 16:09

filip-solecki approved these changes Jun 12, 2024

View reviewed changes

Kicu added 3 commits June 12, 2024 09:45

Fix lint in SelectionList

c41ca39

Make sure Search scrolling works on narrow layout

a97b14d

Merge branch 'main' into kicu/43362-sorting-scrolling-fix

8575219

Kicu force-pushed the kicu/43362-sorting-scrolling-fix branch from 3d3743e to 8575219 Compare June 12, 2024 11:43

Kicu marked this pull request as ready for review June 12, 2024 11:46

Kicu requested a review from a team as a code owner June 12, 2024 11:46

melvin-bot bot requested review from fedirjh and removed request for a team June 12, 2024 11:46

luacmartins requested changes Jun 12, 2024

View reviewed changes

Remove filtering empty reports in Search

c8904da

luacmartins previously approved these changes Jun 12, 2024

View reviewed changes

fedirjh reviewed Jun 14, 2024

View reviewed changes

src/components/Search.tsx Outdated Show resolved Hide resolved

Kicu added 2 commits June 17, 2024 11:33

Merge branch 'main' into kicu/43362-sorting-scrolling-fix

f50473d

Add small fixes to search

5f1a9be

Kicu dismissed luacmartins’s stale review via 5f1a9be June 17, 2024 09:39

luacmartins mentioned this pull request Jun 17, 2024

[Search v1] Some reports are returned without transactions #43853

Closed

luacmartins self-requested a review June 17, 2024 18:31

luacmartins requested changes Jun 17, 2024

View reviewed changes

src/libs/SearchUtils.ts Outdated Show resolved Hide resolved

Kicu added 2 commits June 18, 2024 09:30

Remove extra filtering of Search ReportItems

eca4735

Merge branch 'main' into kicu/43362-sorting-scrolling-fix

bd00445

fedirjh approved these changes Jun 18, 2024

View reviewed changes

melvin-bot bot requested a review from luacmartins June 18, 2024 15:42

luacmartins approved these changes Jun 18, 2024

View reviewed changes

luacmartins merged commit abc9882 into Expensify:main Jun 18, 2024
16 checks passed

github-actions bot mentioned this pull request Jun 19, 2024

Deploy Checklist: New Expensify 2024-06-19 #44047

Closed

59 tasks

melvin-bot bot mentioned this pull request Jun 21, 2024

[HOLD for payment 2024-06-28] [$250] [Search v1] - Selected expense at the bottom of the list is not revealed when selected via keyboard #43362

Closed

6 tasks

ahmedGaber93 mentioned this pull request Jul 23, 2024

[PAID] [$250] [Search v1] - Sometimes blank space appears instead of skeleton loader when scrolling #43396

Closed

6 tasks

Kicu deleted the kicu/43362-sorting-scrolling-fix branch July 24, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong scrolling in Search list when navigating by keyboard #43490

Fix wrong scrolling in Search list when navigating by keyboard #43490

Kicu commented Jun 11, 2024 •

edited

Loading

filip-solecki left a comment

melvin-bot bot commented Jun 12, 2024

luacmartins Jun 12, 2024

Kicu Jun 12, 2024

luacmartins Jun 17, 2024

luacmartins Jun 12, 2024

Kicu Jun 12, 2024

luacmartins commented Jun 12, 2024

luacmartins commented Jun 13, 2024

fedirjh commented Jun 14, 2024 •

edited

Loading

fedirjh commented Jun 14, 2024

Kicu commented Jun 17, 2024

fedirjh commented Jun 17, 2024

Kicu commented Jun 17, 2024

Kicu commented Jun 17, 2024

fedirjh commented Jun 17, 2024

Kicu commented Jun 17, 2024

luacmartins commented Jun 17, 2024

luacmartins commented Jun 17, 2024

Kicu commented Jun 18, 2024

fedirjh left a comment

luacmartins left a comment

OSBotify commented Jun 18, 2024

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

kbecciv commented Jun 20, 2024

Kicu commented Jun 20, 2024 •

edited

Loading

OSBotify commented Jun 21, 2024

Fix wrong scrolling in Search list when navigating by keyboard #43490

Fix wrong scrolling in Search list when navigating by keyboard #43490

Conversation

Kicu commented Jun 11, 2024 • edited Loading

Details

Root cause

Done:

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

filip-solecki left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Jun 12, 2024

luacmartins Jun 12, 2024

Choose a reason for hiding this comment

Kicu Jun 12, 2024

Choose a reason for hiding this comment

luacmartins Jun 17, 2024

Choose a reason for hiding this comment

luacmartins Jun 12, 2024

Choose a reason for hiding this comment

Kicu Jun 12, 2024

Choose a reason for hiding this comment

luacmartins commented Jun 12, 2024

luacmartins commented Jun 13, 2024

fedirjh commented Jun 14, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

fedirjh commented Jun 14, 2024

Kicu commented Jun 17, 2024

fedirjh commented Jun 17, 2024

Kicu commented Jun 17, 2024

Kicu commented Jun 17, 2024

fedirjh commented Jun 17, 2024

Kicu commented Jun 17, 2024

luacmartins commented Jun 17, 2024

luacmartins commented Jun 17, 2024

Kicu commented Jun 18, 2024

fedirjh left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 18, 2024

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

kbecciv commented Jun 20, 2024

Kicu commented Jun 20, 2024 • edited Loading

OSBotify commented Jun 21, 2024

Kicu commented Jun 11, 2024 •

edited

Loading

fedirjh commented Jun 14, 2024 •

edited

Loading

Kicu commented Jun 20, 2024 •

edited

Loading