Fix: force FlashList to use natural DOM order on web #85825
+115
−2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| diff --git a/node_modules/@shopify/flash-list/dist/recyclerview/ViewHolderCollection.js b/node_modules/@shopify/flash-list/dist/recyclerview/ViewHolderCollection.js | ||
| index 8e3db51..702c59b 100644 | ||
| --- a/node_modules/@shopify/flash-list/dist/recyclerview/ViewHolderCollection.js | ||
| +++ b/node_modules/@shopify/flash-list/dist/recyclerview/ViewHolderCollection.js | ||
| @@ -3,10 +3,13 @@ | ||
| * It handles the rendering of a collection of list items, manages layout updates, | ||
| * and coordinates with the RecyclerView context for layout changes. | ||
| */ | ||
| -import React, { useEffect, useImperativeHandle, useLayoutEffect } from "react"; | ||
| +import React, { useCallback, useEffect, useImperativeHandle, useLayoutEffect, useRef, useState, } from "react"; | ||
| +import { Platform } from "react-native"; | ||
| import { ViewHolder } from "./ViewHolder"; | ||
| import { CompatView } from "./components/CompatView"; | ||
| import { useRecyclerViewContext } from "./RecyclerViewContextProvider"; | ||
| +const SORT_DELAY_MS = 1000; | ||
| +const TAB_SCROLL_THRESHOLD_MS = 400; | ||
| /** | ||
| * ViewHolderCollection component that manages the rendering of multiple ViewHolder instances | ||
| * and handles layout updates for the entire collection | ||
| @@ -72,9 +75,64 @@ export const ViewHolderCollection = (props) => { | ||
| // return `${index} => ${reactKey}`; | ||
| // }) | ||
| // ); | ||
| - return (React.createElement(CompatView, { style: hasData && containerStyle }, containerLayout && | ||
| + const containerRef = useRef(null); | ||
| + const lastFocusTimeRef = useRef(0); | ||
| + const renderEntriesRef = useRef(Array.from(renderStack.entries())); | ||
| + const [, setSortId] = useState(0); | ||
| + const doSort = useCallback(() => { | ||
| + const entries = renderEntriesRef.current; | ||
| + const isSorted = entries.every((entry, i) => i === 0 || entries[i - 1][1].index <= entry[1].index); | ||
| + if (isSorted) { | ||
| + return; | ||
| + } | ||
| + entries.sort(([, a], [, b]) => a.index - b.index); | ||
|
There was a problem hiding this comment.
The web reorder logic still hardcodes ascending index order ( Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. This was covered here already. |
||
| + setSortId((prev) => prev + 1); | ||
| + }, []); | ||
| + if (Platform.OS === "web") { | ||
| + // Reconcile: remove stale keys, append new keys | ||
| + const existingKeys = new Set(renderEntriesRef.current.map(([key]) => key)); | ||
| + renderEntriesRef.current = renderEntriesRef.current.filter(([key]) => renderStack.has(key)); | ||
| + for (const key of renderStack.keys()) { | ||
| + if (!existingKeys.has(key)) { | ||
| + renderEntriesRef.current.push([key, renderStack.get(key)]); | ||
| + } | ||
| + } | ||
| + } | ||
| + else { | ||
| + renderEntriesRef.current = Array.from(renderStack.entries()); | ||
| + } | ||
| + useEffect(() => { | ||
| + if (Platform.OS !== "web") { | ||
| + return; | ||
| + } | ||
| + const container = containerRef.current; | ||
| + if (!container) { | ||
| + return; | ||
| + } | ||
| + const onFocusIn = () => { | ||
| + lastFocusTimeRef.current = Date.now(); | ||
| + doSort(); | ||
| + }; | ||
sharabai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| + container.addEventListener("focusin", onFocusIn); | ||
| + return () => container.removeEventListener("focusin", onFocusIn); | ||
| + // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| + }, []); | ||
| + useEffect(() => { | ||
| + if (Platform.OS !== "web") { | ||
| + return; | ||
| + } | ||
| + const isRecentFocus = Date.now() - lastFocusTimeRef.current < TAB_SCROLL_THRESHOLD_MS; | ||
| + if (isRecentFocus) { | ||
| + doSort(); | ||
| + return; | ||
| + } | ||
| + const timeoutId = setTimeout(doSort, SORT_DELAY_MS); | ||
| + return () => clearTimeout(timeoutId); | ||
| + // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| + }, [renderStack, renderId]); | ||
| + return (React.createElement(CompatView, { ref: containerRef, style: hasData && containerStyle }, containerLayout && | ||
| hasData && | ||
| - Array.from(renderStack.entries(), ([reactKey, { index }]) => { | ||
| + renderEntriesRef.current.map(([reactKey, { index }]) => { | ||
| const item = data[index]; | ||
| // Suppress separators for items in the last row to prevent | ||
| // height mismatch. The last data item has no separator (no | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these specific numbers used?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VickyStash The approach is described in more detail in details.md, but for a quick overview - we sort the DOM nodes FlashList is using underneath. The
SORT_DELAY_MSis the delay before the we do the sort of the DOM nodes. It's needed exactly to avoid stale hover states that were reported here.the question then becomes how long do we wait before sorting?
Long enough so that hover states get their mouseleave event fired. With momentum scrolling on Macs for example you have to wait about that time so that the correct element gets the hover.
Example of a late hover (it took about 500ms after scroll completely stopped for hover to change)
late.hover.example.mp4
And it can happen to be even longer, so 1000ms was kind of a good spot to stop to avoid most of state hover states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VickyStash
The second
TAB_SCROLL_THRESHOLD_MSis used when user navigates using tabs or screen readers that focus elements. In that case we need to immediately sort the DOM nodes (because tabbing works on DOM node order in web).This var is used to control the following behavior: we are in the list, we are tabbing - tabbing normally means that we don't scroll, but when we reach an element that is outside of a viewport (browser sees we're focusing on an element that it outside of the FlashList's viewport), the list scrolls so that element is in the middle.
Normally we would defer sorting because it'd introduce late hovers, but since we need it ASAP for navigation, we don't defer scroll.
The
TAB_SCROLL_THRESHOLD_MSis used to answer the question: when have we last tabbed? If it was more recent than this threshold - do DOM sort immediately because we're likely causing scroll by browser focusing on an element outside of the viewport, meaning we're using tab navigation - so sort immediately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timing itself was chosen so that it works well enough under most conditions.