From 005bc8ab4465e76eb584362b3482488e0ddcc741 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sat, 21 Mar 2026 08:38:32 -0700 Subject: [PATCH] fix(page-controller): apply scroll direction to pixels parameter Two bugs in the scroll direction logic: 1. Vertical scroll with `pixels` ignores the `down` boolean because the `??` operator bypasses the direction multiplier when pixels is provided. Fix: move the direction multiplier outside the `??` so it applies to both the pixels and numPages paths. 2. Horizontal scroll with `pixels` applies direction twice - once in PageController.ts and again in actions.ts - causing a double negation that reverses the intended direction. Fix: remove the redundant direction logic from actions.ts since PageController already signs the scroll amount. Also removes the now-unused `down` and `right` parameters from the scrollVertically() and scrollHorizontally() action functions. Co-Authored-By: Claude Opus 4.6 --- packages/page-controller/src/PageController.ts | 6 +++--- packages/page-controller/src/actions.ts | 16 ++++------------ 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/packages/page-controller/src/PageController.ts b/packages/page-controller/src/PageController.ts index 2f56a73..ac83b15 100644 --- a/packages/page-controller/src/PageController.ts +++ b/packages/page-controller/src/PageController.ts @@ -321,11 +321,11 @@ export class PageController extends EventTarget { this.assertIndexed() - const scrollAmount = pixels ?? numPages * (down ? 1 : -1) * window.innerHeight + const scrollAmount = (pixels ?? numPages * window.innerHeight) * (down ? 1 : -1) const element = index !== undefined ? getElementByIndex(this.selectorMap, index) : null - const message = await scrollVertically(down, scrollAmount, element) + const message = await scrollVertically(scrollAmount, element) return { success: true, @@ -356,7 +356,7 @@ export class PageController extends EventTarget { const element = index !== undefined ? getElementByIndex(this.selectorMap, index) : null - const message = await scrollHorizontally(right, scrollAmount, element) + const message = await scrollHorizontally(scrollAmount, element) return { success: true, diff --git a/packages/page-controller/src/actions.ts b/packages/page-controller/src/actions.ts index 92bb2b4..dcf6ab4 100644 --- a/packages/page-controller/src/actions.ts +++ b/packages/page-controller/src/actions.ts @@ -231,11 +231,7 @@ export async function scrollIntoViewIfNeeded(element: Element) { } } -export async function scrollVertically( - down: boolean, - scroll_amount: number, - element?: HTMLElement | null -) { +export async function scrollVertically(scroll_amount: number, element?: HTMLElement | null) { // Element-specific scrolling if element is provided if (element) { const targetElement = element @@ -359,11 +355,7 @@ export async function scrollVertically( } } -export async function scrollHorizontally( - right: boolean, - scroll_amount: number, - element?: HTMLElement | null -) { +export async function scrollHorizontally(scroll_amount: number, element?: HTMLElement | null) { // Element-specific scrolling if element is provided if (element) { const targetElement = element @@ -372,7 +364,7 @@ export async function scrollHorizontally( let scrolledElement: HTMLElement | null = null let scrollDelta = 0 let attempts = 0 - const dx = right ? scroll_amount : -scroll_amount + const dx = scroll_amount while (currentElement && attempts < 10) { const computedStyle = window.getComputedStyle(currentElement) @@ -420,7 +412,7 @@ export async function scrollHorizontally( // Page-level scrolling (default or fallback) - const dx = right ? scroll_amount : -scroll_amount + const dx = scroll_amount const bigEnough = (el: HTMLElement) => el.clientWidth >= window.innerWidth * 0.5 const canScroll = (el: HTMLElement | null) => el &&