forked from akai/readest
fix(toc): prevent auto-scroll snap-back on sidebar open (#3900)
The TOC occasionally flashed a scroll to the current item and then
snapped back to the top, and on slow mobile first-opens sometimes
stayed at the top entirely.
Root cause: `useOverlayScrollbars({ defer: true })` schedules OS
construction via `requestIdleCallback` with a ~2233 ms timeout. On a
busy first open the timeout fires before the browser goes idle, so OS
wraps the viewport late — and the wrap step resets the scroller's
`scrollTop` synchronously, undoing Virtuoso's earlier scroll to the
current item. Virtuoso's `rangeChanged` / `onScroll` don't propagate
the reset for another frame, so any guard based on tracked scroll
state reads stale.
This commit is contained in:
@@ -41,6 +41,39 @@ const flattenTOC = (items: TOCItem[], expandedItems: Set<string>, depth = 0): TO
|
||||
return result;
|
||||
};
|
||||
|
||||
// Helpers mirrored from TOCView.tsx for the initial-scroll-target logic.
|
||||
// These drive the Virtuoso `initialTopMostItemIndex` prop, which avoids the
|
||||
// race where a setTimeout-based scrollToIndex fires before Virtuoso has
|
||||
// finished its first layout pass.
|
||||
const findParentPath = (items: TOCItem[], href: string, path: TOCItem[] = []): TOCItem[] => {
|
||||
for (const item of items) {
|
||||
const newPath = [...path, item];
|
||||
if (item.href === href) return path;
|
||||
if (item.subitems) {
|
||||
const found = findParentPath(item.subitems, href, newPath);
|
||||
if (found.length > 0) return found;
|
||||
}
|
||||
}
|
||||
return [];
|
||||
};
|
||||
|
||||
const computeExpandedSet = (toc: TOCItem[], href: string | undefined): Set<string> => {
|
||||
const topLevel = toc.filter((item) => item.subitems?.length).map(getItemIdentifier);
|
||||
const parents = href ? findParentPath(toc, href).map(getItemIdentifier).filter(Boolean) : [];
|
||||
return new Set([...topLevel, ...parents]);
|
||||
};
|
||||
|
||||
const getInitialScrollTarget = (
|
||||
toc: TOCItem[],
|
||||
href: string | undefined,
|
||||
): { index: number; expanded: Set<string> } => {
|
||||
const expanded = computeExpandedSet(toc, href);
|
||||
if (!href) return { index: 0, expanded };
|
||||
const flat = flattenTOC(toc, expanded);
|
||||
const idx = flat.findIndex((item) => item.href === href);
|
||||
return { index: idx > 0 ? idx : 0, expanded };
|
||||
};
|
||||
|
||||
describe('TOC sidebar initialization', () => {
|
||||
const nestedTOC: TOCItem[] = [
|
||||
{
|
||||
@@ -112,3 +145,62 @@ describe('TOC sidebar initialization', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Regression test for TOC auto-scroll race condition.
|
||||
*
|
||||
* When the TOC opens with an existing book progress, the view must scroll to
|
||||
* the current item. The previous implementation used a 300 ms setTimeout to
|
||||
* trigger `scrollToIndex` after mount. That races with Virtuoso's internal
|
||||
* layout stabilization: under load the timer occasionally fires first, the
|
||||
* TOC scrolls to the target, and then Virtuoso's late layout pass snaps the
|
||||
* list back to the top.
|
||||
*
|
||||
* Fix: compute the initial scroll target synchronously during mount so it
|
||||
* can be fed to Virtuoso's `initialTopMostItemIndex` prop, which Virtuoso
|
||||
* uses to perform the first scroll itself — no setTimeout race.
|
||||
*/
|
||||
describe('TOC initial scroll target', () => {
|
||||
const nestedTOC: TOCItem[] = [
|
||||
{
|
||||
id: 0,
|
||||
label: 'Book',
|
||||
href: undefined,
|
||||
subitems: [
|
||||
{ id: 1, label: 'Chapter 1', href: 'ch1.html' },
|
||||
{ id: 2, label: 'Chapter 2', href: 'ch2.html' },
|
||||
{ id: 3, label: 'Chapter 3', href: 'ch3.html' },
|
||||
],
|
||||
} as unknown as TOCItem,
|
||||
];
|
||||
|
||||
const flatTOC: TOCItem[] = [
|
||||
{ id: 1, label: 'Chapter 1', href: 'ch1.html' } as unknown as TOCItem,
|
||||
{ id: 2, label: 'Chapter 2', href: 'ch2.html' } as unknown as TOCItem,
|
||||
{ id: 3, label: 'Chapter 3', href: 'ch3.html' } as unknown as TOCItem,
|
||||
];
|
||||
|
||||
it('returns index 0 when no current href is provided', () => {
|
||||
const { index, expanded } = getInitialScrollTarget(nestedTOC, undefined);
|
||||
expect(index).toBe(0);
|
||||
// Top-level container is still expanded so the list renders its chapters.
|
||||
expect(expanded.size).toBe(1);
|
||||
});
|
||||
|
||||
it('resolves the current chapter inside a nested TOC with parents expanded', () => {
|
||||
const { index, expanded } = getInitialScrollTarget(nestedTOC, 'ch3.html');
|
||||
// flat order is Book, Ch1, Ch2, Ch3 → current chapter sits at index 3.
|
||||
expect(index).toBe(3);
|
||||
expect(expanded.has(getItemIdentifier(nestedTOC[0]!))).toBe(true);
|
||||
});
|
||||
|
||||
it('resolves the current chapter inside a flat TOC', () => {
|
||||
const { index } = getInitialScrollTarget(flatTOC, 'ch2.html');
|
||||
expect(index).toBe(1);
|
||||
});
|
||||
|
||||
it('falls back to index 0 when the href cannot be found', () => {
|
||||
const { index } = getInitialScrollTarget(nestedTOC, 'missing.html');
|
||||
expect(index).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -60,7 +60,10 @@ const Notebook: React.FC = ({}) => {
|
||||
panelHeight: notebookHeight,
|
||||
handleVerticalDragStart,
|
||||
} = useSwipeToDismiss(
|
||||
() => setNotebookVisible(false),
|
||||
() => {
|
||||
setNotebookVisible(false);
|
||||
setIsFullHeightInMobile(isMobile);
|
||||
},
|
||||
(data) => setIsFullHeightInMobile(data.clientY < 44),
|
||||
);
|
||||
|
||||
|
||||
@@ -76,7 +76,10 @@ const SideBar = ({}) => {
|
||||
panelHeight: sidebarHeight,
|
||||
handleVerticalDragStart,
|
||||
} = useSwipeToDismiss(
|
||||
() => setSideBarVisible(false),
|
||||
() => {
|
||||
setSideBarVisible(false);
|
||||
setIsFullHeightInMobile(isMobile);
|
||||
},
|
||||
(data) => setIsFullHeightInMobile(data.clientY < 44),
|
||||
);
|
||||
|
||||
|
||||
@@ -34,6 +34,23 @@ const computeExpandedSet = (toc: TOCItem[], href: string | undefined): Set<strin
|
||||
return new Set([...topLevel, ...parents]);
|
||||
};
|
||||
|
||||
const setsHaveSameContents = (a: Set<string>, b: Set<string>): boolean => {
|
||||
if (a.size !== b.size) return false;
|
||||
for (const item of a) if (!b.has(item)) return false;
|
||||
return true;
|
||||
};
|
||||
|
||||
const getInitialScrollTarget = (
|
||||
toc: TOCItem[],
|
||||
href: string | undefined,
|
||||
): { index: number; expanded: Set<string> } => {
|
||||
const expanded = computeExpandedSet(toc, href);
|
||||
if (!href) return { index: 0, expanded };
|
||||
const flat = flattenTOC(toc, expanded);
|
||||
const idx = flat.findIndex((f) => f.item.href === href);
|
||||
return { index: idx > 0 ? idx : 0, expanded };
|
||||
};
|
||||
|
||||
const TOCView: React.FC<{
|
||||
bookKey: string;
|
||||
toc: TOCItem[];
|
||||
@@ -43,9 +60,8 @@ const TOCView: React.FC<{
|
||||
const progress = getProgress(bookKey);
|
||||
const isEink = !!getViewSettings(bookKey)?.isEink;
|
||||
|
||||
const [expandedItems, setExpandedItems] = useState<Set<string>>(() =>
|
||||
computeExpandedSet(toc, progress?.sectionHref),
|
||||
);
|
||||
const [initialScrollTarget] = useState(() => getInitialScrollTarget(toc, progress?.sectionHref));
|
||||
const [expandedItems, setExpandedItems] = useState<Set<string>>(initialScrollTarget.expanded);
|
||||
const [containerHeight, setContainerHeight] = useState(400);
|
||||
|
||||
const containerRef = useRef<HTMLDivElement | null>(null);
|
||||
@@ -54,6 +70,7 @@ const TOCView: React.FC<{
|
||||
const scrollCooldownRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||
const pendingScrollRef = useRef(false);
|
||||
const visibleCenterRef = useRef(0);
|
||||
const initialScrollHandledRef = useRef(initialScrollTarget.index > 0);
|
||||
|
||||
// OverlayScrollbars + Virtuoso integration (same pattern as Bookshelf)
|
||||
const osRootRef = useRef<HTMLDivElement>(null);
|
||||
@@ -66,6 +83,16 @@ const TOCView: React.FC<{
|
||||
const { viewport } = instance.elements();
|
||||
viewport.style.overflowX = 'var(--os-viewport-overflow-x)';
|
||||
viewport.style.overflowY = 'var(--os-viewport-overflow-y)';
|
||||
const target = initialScrollTarget.index;
|
||||
if (target > 0) {
|
||||
requestAnimationFrame(() => {
|
||||
virtuosoRef.current?.scrollToIndex({
|
||||
index: target,
|
||||
align: 'center',
|
||||
behavior: 'auto',
|
||||
});
|
||||
});
|
||||
}
|
||||
},
|
||||
},
|
||||
});
|
||||
@@ -145,26 +172,32 @@ const TOCView: React.FC<{
|
||||
return;
|
||||
}
|
||||
if (userScrolledRef.current) return;
|
||||
setExpandedItems(computeExpandedSet(toc, progress?.sectionHref));
|
||||
if (progress?.sectionHref) pendingScrollRef.current = true;
|
||||
setExpandedItems((prev) => {
|
||||
const next = computeExpandedSet(toc, progress?.sectionHref);
|
||||
return setsHaveSameContents(prev, next) ? prev : next;
|
||||
});
|
||||
if (progress?.sectionHref) {
|
||||
if (initialScrollHandledRef.current) {
|
||||
initialScrollHandledRef.current = false;
|
||||
} else {
|
||||
pendingScrollRef.current = true;
|
||||
}
|
||||
}
|
||||
}, [isSideBarVisible, sideBarBookKey, bookKey, toc, progress]);
|
||||
|
||||
useEffect(() => {
|
||||
if (!pendingScrollRef.current || !activeHref || !isSideBarVisible) return;
|
||||
const timer = setTimeout(() => {
|
||||
const idx = flatItems.findIndex((f) => f.item.href === activeHref);
|
||||
if (idx !== -1) {
|
||||
// Eink displays ghost previous frames during smooth JS scroll
|
||||
// animations; force an instant jump to avoid the artifact. A CSS-only
|
||||
// fix is impossible because scrollTo({ behavior: 'smooth' }) overrides
|
||||
// CSS scroll-behavior and is not a CSS transition.
|
||||
const distance = Math.abs(idx - visibleCenterRef.current);
|
||||
const behavior = isEink || distance > 16 ? 'auto' : 'smooth';
|
||||
virtuosoRef.current?.scrollToIndex({ index: idx, align: 'center', behavior });
|
||||
}
|
||||
pendingScrollRef.current = false;
|
||||
}, 200);
|
||||
return () => clearTimeout(timer);
|
||||
const idx = flatItems.findIndex((f) => f.item.href === activeHref);
|
||||
if (idx !== -1) {
|
||||
// Eink displays ghost previous frames during smooth JS scroll
|
||||
// animations; force an instant jump to avoid the artifact. A CSS-only
|
||||
// fix is impossible because scrollTo({ behavior: 'smooth' }) overrides
|
||||
// CSS scroll-behavior and is not a CSS transition.
|
||||
const distance = Math.abs(idx - visibleCenterRef.current);
|
||||
const behavior = isEink || distance > 16 ? 'auto' : 'smooth';
|
||||
virtuosoRef.current?.scrollToIndex({ index: idx, align: 'center', behavior });
|
||||
}
|
||||
pendingScrollRef.current = false;
|
||||
}, [flatItems, activeHref, isSideBarVisible, isEink]);
|
||||
|
||||
return (
|
||||
@@ -173,6 +206,11 @@ const TOCView: React.FC<{
|
||||
<Virtuoso
|
||||
ref={virtuosoRef}
|
||||
scrollerRef={handleScrollerRef}
|
||||
initialTopMostItemIndex={
|
||||
initialScrollTarget.index > 0
|
||||
? { index: initialScrollTarget.index, align: 'center' }
|
||||
: 0
|
||||
}
|
||||
rangeChanged={({ startIndex, endIndex }) => {
|
||||
visibleCenterRef.current = Math.floor((startIndex + endIndex) / 2);
|
||||
}}
|
||||
|
||||
Reference in New Issue
Block a user