[$250] [Search v2.2] - Chats - Selected chat is hidden when navigating via keyboard

[IuliiaHerets]

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: 9.0.31-14
Reproducible in staging?: Y
Reproducible in production?: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

• Account has many chats.

1. Go to staging.new.expensify.com
2. Go to Search > Chats.
3. Navigate to the bottom of the list via keyboard up button.

Expected Result:

The selected chat will be not hidden when navigating via keyboard.

Actual Result:

The selected chat is hidden when navigating via keyboard.
This issue happens with the chat at the bottom of the list.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6598844_1725981162147.20240910_230910.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021833559992291211111
• Upwork Job ID: 1833559992291211111
• Last Price Increase: 2024-10-01

Issue Owner

Current Issue Owner: @rayane-djouah

[melvin-bot]

Triggered auto assignment to @marcochavezf (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[marcochavezf]

I don't see the Chats section in Search tab in production:

Seems this is a new feature and I don't think it should block the deploy

[melvin-bot]

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

[melvin-bot]

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

[Nodebrute]

@marcochavezf I am unable to reproduce this issue on latest main

Screen.Recording.2024-09-10.at.10.59.35.PM.mov

[Amoralchik]

Edited by proposal-police: This proposal was edited at 2024-09-10 21:07:46 UTC.

Proposal

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

The element selected using the keyboard is outside the visible area.

What is the root cause of that problem?

In useArrowKeyFocusManager, for the up and down arrows, we call the scrollToIndex function within onFocusedIndexChange, which scrolls to the selected element.

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 273 to 282 in 41e6968

	const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
	initialFocusedIndex: flattenedSections.allOptions.findIndex((option) => option.keyForList === initiallyFocusedOptionKey),
	maxIndex: Math.min(flattenedSections.allOptions.length - 1, CONST.MAX_SELECTION_LIST_PAGE_LENGTH * currentPage - 1),
	disabledIndexes: disabledArrowKeyIndexes,
	isActive: isFocused,
	onFocusedIndexChange: (index: number) => {
	scrollToIndex(index, true);
	},
	isFocused,
	});

scrollToLocation isn't functioning correctly due to variations in element sizes, caused by the offset in getItemLayout not accurately reflecting the distance between elements. This is due to the offset being set here:

App/src/components/Search/index.tsx

Lines 149 to 151 in a8b5914

	if (SearchUtils.isTransactionListItemType(item) || SearchUtils.isReportActionListItemType(item)) {
	return isLargeScreenWidth ? variables.optionRowHeight + listItemPadding : transactionItemMobileHeight + listItemPadding;
	}

to be the same as for Transactions.

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

First, I would remove scrollToIndex from onFocusedIndexChange since I don’t see the point in keeping it (see optional).

We can set a fixed height for all elements or somehow determine the height of the component and store this data in reportAction. Then, we can retrieve it in getItemHeight.

proof of concept:

2024-09-11.18-55-07.mp4

What alternative solutions did you explore? (Optional)

I propose removing scrollToIndex from onFocusedIndexChange, as it only handles scrolling, and without it, I noticed everything works correctly.

code after:

    const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
        initialFocusedIndex: flattenedSections.allOptions.findIndex((option) => option.keyForList === initiallyFocusedOptionKey),
        maxIndex: Math.min(flattenedSections.allOptions.length - 1, CONST.MAX_SELECTION_LIST_PAGE_LENGTH * currentPage - 1),
        disabledIndexes: disabledArrowKeyIndexes,
        isActive: isFocused,
        onFocusedIndexChange: () => {},
        isFocused,
    });

proof of concept:

2024-09-10.22-55-20.mp4

[1subodhpathak]

I am able to reproduce this problem!

Recording.2024-09-11.195526.mp4

But observed one thing, it only happens when the chat size greater than initialy defined is selected

See this

[Amoralchik]

Update Proposal

[isabelastisser]

@rayane-djouah, are you still able to reproduce this? Thanks!

[rayane-djouah]

I'm able to reproduce

Screen.Recording.2024-09-11.at.11.48.46.PM.mov

[melvin-bot]

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

[rayane-djouah]

Reviewing now 👀

[1subodhpathak]

Proposal

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

The issue occurs when the user navigates to the bottom of the chat list using the keyboard, the selected chat becomes hidden instead of staying visible

What is the root cause of that problem?

The root cause of this problem is related to the focus and scrolling behavior when using keyboard navigation in the chat list.

• The useArrowKeyFocusManager hook, which handles keyboard navigation, does not ensure the focused chat item remains visible when the user navigates to the bottom.

• The scrollToIndex method in BaseSelectionList.tsx may not be properly scrolling to the last item in the list when it is selected.

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

1. update the setFocusedIndex function in arrowUpCallback and arrowDownCallback to ensure that when the focused index changes, the list scrolls to bring the selected chat into view.
   

   App/src/hooks/useArrowKeyFocusManager.ts

   Lines 80 to 83 in bd0132b

   	const arrowUpCallback = useCallback(() => {
   	if (maxIndex < 0 || !isFocused) {
   	return;
   	}

2. Update the scrollToIndex function to ensure that when navigating to the last item in the list, the selected item is scrolled into view and remains visible.
   

   App/src/components/SelectionList/BaseSelectionList.tsx

   Lines 245 to 261 in bd0132b

   	const scrollToIndex = useCallback(
   	(index: number, animated = true) => {
   	const item = flattenedSections.allOptions[index];
   	if (!listRef.current || !item) {
   	return;
   	}
   	const itemIndex = item.index ?? -1;
   	const sectionIndex = item.sectionIndex ?? -1;
   	listRef.current.scrollToLocation({sectionIndex, itemIndex, animated, viewOffset: variables.contentHeaderHeight});
   	},
   	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
   	[flattenedSections.allOptions],
   	);

3. onFocusedIndexChange passed to useArrowKeyFocusManager triggers the scrolling behavior in BaseSelectionList when the selected chat item changes.
   

   App/src/components/SelectionList/BaseSelectionList.tsx

   Lines 274 to 283 in bd0132b

   	const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({
   	initialFocusedIndex: flattenedSections.allOptions.findIndex((option) => option.keyForList === initiallyFocusedOptionKey),
   	maxIndex: Math.min(flattenedSections.allOptions.length - 1, CONST.MAX_SELECTION_LIST_PAGE_LENGTH * currentPage - 1),
   	disabledIndexes: disabledArrowKeyIndexes,
   	isActive: isFocused,
   	onFocusedIndexChange: (index: number) => {
   	scrollToIndex(index, true);
   	},
   	isFocused,
   	});

4. the layout is properly calculated so that the selected chat remains in view when navigating to the bottom of the list.

const getItemLayout = (data, index) => ({
    length: ITEM_HEIGHT, 
    offset: ITEM_HEIGHT * index,
    index,
}); 

What alternative solutions did you explore? (Optional)

An alternative approach could involve increasing the render window size of the list to ensure that more items are rendered and remain visible as the user navigates with the keyboard. However, this could lead to performance issues for long lists and is less optimal than ensuring the focus and scroll behavior work correctly.

[isabelastisser]

@rayane-djouah, can you please provide an update? Did you review all the proposals? Thanks!

[rayane-djouah]

@Amoralchik Thanks for the proposal.

It appears the main issue is that auto-scrolling to the active list element isn't functioning correctly due to the variations in list element height, which is not accurately reflected in the calculations.

Is the problem linked to both the getItemHeight function and the getItemLayout?

Regarding the proposed solution, setting a fixed height might not be ideal as it breaks the content, We need to ensure the full content of report actions is displayed.

How might we dynamically determine the height of ReportActionListItemType items?

Regarding your optional solution: removing scrollToIndex causes a regression:

Staging	Removing scrollToIndex
Screen.Recording.2024-09-20.at.12.16.27.PM.mov	Screen.Recording.2024-09-20.at.12.15.47.PM.mov

[rayane-djouah]

@1subodhpathak I don't see a difference between your proposal and the later one. Additionally, please refrain from using overly wordy explanations; the changes you intend to make to fix the bug are not clear. Make sure to follow the guidelines in CONTRIBUTING.md when submitting proposals.

[1subodhpathak]

Okay! marked your words @rayane-djouah
Thanks for the review

[Amoralchik]

@rayane-djouah Hi,

How might we dynamically determine the height of ReportActionListItemType items? - I think (though I'm not sure, this is just a hypothesis) we could try creating a function that splits the text into Markdown from the HTML value, then gathers all the attributes and adds them to the block size.

To make it clearer, here’s an example of what the code might look like:

function calculateHeight(inputString) {
  const lines = inputString.split('\n'); // or <br/>
  let height = 0;
  lines.forEach(line => {
    if (line.startsWith('<h1>')) {
      height += variables.h1; //
    } 
    // ......
    else {
      height += variables.span;
    }
  });

  return height;
}

I will try this approach, and if everything works well, I will update the proposal by/on Monday.

Is the problem linked to both the getItemHeight function and the getItemLayout? - Yes
Here:

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 172 to 174 in 41e6968

	const fullItemHeight = getItemHeight(item);
	itemLayouts.push({length: fullItemHeight, offset});
	offset += fullItemHeight;

and then here:

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 357 to 358 in 41e6968

	const getItemLayout = (data: Array<SectionListData<TItem, SectionWithIndexOffset<TItem>>> | null, flatDataArrayIndex: number) => {
	const targetItem = flattenedSections.itemLayouts[flatDataArrayIndex];

Regarding your optional solution: removing scrollToIndex causes a regression - Oh I'm sorry, yeah that makes sense

[rayane-djouah]

@rayane-djouah, can you please provide an update? Did you review all the proposals? Thanks!

@isabelastisser We don't have a satisfactory proposal yet.

[rayane-djouah]

Waiting on proposals

[melvin-bot]

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

[melvin-bot]

@marcochavezf @isabelastisser @rayane-djouah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[rayane-djouah]

Waiting on proposals

[melvin-bot]

@marcochavezf, @isabelastisser, @rayane-djouah Huh... This is 4 days overdue. Who can take care of this?

[rayane-djouah]

Waiting on proposals

[melvin-bot]

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

[rayane-djouah]

@isabelastisser Do we need to adjust the bounty for this issue to get more proposals?

[isabelastisser]

@rayane-djouah, we're keeping the original amount. Thanks!

[tushar-a-b]

Proposal

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

Selected chat is hidden when navigating via keyboard

What is the root cause of that problem?

We are setting static fullItemHeight of 76 for every item in the itemLayouts of flattenedSections list here:
(you can try console logging flattenedSections and look for itemLayouts you will notice every item has 76 height even if it large in size.)

App/src/components/SelectionList/BaseSelectionList.tsx

Line 175 in 99f280b

const fullItemHeight = getItemHeight(item);

App/src/components/Search/index.tsx

Lines 132 to 150 in 99f280b

	const getItemHeight = useCallback(
	(item: TransactionListItemType | ReportListItemType | ReportActionListItemType) => {
	if (SearchUtils.isTransactionListItemType(item) || SearchUtils.isReportActionListItemType(item)) {
	return isLargeScreenWidth ? variables.optionRowHeight + listItemPadding : transactionItemMobileHeight + listItemPadding;
	}
	if (item.transactions.length === 0) {
	return 0;
	}
	if (item.transactions.length === 1) {
	return isLargeScreenWidth ? variables.optionRowHeight + listItemPadding : transactionItemMobileHeight + listItemPadding;
	}
	const baseReportItemHeight = isLargeScreenWidth ? 72 : 108;
	return baseReportItemHeight + item.transactions.length * reportItemTransactionHeight + listItemPadding;
	},
	[isLargeScreenWidth],
	);

We have incorrect item heights in the list with the same value of 76 for every item. When an item in the list is larger than 76 then only in that scenario we encounter this issue since it scrolls by considering every item's height as 76 and hence sometimes the selected item goes out of the view.

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

We should find out every item's original height and set the fullItemHeight while calculating itemLayouts in flattenedSections below:

App/src/components/SelectionList/BaseSelectionList.tsx

Line 175 in 99f280b

const fullItemHeight = getItemHeight(item);

To get the original height of each item we should first wrap the ListItem of renderItem around View component so that we can pass onLayout function to get the height of the item as below:

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 431 to 482 in 99f280b

	const renderItem = ({item, index, section}: SectionListRenderItemInfo<TItem, SectionWithIndexOffset<TItem>>) => {
	const normalizedIndex = index + (section?.indexOffset ?? 0);
	const isDisabled = !!section.isDisabled || item.isDisabled;
	const isItemFocused = (!isDisabled || item.isSelected) && (focusedIndex === normalizedIndex || itemsToHighlight?.has(item.keyForList ?? ''));
	// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
	const showTooltip = shouldShowTooltips && normalizedIndex < 10;
	const handleOnCheckboxPress = () => {
	if (SearchUtils.isReportListItemType(item)) {
	return onCheckboxPress;
	}
	return onCheckboxPress ? () => onCheckboxPress(item) : undefined;
	};
	return (
	<>
	<ListItem
	item={item}
	isFocused={isItemFocused}
	isDisabled={isDisabled}
	showTooltip={showTooltip}
	canSelectMultiple={canSelectMultiple}
	onLongPressRow={onLongPressRow}
	onSelectRow={() => {
	if (shouldSingleExecuteRowSelect) {
	singleExecution(() => selectRow(item, index))();
	} else {
	selectRow(item, index);
	}
	}}
	onCheckboxPress={handleOnCheckboxPress()}
	onDismissError={() => onDismissError?.(item)}
	shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
	// We're already handling the Enter key press in the useKeyboardShortcut hook, so we don't want the list item to submit the form
	shouldPreventEnterKeySubmit
	rightHandSideComponent={rightHandSideComponent}
	keyForList={item.keyForList ?? ''}
	isMultilineSupported={isRowMultilineSupported}
	isAlternateTextMultilineSupported={isAlternateTextMultilineSupported}
	alternateTextNumberOfLines={alternateTextNumberOfLines}
	onFocus={() => {
	if (shouldIgnoreFocus || isDisabled) {
	return;
	}
	setFocusedIndex(normalizedIndex);
	}}
	shouldSyncFocus={!isTextInputFocusedRef.current}
	/>
	{item.footerContent && item.footerContent}
	</>
	);
	};

to

<View onLayout={onItemLayout}>
       <ListItem 
        .........
        .........
        .........
        />
</View>

where in onItemLayout function we will track the height of every rendered item in itemHeights state so that we can use it later:

const [itemHeights, setItemHeights] = useState<{ [key: number]: number }>({});
const onItemLayout = useCallback((event: LayoutChangeEvent, itemIndex: number) => {
        const { height } = event.nativeEvent.layout;
        setItemHeights(prevHeights => ({
            ...prevHeights,
            [itemIndex]: height,
        }));
    }, []);

Now we will use this itemHeights to set the fullItemHeight in flattenedSections so that we will get correct height for every item in itemLayouts here:

App/src/components/SelectionList/BaseSelectionList.tsx

Line 175 in 99f280b

const fullItemHeight = getItemHeight(item);

to

const fullItemHeight = itemHeights[optionIndex] ?? getItemHeight(item);

We will also update the dependency of flattenedSections and will add itemHeights to it.

const flattenedSections = useMemo<FlattenedSectionsReturn<TItem>>(() => {
...
}, [canSelectMultiple, sections, customListHeader, customListHeaderHeight, getItemHeight, itemHeights]);

Result (refer video below):

result.mp4

What alternative solutions did you explore? (Optional)