From 7fc8e17b9e1c38e55a027e9054d0a2cebb8098c1 Mon Sep 17 00:00:00 2001 From: Andrei Zhaleznichenka Date: Mon, 25 Aug 2025 18:10:47 +0200 Subject: [PATCH 1/2] fix: Fixes auto-scroll releasing when transation commits --- src/board/internal.tsx | 6 +- src/internal/utils/use-auto-scroll.ts | 130 ++++++++++-------- .../board-layout/regressions.test.ts | 36 +++++ 3 files changed, 108 insertions(+), 64 deletions(-) create mode 100644 test/functional/board-layout/regressions.test.ts diff --git a/src/board/internal.tsx b/src/board/internal.tsx index 7e809b22..a41f697e 100644 --- a/src/board/internal.tsx +++ b/src/board/internal.tsx @@ -155,7 +155,7 @@ export function InternalBoard({ collisionIds: interactionType === "pointer" && isElementOverBoard(collisionRect) ? collisionIds : [], }); - autoScrollHandlers.addPointerEventHandlers(); + autoScrollHandlers.run(); }); useDragSubscription("update", ({ interactionType, collisionIds, positionOffset, collisionRect }) => { @@ -170,7 +170,7 @@ export function InternalBoard({ useDragSubscription("submit", () => { dispatch({ type: "submit" }); - autoScrollHandlers.removePointerEventHandlers(); + autoScrollHandlers.stop(); if (!transition) { throw new Error("Invariant violation: no transition."); @@ -196,7 +196,7 @@ export function InternalBoard({ useDragSubscription("discard", () => { dispatch({ type: "discard" }); - autoScrollHandlers.removePointerEventHandlers(); + autoScrollHandlers.stop(); }); useDragSubscription("acquire", ({ droppableId, draggableItem, renderAcquiredItem }) => { diff --git a/src/internal/utils/use-auto-scroll.ts b/src/internal/utils/use-auto-scroll.ts index edde3eb5..ccc1d5a0 100644 --- a/src/internal/utils/use-auto-scroll.ts +++ b/src/internal/utils/use-auto-scroll.ts @@ -1,80 +1,88 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import { useCallback, useEffect, useRef, useState } from "react"; +import { useEffect, useRef } from "react"; import { useLastInteraction } from "./use-last-interaction"; +const AUTO_SCROLL_INCREMENT = 5; +const AUTO_SCROLL_MARGIN = 50; +const AUTO_SCROLL_DELAY = 10; export function useAutoScroll() { - const [activeAutoScroll, setActiveAutoScroll] = useState<"up" | "down" | "none">("none"); - const scrollIntoViewTimerRef = useRef>(null); const getLastInteraction = useLastInteraction(); + const scrollControllerRef = useRef(new AutoScrollController(getLastInteraction)); + useEffect(() => scrollControllerRef.current.init(), []); + return scrollControllerRef.current; +} - // Scroll window repeatedly if activeAutoScroll="up" or activeAutoScroll="down". - useEffect(() => { - if (activeAutoScroll === "none") { - return; - } - const direction = activeAutoScroll === "up" ? -1 : 1; +class AutoScrollController { + private getLastInteraction: () => "pointer" | "keyboard"; + private active = false; + private direction: 0 | -1 | 1 = 0; + private timeout = setTimeout(() => {}, 0); - let timer: ReturnType; + constructor(getLastInteraction: () => "pointer" | "keyboard") { + this.getLastInteraction = getLastInteraction; + } - function scrollLoop() { - timer = setTimeout(() => { - window.scrollBy({ top: direction * 5 }); - scrollLoop(); - }, 10); - } - scrollLoop(); + public init() { + this.scrollRepeat(); + window.addEventListener("pointermove", this.onPointerMove); + window.addEventListener("pointerup", this.onPointerUp); + return () => { + clearTimeout(this.timeout); + window.removeEventListener("pointermove", this.onPointerMove); + window.removeEventListener("pointerup", this.onPointerUp); + }; + } - return () => clearTimeout(timer); - }, [activeAutoScroll]); + public run() { + this.active = true; + } - const onPointerMove = useCallback((event: PointerEvent) => { - const autoScrollMargin = 50; - if (event.clientY > window.innerHeight - autoScrollMargin) { - setActiveAutoScroll("down"); - } else if (event.clientY < autoScrollMargin) { - setActiveAutoScroll("up"); - } else { - setActiveAutoScroll("none"); - } - }, []); + public stop() { + this.active = false; + } - const onPointerUp = useCallback(() => { - setActiveAutoScroll("none"); - }, []); + public scheduleActiveElementScrollIntoView(delay: number) { + clearTimeout(this.timeout); - const addPointerEventHandlers = useCallback(() => { - if (getLastInteraction() === "pointer") { - window.addEventListener("pointermove", onPointerMove); - window.addEventListener("pointerup", onPointerUp); - } - }, [getLastInteraction, onPointerMove, onPointerUp]); + const activeElementBeforeDelay = document.activeElement; + this.timeout = setTimeout(() => { + if ( + document.activeElement && + document.activeElement === activeElementBeforeDelay && + this.getLastInteraction() === "keyboard" + ) { + document.activeElement.scrollIntoView?.({ behavior: "smooth", block: "nearest" }); + } + this.scrollRepeat(); + }, delay); + } - const removePointerEventHandlers = useCallback(() => { - window.removeEventListener("pointermove", onPointerMove); - window.removeEventListener("pointerup", onPointerUp); - }, [onPointerMove, onPointerUp]); - - const scheduleActiveElementScrollIntoView = useCallback( - (delay: number) => { - scrollIntoViewTimerRef.current && clearTimeout(scrollIntoViewTimerRef.current); - - const activeElementBeforeDelay = document.activeElement; + private onPointerMove = (event: PointerEvent) => { + if (!this.active) { + return; + } + if (event.clientY > window.innerHeight - AUTO_SCROLL_MARGIN) { + this.direction = 1; + } else if (event.clientY < AUTO_SCROLL_MARGIN) { + this.direction = -1; + } else { + this.direction = 0; + } + }; - scrollIntoViewTimerRef.current = setTimeout(() => { - if ( - document.activeElement && - document.activeElement === activeElementBeforeDelay && - getLastInteraction() === "keyboard" - ) { - document.activeElement.scrollIntoView?.({ behavior: "smooth", block: "nearest" }); - } - }, delay); - }, - [getLastInteraction], - ); + private onPointerUp = () => { + this.direction = 0; + }; - return { addPointerEventHandlers, removePointerEventHandlers, scheduleActiveElementScrollIntoView }; + private scrollRepeat() { + this.timeout = setTimeout(() => { + if (this.active && this.direction !== 0) { + window.scrollBy({ top: this.direction * AUTO_SCROLL_INCREMENT }); + } + this.scrollRepeat(); + }, AUTO_SCROLL_DELAY); + } } diff --git a/test/functional/board-layout/regressions.test.ts b/test/functional/board-layout/regressions.test.ts new file mode 100644 index 00000000..e4ae6a60 --- /dev/null +++ b/test/functional/board-layout/regressions.test.ts @@ -0,0 +1,36 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { expect, test } from "vitest"; + +import createWrapper from "../../../lib/components/test-utils/selectors"; +import { setupTest } from "../../utils"; +import { DndPageObject } from "./dnd-page-object"; + +const boardWrapper = createWrapper().findBoard(); + +test( + "should stop autoscroll upon receiving pointer-up", + setupTest("/index.html#/dnd/engine-a2h-test", DndPageObject, async (page) => { + await page.mouseDown(boardWrapper.findItemById("G").findResizeHandle().toSelector()); + + const scroll1 = await page.getWindowScroll(); + expect(scroll1.top).toBe(0); + + // This should cancel the operation. + await page.mouseMove(0, 255); + await page.pause(25); + await page.mouseUp(); + + const scroll2 = await page.getWindowScroll(); + expect(scroll2.top).toBeGreaterThan(100); + + await page.pause(25); + const scroll3 = await page.getWindowScroll(); + expect(scroll3.top).toBe(scroll2.top); + + await page.pause(25); + const scroll4 = await page.getWindowScroll(); + expect(scroll4.top).toBe(scroll3.top); + }), +); From b9c9f45c5a5fbf54bf6bd275ceee27a83e2b3149 Mon Sep 17 00:00:00 2001 From: Andrei Zhaleznichenka Date: Mon, 25 Aug 2025 18:34:03 +0200 Subject: [PATCH 2/2] tests cov --- .../utils/__tests__/use-auto-scroll.test.tsx | 339 ++++++++++++++++++ 1 file changed, 339 insertions(+) create mode 100644 src/internal/utils/__tests__/use-auto-scroll.test.tsx diff --git a/src/internal/utils/__tests__/use-auto-scroll.test.tsx b/src/internal/utils/__tests__/use-auto-scroll.test.tsx new file mode 100644 index 00000000..ec65aca0 --- /dev/null +++ b/src/internal/utils/__tests__/use-auto-scroll.test.tsx @@ -0,0 +1,339 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, type MockedFunction, vi } from "vitest"; + +import { useAutoScroll } from "../use-auto-scroll"; +import { useLastInteraction } from "../use-last-interaction"; + +vi.mock("../use-last-interaction", () => ({ useLastInteraction: vi.fn() })); + +const mockedUseLastInteraction = vi.mocked(useLastInteraction); + +describe("useAutoScroll", () => { + let mockGetLastInteraction: MockedFunction<() => "pointer" | "keyboard">; + let mockScrollIntoView: MockedFunction<(options?: ScrollIntoViewOptions) => void>; + + beforeEach(() => { + vi.clearAllMocks(); + vi.useFakeTimers(); + + mockGetLastInteraction = vi.fn().mockReturnValue("pointer"); + mockedUseLastInteraction.mockReturnValue(mockGetLastInteraction); + + vi.spyOn(window, "addEventListener").mockImplementation(() => {}); + vi.spyOn(window, "removeEventListener").mockImplementation(() => {}); + vi.spyOn(window, "scrollBy").mockImplementation(() => {}); + vi.spyOn(global, "setTimeout"); + vi.spyOn(global, "clearTimeout"); + + mockScrollIntoView = vi.fn(); + + // Mock window dimensions + Object.defineProperty(window, "innerHeight", { + writable: true, + configurable: true, + value: 600, + }); + + Object.defineProperty(document, "activeElement", { + writable: true, + configurable: true, + value: { + scrollIntoView: mockScrollIntoView, + }, + }); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it("should set up event listeners on init and return cleanup function", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + const cleanup = controller.init(); + + expect(window.addEventListener).toHaveBeenCalledWith("pointermove", expect.any(Function)); + expect(window.addEventListener).toHaveBeenCalledWith("pointerup", expect.any(Function)); + + cleanup(); + + expect(clearTimeout).toHaveBeenCalled(); + expect(window.removeEventListener).toHaveBeenCalledWith("pointermove", expect.any(Function)); + expect(window.removeEventListener).toHaveBeenCalledWith("pointerup", expect.any(Function)); + }); + + it("should start auto-scrolling when run() is called and pointer is in bottom margin", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + controller.run(); + + // Simulate pointer move to bottom margin (clientY > innerHeight - 50) + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + + pointerMoveHandler?.({ clientY: 560 } as PointerEvent); // 560 > 550 (600 - 50) + + // Fast-forward timer to trigger scroll + vi.advanceTimersByTime(10); + + expect(window.scrollBy).toHaveBeenCalledWith({ top: 5 }); // direction=1, increment=5 + }); + + it("should start auto-scrolling upward when pointer is in top margin", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + controller.run(); + + // Simulate pointer move to top margin (clientY < 50) + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + + pointerMoveHandler?.({ clientY: 30 } as PointerEvent); // 30 < 50 + + // Fast-forward timer to trigger scroll + vi.advanceTimersByTime(10); + + expect(window.scrollBy).toHaveBeenCalledWith({ top: -5 }); // direction=-1, increment=5 + }); + + it("should stop auto-scrolling when pointer is in middle area", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + controller.run(); + + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + + // First, move to bottom margin to start scrolling + pointerMoveHandler?.({ clientY: 560 } as PointerEvent); + vi.advanceTimersByTime(10); + expect(window.scrollBy).toHaveBeenCalledWith({ top: 5 }); + + // Then move to middle area + vi.mocked(window.scrollBy).mockClear(); + pointerMoveHandler?.({ clientY: 300 } as PointerEvent); // Middle of screen + + // Fast-forward timer - should not scroll anymore + vi.advanceTimersByTime(10); + expect(window.scrollBy).not.toHaveBeenCalled(); + }); + + it("should stop auto-scrolling when stop() is called", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + controller.run(); + + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + + // Move to bottom margin + pointerMoveHandler?.({ clientY: 560 } as PointerEvent); + + // Stop the controller + controller.stop(); + + // Fast-forward timer - should not scroll + vi.advanceTimersByTime(10); + expect(window.scrollBy).not.toHaveBeenCalled(); + }); + + it("should reset scroll direction on pointer up", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + controller.run(); + + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + const pointerUpHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointerup")?.[1] as (() => void) | undefined; + + // Move to bottom margin to start scrolling + pointerMoveHandler?.({ clientY: 560 } as PointerEvent); + vi.advanceTimersByTime(10); + expect(window.scrollBy).toHaveBeenCalledWith({ top: 5 }); + + // Trigger pointer up + vi.mocked(window.scrollBy).mockClear(); + pointerUpHandler?.(); + + // Fast-forward timer - should not scroll anymore + vi.advanceTimersByTime(10); + expect(window.scrollBy).not.toHaveBeenCalled(); + }); + + it("should not respond to pointer events when not active", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + // Don't call run() - controller is not active + + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + + // Move to bottom margin + pointerMoveHandler?.({ clientY: 560 } as PointerEvent); + + // Fast-forward timer - should not scroll + vi.advanceTimersByTime(10); + expect(window.scrollBy).not.toHaveBeenCalled(); + }); + + it("should schedule active element scroll into view for keyboard interactions", () => { + mockGetLastInteraction.mockReturnValue("keyboard"); + + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + const activeElement = { scrollIntoView: mockScrollIntoView }; + Object.defineProperty(document, "activeElement", { + value: activeElement, + configurable: true, + }); + + controller.scheduleActiveElementScrollIntoView(100); + + // Fast-forward to after the delay + vi.advanceTimersByTime(100); + + expect(mockScrollIntoView).toHaveBeenCalledWith({ + behavior: "smooth", + block: "nearest", + }); + }); + + it("should not scroll active element into view for pointer interactions", () => { + mockGetLastInteraction.mockReturnValue("pointer"); + + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + const activeElement = { scrollIntoView: mockScrollIntoView }; + Object.defineProperty(document, "activeElement", { + value: activeElement, + configurable: true, + }); + + controller.scheduleActiveElementScrollIntoView(100); + + // Fast-forward to after the delay + vi.advanceTimersByTime(100); + + expect(mockScrollIntoView).not.toHaveBeenCalled(); + }); + + it("should not scroll into view if active element changes during delay", () => { + mockGetLastInteraction.mockReturnValue("keyboard"); + + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + const originalActiveElement = { scrollIntoView: mockScrollIntoView }; + Object.defineProperty(document, "activeElement", { + value: originalActiveElement, + configurable: true, + }); + + controller.scheduleActiveElementScrollIntoView(100); + + // Change active element before delay completes + const newActiveElement = { scrollIntoView: vi.fn() }; + Object.defineProperty(document, "activeElement", { + value: newActiveElement, + configurable: true, + }); + + // Fast-forward to after the delay + vi.advanceTimersByTime(100); + + expect(mockScrollIntoView).not.toHaveBeenCalled(); + }); + + it("should not scroll into view if no active element", () => { + mockGetLastInteraction.mockReturnValue("keyboard"); + + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + Object.defineProperty(document, "activeElement", { + value: null, + configurable: true, + }); + + controller.scheduleActiveElementScrollIntoView(100); + + // Fast-forward to after the delay + vi.advanceTimersByTime(100); + + expect(mockScrollIntoView).not.toHaveBeenCalled(); + }); + + it("should clear previous timeout when scheduling new active element scroll", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + // Schedule first scroll + controller.scheduleActiveElementScrollIntoView(100); + const firstTimeoutId = vi.mocked(setTimeout).mock.results[vi.mocked(setTimeout).mock.results.length - 1]?.value; + + // Schedule second scroll before first completes + controller.scheduleActiveElementScrollIntoView(200); + + expect(clearTimeout).toHaveBeenCalledWith(firstTimeoutId); + }); + + it("should continue scrolling repeatedly while active and in margin", () => { + const { result } = renderHook(() => useAutoScroll()); + const controller = result.current; + + controller.init(); + controller.run(); + + const pointerMoveHandler = vi + .mocked(window.addEventListener) + .mock.calls.find((call) => call[0] === "pointermove")?.[1] as ((event: PointerEvent) => void) | undefined; + + // Move to bottom margin + pointerMoveHandler?.({ clientY: 560 } as PointerEvent); + + // Advance timer once to get the first scroll call + vi.advanceTimersByTime(10); + + // Verify it's scrolling with the correct parameters + expect(window.scrollBy).toHaveBeenCalledWith({ top: 5 }); + const initialCallCount = vi.mocked(window.scrollBy).mock.calls.length; + + // Advance timer again to verify it continues scrolling + vi.advanceTimersByTime(10); + expect(vi.mocked(window.scrollBy).mock.calls.length).toBeGreaterThan(initialCallCount); + + // Advance timer once more to verify it's still scrolling + vi.advanceTimersByTime(10); + expect(vi.mocked(window.scrollBy).mock.calls.length).toBeGreaterThan(initialCallCount + 1); + + // All calls should be the same + expect(window.scrollBy).toHaveBeenCalledWith({ top: 5 }); + }); +});