From 5811749daade79d24e05e95dc0b7a85865852f96 Mon Sep 17 00:00:00 2001 From: inokawa <48897392+inokawa@users.noreply.github.com> Date: Mon, 5 Feb 2024 01:51:48 +0900 Subject: [PATCH] Refactor jump calculation --- e2e/VList.spec.ts | 2 +- e2e/WindowVirtualizer.spec.ts | 2 +- src/core/cache.spec.ts | 8 +++--- src/core/cache.ts | 6 ++-- src/core/scroller.ts | 54 +++++++++++++++++++++++------------ src/core/store.ts | 54 ++++++++++++++--------------------- 6 files changed, 67 insertions(+), 59 deletions(-) diff --git a/e2e/VList.spec.ts b/e2e/VList.spec.ts index eaa5dba27..b0445d9a1 100644 --- a/e2e/VList.spec.ts +++ b/e2e/VList.spec.ts @@ -1048,7 +1048,7 @@ test.describe("check if item shift compensation works", () => { // Check if bottom is always visible and on bottom expectInRange(itemBottom, { min: -0.5, - max: browserName === "firefox" ? 0.6 : 0.1, + max: browserName === "firefox" ? 0.6 : 0.50001, }); if (!isScrollBarVisible) { diff --git a/e2e/WindowVirtualizer.spec.ts b/e2e/WindowVirtualizer.spec.ts index fdfd62fb9..28731c44b 100644 --- a/e2e/WindowVirtualizer.spec.ts +++ b/e2e/WindowVirtualizer.spec.ts @@ -383,7 +383,7 @@ test.describe("check if item shift compensation works", () => { // Check if bottom is always visible and on bottom expectInRange(itemBottom, { min: -0.5, - max: browserName === "firefox" ? 0.6 : 0.1, + max: browserName === "firefox" ? 0.6 : 0.50001, }); // may have subpixel error so scroll to bottom again await component.evaluate((e) => window.scrollBy(0, e.scrollHeight)); diff --git a/src/core/cache.spec.ts b/src/core/cache.spec.ts index fcf078ce9..bf3b9d856 100644 --- a/src/core/cache.spec.ts +++ b/src/core/cache.spec.ts @@ -786,7 +786,7 @@ describe(updateCacheLength.name, () => { it("should increase cache length", () => { const cache = initCache(10, 40); const res = updateCacheLength(cache, 15, undefined); - expect(res).toEqual([40 * 5, true]); + expect(res).toEqual(40 * 5); expect(cache).toMatchInlineSnapshot(` { "_computedOffsetIndex": -1, @@ -834,7 +834,7 @@ describe(updateCacheLength.name, () => { const cache = initCache(10, 40); cache._sizes[9] = 123; const res = updateCacheLength(cache, 5, undefined); - expect(res).toEqual([40 * 4 + 123, false]); + expect(res).toEqual(-(40 * 4 + 123)); expect(cache).toMatchInlineSnapshot(` { "_computedOffsetIndex": -1, @@ -869,7 +869,7 @@ describe(updateCacheLength.name, () => { it("should increase cache length with shifting", () => { const cache = initCache(10, 40); const res = updateCacheLength(cache, 15, true); - expect(res).toEqual([40 * 5, true]); + expect(res).toEqual(40 * 5); expect(cache).toMatchInlineSnapshot(` { "_computedOffsetIndex": -1, @@ -917,7 +917,7 @@ describe(updateCacheLength.name, () => { const cache = initCache(10, 40); cache._sizes[0] = 123; const res = updateCacheLength(cache, 5, true); - expect(res).toEqual([40 * 4 + 123, false]); + expect(res).toEqual(-(40 * 4 + 123)); expect(cache).toMatchInlineSnapshot(` { "_computedOffsetIndex": -1, diff --git a/src/core/cache.ts b/src/core/cache.ts index 7cffb43e9..4061329ab 100644 --- a/src/core/cache.ts +++ b/src/core/cache.ts @@ -178,7 +178,7 @@ export const updateCacheLength = ( cache: Writeable, length: number, isShift?: boolean -): [number, boolean] => { +): number => { const diff = length - cache._length; const isAdd = diff > 0; @@ -190,7 +190,7 @@ export const updateCacheLength = ( fill(cache._offsets, diff); } else { // Removed - shift = ( + shift = -( isShift ? cache._sizes.splice(0, -diff) : cache._sizes.splice(diff) ).reduce( (acc, removed) => @@ -205,5 +205,5 @@ export const updateCacheLength = ( -1 : min(length - 1, cache._computedOffsetIndex); cache._length = length; - return [shift, isAdd]; + return shift; }; diff --git a/src/core/scroller.ts b/src/core/scroller.ts index a219a7e4e..eb1277c14 100644 --- a/src/core/scroller.ts +++ b/src/core/scroller.ts @@ -38,7 +38,11 @@ const createScrollObserver = ( viewport: HTMLElement | Window, isHorizontal: boolean, getScrollOffset: () => number, - updateScrollOffset: (value: number, isMomentumScrolling: boolean) => void + updateScrollOffset: ( + value: number, + shift: boolean, + isMomentumScrolling: boolean + ) => void ) => { const now = Date.now; @@ -125,11 +129,14 @@ const createScrollObserver = ( onScrollEnd._cancel(); }, _fixScrollJump: () => { - const [jump, prepend] = store._flushJump(); + const [jump, shift] = store._flushJump(); if (!jump) return; - updateScrollOffset(jump, stillMomentumScrolling); + updateScrollOffset( + normalizeOffset(jump, isHorizontal), + shift, + stillMomentumScrolling + ); stillMomentumScrolling = false; - return prepend; }, }; }; @@ -253,7 +260,7 @@ export const createScroller = ( viewport, isHorizontal, () => normalizeOffset(viewport[scrollOffsetKey], isHorizontal), - (jump, isMomentumScrolling) => { + (jump, shift, isMomentumScrolling) => { // If we update scroll position while touching on iOS, the position will be reverted. // However iOS WebKit fires touch events only once at the beginning of momentum scrolling. // That means we have no reliable way to confirm still touched or not if user touches more than once during momentum scrolling... @@ -267,7 +274,13 @@ export const createScroller = ( }); } - viewport[scrollOffsetKey] += normalizeOffset(jump, isHorizontal); + if (shift) { + viewport[scrollOffsetKey] = store._getScrollOffset() + jump; + // https://github.com/inokawa/virtua/issues/357 + cancelScroll && cancelScroll(); + } else { + viewport[scrollOffsetKey] += jump; + } } ); }, @@ -316,12 +329,7 @@ export const createScroller = ( }, smooth); }, _fixScrollJump: () => { - if (scrollObserver) { - if (scrollObserver._fixScrollJump()) { - // https://github.com/inokawa/virtua/issues/357 - cancelScroll && cancelScroll(); - } - } + scrollObserver && scrollObserver._fixScrollJump(); }, }; }; @@ -346,6 +354,8 @@ export const createWindowScroller = ( return { _observe(container) { + let prevStartOffset = 0; + const scrollOffsetKey = isHorizontal ? "scrollX" : "scrollY"; const document = getCurrentDocument(container); @@ -385,13 +395,21 @@ export const createWindowScroller = ( isHorizontal, () => normalizeOffset(window[scrollOffsetKey], isHorizontal) - - calcOffsetToViewport(container, documentBody, isHorizontal), - (jump) => { + (prevStartOffset = calcOffsetToViewport( + container, + documentBody, + isHorizontal + )), + (jump, shift) => { // TODO support case two window scrollers exist in the same view - window.scrollBy( - isHorizontal ? normalizeOffset(jump, isHorizontal) : 0, - isHorizontal ? 0 : jump - ); + if (shift) { + window.scroll({ + [isHorizontal ? "left" : "top"]: + store._getScrollOffset() + prevStartOffset + jump, + }); + } else { + window.scrollBy(isHorizontal ? jump : 0, isHorizontal ? 0 : jump); + } } ); }, diff --git a/src/core/store.ts b/src/core/store.ts index c0d5e8937..6ef2f2aeb 100644 --- a/src/core/store.ts +++ b/src/core/store.ts @@ -31,11 +31,11 @@ export type ScrollDirection = const SCROLL_BY_NATIVE = 0; const SCROLL_BY_MANUAL_SCROLL = 1; -const SCROLL_BY_PREPENDING = 2; +const SCROLL_BY_SHIFT = 2; type ScrollMode = | typeof SCROLL_BY_NATIVE | typeof SCROLL_BY_MANUAL_SCROLL - | typeof SCROLL_BY_PREPENDING; + | typeof SCROLL_BY_SHIFT; /** @internal */ export const ACTION_ITEM_RESIZE = 1; @@ -169,7 +169,7 @@ export const createVirtualStore = ( let jumpCount = 0; let jump = 0; let pendingJump = 0; - let isJumpByPrepend = false; + let isJumpByShift = false; let _flushedJump = 0; let _scrollDirection: ScrollDirection = SCROLL_IDLE; let _scrollMode: ScrollMode = SCROLL_BY_NATIVE; @@ -264,15 +264,15 @@ export const createVirtualStore = ( }, _flushJump() { const flushedJump = jump; - const flushedIsJumpByPrepend = isJumpByPrepend; + const flushedIsJumpByShift = isJumpByShift; jump = 0; - isJumpByPrepend = false; + isJumpByShift = false; if (viewportSize > getScrollableSize()) { // In this case applying jump will not cause scroll. // Current logic expects scroll event occurs after applying jump so discard it. return [0, false]; } else { - return [(_flushedJump = flushedJump), flushedIsJumpByPrepend]; + return [(_flushedJump = flushedJump), flushedIsJumpByShift]; } }, _subscribe(target, cb) { @@ -300,26 +300,23 @@ export const createVirtualStore = ( // Calculate jump // Should maintain visible position to minimize junks in appearance - let diff = 0; - - if (scrollOffset === 0) { - // Do nothing to stick to the start - } else if (scrollOffset > getMaxScrollOffset() - SUBPIXEL_THRESHOLD) { - // Keep end to stick to the end - diff = calculateJump(cache, updated, true); - } else { - if (_scrollMode === SCROLL_BY_PREPENDING) { + let diff: number; + if (_scrollMode === SCROLL_BY_SHIFT) { + if (scrollOffset > getMaxScrollOffset() - SUBPIXEL_THRESHOLD) { + // Keep end to stick to the end + diff = calculateJump(cache, updated, true); + } else { // Keep distance from end immediately after prepending // We can assume jumps occurred on the upper outside diff = calculateJump(cache, updated); - } else { - // Keep start at mid - const [startIndex] = _prevRange; - diff = calculateJump( - cache, - updated.filter(([index]) => index < startIndex) - ); } + } else { + // Keep start at mid + const [startIndex] = _prevRange; + diff = calculateJump( + cache, + updated.filter(([index]) => index < startIndex) + ); } if (diff) { @@ -360,16 +357,9 @@ export const createVirtualStore = ( } case ACTION_ITEMS_LENGTH_CHANGE: { if (payload[1]) { - // Calc distance before updating cache - const distanceToEnd = getMaxScrollOffset() - scrollOffset; - - const [shift, isAdd] = updateCacheLength(cache, payload[0], true); - applyJump(isAdd ? shift : -min(shift, distanceToEnd)); - - if (isAdd) { - _scrollMode = SCROLL_BY_PREPENDING; - isJumpByPrepend = true; - } + applyJump(updateCacheLength(cache, payload[0], true)); + _scrollMode = SCROLL_BY_SHIFT; + isJumpByShift = true; mutated = UPDATE_SCROLL_STATE; } else { updateCacheLength(cache, payload[0]);