Skip to content

Commit

Permalink
Refactor jump calculation
Browse files Browse the repository at this point in the history
  • Loading branch information
inokawa committed Feb 5, 2024
1 parent 56ef0f3 commit d5b2d24
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 59 deletions.
2 changes: 1 addition & 1 deletion e2e/VList.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion e2e/WindowVirtualizer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,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));
Expand Down
8 changes: 4 additions & 4 deletions src/core/cache.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,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,
Expand Down Expand Up @@ -715,7 +715,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,
Expand Down Expand Up @@ -750,7 +750,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,
Expand Down Expand Up @@ -798,7 +798,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,
Expand Down
6 changes: 3 additions & 3 deletions src/core/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ export const updateCacheLength = (
cache: Writeable<Cache>,
length: number,
isShift?: boolean
): [number, boolean] => {
): number => {
const diff = length - cache._length;

const isAdd = diff > 0;
Expand All @@ -177,7 +177,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) =>
Expand All @@ -192,5 +192,5 @@ export const updateCacheLength = (
-1
: min(length - 1, cache._computedOffsetIndex);
cache._length = length;
return [shift, isAdd];
return shift;
};
54 changes: 36 additions & 18 deletions src/core/scroller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
},
};
};
Expand Down Expand Up @@ -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...
Expand All @@ -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;
}
}
);
},
Expand Down Expand Up @@ -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();
},
};
};
Expand All @@ -346,6 +354,8 @@ export const createWindowScroller = (

return {
_observe(container) {
let prevStartOffset = 0;

const scrollOffsetKey = isHorizontal ? "scrollX" : "scrollY";

const document = getCurrentDocument(container);
Expand Down Expand Up @@ -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);
}
}
);
},
Expand Down
54 changes: 22 additions & 32 deletions src/core/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -187,7 +187,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;
Expand Down Expand Up @@ -284,15 +284,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) {
Expand Down Expand Up @@ -320,26 +320,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) {
Expand Down Expand Up @@ -380,16 +377,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]);
Expand Down

0 comments on commit d5b2d24

Please sign in to comment.