Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Comment thread
mountiny marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
diff --git a/node_modules/@shopify/flash-list/dist/recyclerview/RecyclerView.js b/node_modules/@shopify/flash-list/dist/recyclerview/RecyclerView.js
index c6056a1..af012e5 100644
--- a/node_modules/@shopify/flash-list/dist/recyclerview/RecyclerView.js
+++ b/node_modules/@shopify/flash-list/dist/recyclerview/RecyclerView.js
@@ -69,6 +69,10 @@ const RecyclerViewComponent = (props, ref) => {
if (internalViewRef.current && firstChildViewRef.current) {
// Measure the outer and inner container layouts
const outerViewLayout = measureParentSize(internalViewRef.current);
+ if (outerViewLayout.width === 0 && outerViewLayout.height === 0) {
+ containerViewSizeRef.current = outerViewLayout;
+ return;
+ }
const firstChildViewLayout = measureFirstChildLayout(firstChildViewRef.current, internalViewRef.current);
containerViewSizeRef.current = outerViewLayout;
// Calculate offset of first item
@@ -99,6 +103,10 @@ const RecyclerViewComponent = (props, ref) => {
if (pendingChildIds.size > 0) {
return;
}
+ if (((_a = containerViewSizeRef.current) === null || _a === void 0 ? void 0 : _a.width) === 0 &&
+ ((_b = containerViewSizeRef.current) === null || _b === void 0 ? void 0 : _b.height) === 0) {
+ return;
+ }
const layoutInfo = Array.from(refHolder, ([index, viewHolderRef]) => {
const layout = measureItemLayout(viewHolderRef.current, recyclerViewManager.tryGetLayout(index));
// comapre height with stored layout
diff --git a/node_modules/@shopify/flash-list/src/recyclerview/RecyclerView.tsx b/node_modules/@shopify/flash-list/src/recyclerview/RecyclerView.tsx
index 73c9649..9e34907 100644
--- a/node_modules/@shopify/flash-list/src/recyclerview/RecyclerView.tsx
+++ b/node_modules/@shopify/flash-list/src/recyclerview/RecyclerView.tsx
@@ -159,6 +159,16 @@ const RecyclerViewComponent = <T,>(
if (internalViewRef.current && firstChildViewRef.current) {
// Measure the outer and inner container layouts
const outerViewLayout = measureParentSize(internalViewRef.current);
+ // Skip layout update when the container reports zero dimensions.
+ // This happens when a parent has `display: none` applied (e.g.,
+ // React's <Activity mode="hidden"> in navigation stacks). Accepting
+ // 0x0 would wipe the render stack and force a full re-initialization
+ // when the container becomes visible again.
+ if (outerViewLayout.width === 0 && outerViewLayout.height === 0) {
+ // Record the zero size so the item measurement effect also bails out.
+ containerViewSizeRef.current = outerViewLayout;
+ return;
+ }
const firstChildViewLayout = measureFirstChildLayout(
firstChildViewRef.current,
internalViewRef.current
@@ -198,6 +208,11 @@ const RecyclerViewComponent = <T,>(
if (pendingChildIds.size > 0) {
return;
}
+ // Skip item measurement when the container is hidden (0x0).
+ // See the guard in the layout params effect above.
+ if (containerViewSizeRef.current?.width === 0 && containerViewSizeRef.current?.height === 0) {
+ return;
+ }
const layoutInfo = Array.from(refHolder, ([index, viewHolderRef]) => {
const layout = measureItemLayout(
viewHolderRef.current!,
11 changes: 11 additions & 0 deletions patches/@shopify/flash-list/details.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,14 @@
- Upstream PR/issue: TBD
- E/App issue: https://github.com/Expensify/App/issues/33725
- PR introducing patch: https://github.com/Expensify/App/pull/81566

### [@shopify+flash-list+2.2.0+002+skip-layout-when-hidden.patch](@shopify+flash-list+2.2.0+002+skip-layout-when-hidden.patch)

- Reason: Prevents FlashList from losing its render state when a navigation stack hides the parent container with `display: none`. Two early-return guards added in `RecyclerView`:
1. **First `useLayoutEffect`** (measures parent container): After calling `measureParentSize()`, if both width and height are 0, return early before calling `updateLayoutParams()` or updating `containerViewSizeRef`. This preserves the last known valid window size and prevents the layout manager from receiving zero dimensions.
2. **Second `useLayoutEffect`** (measures individual items): If `containerViewSizeRef.current` is 0x0 (because the first effect bailed out), return early before calling `modifyChildrenLayout()`. This prevents item measurements taken under `display: none` (also 0) from corrupting stored layouts.
When the container becomes visible again, `onLayout` fires (React Native Web uses ResizeObserver), triggering a re-render with correct dimensions so FlashList resumes normally without re-initialization.
- Files changed: Both `src/recyclerview/RecyclerView.tsx` and `dist/recyclerview/RecyclerView.js`. The `src/` file contains the full explanatory comments describing the intent of each guard. The `dist/` file contains only the bare code without comments, since it is compiled output. If the `dist/` file changes in a future version, refer to the `src/` diff to understand the intent and re-apply the equivalent guards.
- Upstream PR/issue: TBD
- E/App issue: https://github.com/Expensify/App/issues/83976
- PR introducing patch: https://github.com/Expensify/App/pull/84887
13 changes: 2 additions & 11 deletions src/components/Form/FormProvider.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import {useFocusEffect} from '@react-navigation/native';
import {deepEqual} from 'fast-equals';
import type {ForwardedRef, ReactNode, RefObject} from 'react';
import React, {createRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react';
Expand All @@ -7,6 +6,7 @@
import {useInputBlurActions} from '@components/InputBlurContext';
import type {LocalizedTranslate} from '@components/LocaleContextProvider';
import useDebounceNonReactive from '@hooks/useDebounceNonReactive';
import useIsFocusedRef from '@hooks/useIsFocusedRef';
import useLocalize from '@hooks/useLocalize';
import useOnyx from '@hooks/useOnyx';
import {isSafari} from '@libs/Browser';
Expand Down Expand Up @@ -268,16 +268,7 @@

// Keep track of the focus state of the current screen.
// This is used to prevent validating the form on blur before it has been interacted with.
const isFocusedRef = useRef(true);

useFocusEffect(
useCallback(() => {
isFocusedRef.current = true;
return () => {
isFocusedRef.current = false;
};
}, []),
);
const isFocusedRef = useIsFocusedRef();

const resetForm = useCallback(
(optionalValue: FormOnyxValues) => {
Expand Down Expand Up @@ -454,7 +445,7 @@
},
};
},
[draftValues, inputValues, formState?.errorFields, errors, submit, setTouchedInput, shouldValidateOnBlur, onValidate, hasServerError, setIsBlurred, formID, shouldValidateOnChange],

Check warning on line 448 in src/components/Form/FormProvider.tsx

View workflow job for this annotation

GitHub Actions / ESLint check

React Hook useCallback has a missing dependency: 'isFocusedRef'. Either include it or remove the dependency array

Check warning on line 448 in src/components/Form/FormProvider.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

React Hook useCallback has a missing dependency: 'isFocusedRef'. Either include it or remove the dependency array
);
const value = useMemo(() => ({registerInput}), [registerInput]);

Expand Down
75 changes: 64 additions & 11 deletions src/components/SelectionList/ListItem/BaseListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {getButtonRole} from '@components/Button/utils';
import Icon from '@components/Icon';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback';
import type {PressableWithFeedbackProps} from '@components/Pressable/PressableWithFeedback';
import useHover from '@hooks/useHover';
import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset';
import {useMouseActions, useMouseState} from '@hooks/useMouseContext';
Expand All @@ -15,6 +16,56 @@ import variables from '@styles/variables';
import CONST from '@src/CONST';
import type {BaseListItemProps, ListItem} from './types';

type AccessibilityProps = Pick<PressableWithFeedbackProps, 'accessible' | 'role' | 'tabIndex'>;

type CalculatedAccessibilityProps = Pick<PressableWithFeedbackProps, 'role' | 'tabIndex' | 'accessibilityState'> & {
accessibleAndAccessibilityLabel: Pick<PressableWithFeedbackProps, 'accessible' | 'accessibilityLabel'>;
};

function getAccessibilityProps<TItem extends ListItem>({
role,
tabIndex,
accessible,
item,
isFocused,
canSelectMultiple,
}: AccessibilityProps & Pick<BaseListItemProps<TItem>, 'item' | 'isFocused' | 'canSelectMultiple'>) {
const accessibilityState = role === CONST.ROLE.CHECKBOX || role === CONST.ROLE.RADIO ? {checked: !!item.isSelected, selected: !!isFocused} : {selected: !!item.isSelected};

if (accessible === false) {
return {
role: CONST.ROLE.PRESENTATION,
tabIndex: -1,
accessibilityState,
accessibleAndAccessibilityLabel: {accessible: false},
} satisfies CalculatedAccessibilityProps;
}

const accessibilityLabel = getAccessibilityLabel(item);

// For single-select lists, use role="option" with aria-selected so screen readers announce "selected"/"not selected".
// For multi-select (checkbox/radio), keep existing role and state.
const isSelectableOption = !canSelectMultiple && role !== CONST.ROLE.CHECKBOX && role !== CONST.ROLE.RADIO;
const effectiveRole = isSelectableOption ? CONST.ROLE.OPTION : role;

return {
role: effectiveRole,
tabIndex,
accessibilityState,
accessibleAndAccessibilityLabel: {accessible: undefined, accessibilityLabel},
} satisfies CalculatedAccessibilityProps;
}

function getAccessibilityLabel<TItem extends ListItem>(item: TItem) {
if (item.accessibilityLabel) {
return item.accessibilityLabel;
}

const defaultAccessibilityLabel = item.text === item.alternateText ? (item.text ?? '') : [item.text, item.alternateText].filter(Boolean).join(', ');

return defaultAccessibilityLabel;
}

function BaseListItem<TItem extends ListItem>({
item,
pressableStyle,
Expand Down Expand Up @@ -84,12 +135,14 @@ function BaseListItem<TItem extends ListItem>({

const shouldShowHiddenCheckmark = shouldShowRBRIndicator && !shouldShowCheckmark && !!item.canShowSeveralIndicators;

// For single-select lists, use role="option" with aria-selected so screen readers announce "selected"/"not selected".
// For multi-select (checkbox/radio), keep existing role and state.
const isSelectableOption = !canSelectMultiple && accessibilityRole !== CONST.ROLE.CHECKBOX && accessibilityRole !== CONST.ROLE.RADIO;
const effectiveRole = isSelectableOption ? CONST.ROLE.OPTION : accessibilityRole;
const accessibilityState =
accessibilityRole === CONST.ROLE.CHECKBOX || accessibilityRole === CONST.ROLE.RADIO ? {checked: !!item.isSelected, selected: !!isFocused} : {selected: !!item.isSelected};
const {role, tabIndex, accessibilityState, accessibleAndAccessibilityLabel} = getAccessibilityProps({
role: accessibilityRole,
accessible,
tabIndex: item.tabIndex,
item,
isFocused,
canSelectMultiple,
});

return (
<OfflineWithFeedback
Expand Down Expand Up @@ -119,8 +172,6 @@ function BaseListItem<TItem extends ListItem>({
}}
disabled={isDisabled && !item.isSelected}
interactive={item.isInteractive}
accessibilityLabel={item.accessibilityLabel ?? [item.text, item.text !== item.alternateText ? item.alternateText : undefined].filter(Boolean).join(', ')}
accessibilityState={accessibilityState}
isNested
hoverDimmingValue={1}
pressDimmingValue={item.isInteractive === false ? 1 : variables.pressDimValue}
Expand All @@ -140,12 +191,14 @@ function BaseListItem<TItem extends ListItem>({
StyleUtils.getItemBackgroundColorStyle(!!item.isSelected, !!isFocused, !!item.isDisabled, theme.activeComponentBG, theme.hoverComponentBG),
]}
onFocus={onFocus}
role={role}
tabIndex={tabIndex}
// eslint-disable-next-line react/jsx-props-no-spreading -- we can't pass those props here on their own because this Component expects a discriminated Union
{...accessibleAndAccessibilityLabel}
accessibilityState={accessibilityState}
onMouseLeave={handleMouseLeave}
tabIndex={accessible === false ? -1 : item.tabIndex}
wrapperStyle={pressableWrapperStyle}
testID={`${CONST.BASE_LIST_ITEM_TEST_ID}${item.keyForList}`}
accessible={accessible}
role={accessible === false ? CONST.ROLE.PRESENTATION : effectiveRole}
>
<View
testID={testID}
Expand Down
17 changes: 13 additions & 4 deletions src/components/SelectionList/ListItem/UserListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ function UserListItem<TItem extends ListItem>({
}
}, [item, onCheckboxPress, onSelectRow]);

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const [isReportInOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${item.reportID}`, {
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- some utils that are used to get reportID return empty string "", which would make subscription to the whole collection with nullish coalescing operator, example of this could be found in NewChatPage.tsx where some hooks return reportID as empty strings
const [isReportInOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${item.reportID || undefined}`, {
selector: reportExistsSelector,
});

Expand All @@ -73,6 +73,9 @@ function UserListItem<TItem extends ListItem>({
const shouldUseIconPolicyID = !item.reportID && !item.accountID && !item.policyID;
const policyID = isThereOnlyWorkspaceIcon && shouldUseIconPolicyID ? String(item.icons?.at(0)?.id) : item.policyID;

const shouldDisableAccessibleGrouping = !!rightHandSideComponent && !canSelectMultiple;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate accessibility ungrouping on rendered right-side control

shouldDisableAccessibleGrouping now keys off !!rightHandSideComponent, which is always true when a callback is provided, even when that callback returns null for a given row (e.g., NewChatPage rows for self-DM or excluded emails). That means those rows still get accessible={false} on the main pressable without an actual sibling action to justify ungrouping, so per-row accessibility semantics regress compared with the previous behavior that checked the rendered right component.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that.


const contactAccessibilityLabel = item.text === item.alternateText ? (item.text ?? '') : [item.text, item.alternateText].filter(Boolean).join(', ');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-3 (docs)

The accessibility label logic item.text === item.alternateText ? (item.text ?? '') : [item.text, item.alternateText].filter(Boolean).join(', ') is duplicated here and in the newly added getAccessibilityLabel function in BaseListItem.tsx (which also handles the item.accessibilityLabel check). Reuse the existing helper instead of duplicating the logic.

Import and call getAccessibilityLabel from BaseListItem:

import {getAccessibilityLabel} from './BaseListItem';
// ...
const contactAccessibilityLabel = getAccessibilityLabel(item);

(This requires exporting getAccessibilityLabel from BaseListItem.tsx.)


Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that.

return (
<BaseListItem
item={item}
Expand All @@ -96,13 +99,19 @@ function UserListItem<TItem extends ListItem>({
keyForList={item.keyForList}
onFocus={onFocus}
shouldSyncFocus={shouldSyncFocus}
accessible={shouldDisableAccessibleGrouping ? false : undefined}
shouldDisableHoverStyle={shouldDisableHoverStyle}
>
{(hovered?: boolean) => {
const isHovered = !!hovered && !shouldDisableHoverStyle;

return (
<>
<View
accessible={shouldDisableAccessibleGrouping || undefined}
accessibilityLabel={shouldDisableAccessibleGrouping ? contactAccessibilityLabel : undefined}
role={shouldDisableAccessibleGrouping ? CONST.ROLE.BUTTON : undefined}
style={[styles.flex1, styles.flexRow, styles.alignItemsCenter]}
>
{!shouldUseDefaultRightHandSideCheckmark && !!canSelectMultiple && (
<PressableWithFeedback
accessibilityLabel={item.text ?? ''}
Expand Down Expand Up @@ -194,7 +203,7 @@ function UserListItem<TItem extends ListItem>({
</View>
</PressableWithFeedback>
)}
</>
</View>
);
}}
</BaseListItem>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {useIsFocused} from '@react-navigation/native';
import {FlashList} from '@shopify/flash-list';
import type {FlashListRef, ListRenderItemInfo} from '@shopify/flash-list';
import React, {useImperativeHandle, useRef} from 'react';
import React, {useCallback, useImperativeHandle, useRef} from 'react';
import type {TextInputKeyPressEvent} from 'react-native';
import {View} from 'react-native';
import type {ValueOf} from 'type-fest';
Expand Down Expand Up @@ -101,23 +101,26 @@ function BaseSelectionListWithSections<TItem extends ListItem>({
hasKeyBeenPressed.current = true;
};

const scrollToIndex = (index: number) => {
if (index < 0 || index >= flattenedData.length || !listRef.current) {
return;
}
const item = flattenedData.at(index);
if (!item) {
return;
}
try {
listRef.current.scrollToIndex({index});
} catch (error) {
// FlashList may throw if layout for this index doesn't exist yet
// This can happen when data changes rapidly (e.g., during search filtering)
// The layout will be computed on next render, so we can safely ignore this
Log.warn('SelectionListWithSections: error scrolling to index', {error});
}
};
const scrollToIndex = useCallback(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CLEAN-REACT-PATTERNS-0 (docs)

React Compiler is enabled in this codebase and automatically memoizes closures based on their captured variables. Wrapping scrollToIndex in useCallback is redundant and may interfere with the compiler's optimization model.

Remove the useCallback wrapper and use a plain function:

const scrollToIndex = (index: number) => {
    if (index < 0 || index >= flattenedData.length || !listRef.current) {
        return;
    }
    // ...
};

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added previously when linter was providing a warning, but now it can be removed since compiler does memoize this function correctly.

(index: number) => {
if (index < 0 || index >= flattenedData.length || !listRef.current) {
return;
}
const item = flattenedData.at(index);
if (!item) {
return;
}
try {
listRef.current.scrollToIndex({index});
} catch (error) {
// FlashList may throw if layout for this index doesn't exist yet
// This can happen when data changes rapidly (e.g., during search filtering)
// The layout will be computed on next render, so we can safely ignore this
Log.warn('SelectionListWithSections: error scrolling to index', {error});
}
},
[flattenedData],
);

const debouncedScrollToIndex = useDebounce(scrollToIndex, CONST.TIMING.LIST_SCROLLING_DEBOUNCE_TIME, {leading: true, trailing: true});

Expand Down Expand Up @@ -188,6 +191,10 @@ function BaseSelectionListWithSections<TItem extends ListItem>({
innerTextInputRef.current?.focus();
};

const clearInputAfterSelect = () => {
textInputOptions?.onChangeText?.('');
};

const updateAndScrollToFocusedIndex = (index: number, shouldScroll = true) => {
if (!shouldScroll) {
suppressNextFocusScrollRef.current = true;
Expand All @@ -207,6 +214,8 @@ function BaseSelectionListWithSections<TItem extends ListItem>({

useImperativeHandle(ref, () => ({
focusTextInput,
scrollToIndex,
clearInputAfterSelect,
updateAndScrollToFocusedIndex,
updateExternalTextInputFocus,
getFocusedOption: getFocusedItem,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ type SelectionListWithSectionsProps<TItem extends ListItem> = BaseSelectionListP

type SelectionListWithSectionsHandle<TItem extends ListItem = ListItem> = {
focusTextInput: () => void;
scrollToIndex: (index: number) => void;
clearInputAfterSelect: () => void;
updateAndScrollToFocusedIndex: (index: number, shouldScroll?: boolean) => void;
updateExternalTextInputFocus: (isTextInputFocused: boolean) => void;
getFocusedOption: () => TItem | undefined;
Expand Down
19 changes: 17 additions & 2 deletions src/components/SelectionList/components/TextInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,22 @@ function TextInput({
}: TextInputProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const {label, value, onChangeText, errorText, headerMessage, hint, disableAutoFocus, placeholder, maxLength, inputMode, ref: optionsRef, style, disableAutoCorrect} = options ?? {};
const {
label,
value,
onChangeText,
errorText,
headerMessage,
hint,
disableAutoFocus,
placeholder,
maxLength,
inputMode,
ref: optionsRef,
style,
disableAutoCorrect,
shouldInterceptSwipe,
} = options ?? {};
const noResultsFoundText = translate('common.noResultsFound');
const isNoResultsFoundMessage = headerMessage === noResultsFoundText;
const noData = dataLength === 0 && !shouldShowLoadingPlaceholder;
Expand Down Expand Up @@ -139,8 +154,8 @@ function TextInput({
isLoading={isLoading}
testID="selection-list-text-input"
errorText={errorText}
shouldInterceptSwipe={false}
autoCorrect={!disableAutoCorrect}
shouldInterceptSwipe={shouldInterceptSwipe ?? false}
/>
</View>
{shouldShowHeaderMessage && (
Expand Down
Loading
Loading