Merge pull request #541 from luissardon/fix/plugin-scroll-horizontal-smooth-scroll

bobsingor · web-flow · commit 352cc5f87858 · 2026-03-25T21:28:12.000+02:00
fix(plugin-scroll): horizontal layout extent, centering and programmatic scroll
diff --git a/.changeset/bright-birds-tie.md b/.changeset/bright-birds-tie.md
@@ -0,0 +1,7 @@
+---
+'@embedpdf/plugin-scroll': patch
+---
+
+Fix horizontal scrolling layout calculations and page navigation state updates in `@embedpdf/plugin-scroll`.
+
+This corrects horizontal visible-range and end-spacing math, preserves the optimistic `currentPage` during smooth next/previous navigation, and fixes page-coordinate targeting for mixed-height pages in horizontal mode by matching the scroller's vertical centering.
diff --git a/packages/plugin-scroll/src/lib/scroll-plugin.ts b/packages/plugin-scroll/src/lib/scroll-plugin.ts
@@ -435,7 +435,18 @@ export class ScrollPlugin extends BasePlugin<
       startTime: Date.now(),
     };
 
-    this.dispatch(updateDocumentScrollState(documentId, { pageChangeState }));
+    // Optimistic update: set currentPage immediately so UI reflects target during smooth scroll
+    this.dispatch(
+      updateDocumentScrollState(documentId, {
+        pageChangeState,
+        currentPage: targetPage,
+      }),
+    );
+    this.pageChange$.emit({
+      documentId,
+      pageNumber: targetPage,
+      totalPages: docState.totalPages,
+    });
 
     if (behavior === 'instant') {
       this.completePageChange(documentId);
@@ -494,14 +505,22 @@ export class ScrollPlugin extends BasePlugin<
     const docState = this.getDocumentState(documentId);
     if (!docState) return;
 
-    // Update state
-    this.dispatch(updateDocumentScrollState(documentId, metrics));
-
-    // Emit scroll event
-    this.scroll$.emit({ documentId, metrics });
-
-    // Emit page change if current page changed
-    if (metrics.currentPage !== docState.currentPage) {
+    // During smooth scroll (Next/Previous), viewport fires before scroll completes.
+    // Metrics would derive currentPage from old viewport position and revert the optimistic update.
+    const isProgrammaticScroll = docState.pageChangeState.isChanging;
+    // Preserve optimistic currentPage during programmatic scroll; use viewport-derived value otherwise
+    const metricsToCommit =
+      isProgrammaticScroll && metrics.currentPage !== docState.currentPage
+        ? { ...metrics, currentPage: docState.currentPage }
+        : metrics;
+
+    // Update state before emitting scroll event,
+    // so any scroll$ listener that reads state sees metricsToCommit; then notify
+    this.dispatch(updateDocumentScrollState(documentId, metricsToCommit));
+    this.scroll$.emit({ documentId, metrics: metricsToCommit });
+
+    // Only emit pageChange$ for user-driven scroll; programmatic scroll already emitted in startPageChange
+    if (!isProgrammaticScroll && metrics.currentPage !== docState.currentPage) {
       this.pageChange$.emit({
         documentId,
         pageNumber: metrics.currentPage,
diff --git a/packages/plugin-scroll/src/lib/strategies/base-strategy.ts b/packages/plugin-scroll/src/lib/strategies/base-strategy.ts
@@ -34,6 +34,31 @@ export abstract class BaseScrollStrategy {
   protected abstract getScrollOffset(viewport: ViewportMetrics): number;
   protected abstract getClientSize(viewport: ViewportMetrics): number;
 
+  /**
+   * Returns the item's extent along the scroll axis. Vertical layout uses height;
+   * horizontal layout uses width. Used for visible range and end spacing calculations.
+   */
+  protected abstract getItemSizeAlongScrollAxis(item: VirtualItem): number;
+
+  /**
+   * Horizontal centering offset for items narrower than the content max width.
+   * Vertical layout centers spreads within the row; horizontal layout overrides to 0
+   * since items stay in a simple row without centering.
+   */
+  protected getCenteringOffsetX(_item: VirtualItem, totalContentSize: Size | undefined): number {
+    if (!totalContentSize || _item.width >= totalContentSize.width) return 0;
+    return (totalContentSize.width - _item.width) / 2;
+  }
+
+  /**
+   * Vertical centering offset for items shorter than the content max height.
+   * Horizontal layout centers items within the row height; vertical layout keeps
+   * items top-aligned and uses the default of 0.
+   */
+  protected getCenteringOffsetY(_item: VirtualItem, _totalContentSize: Size | undefined): number {
+    return 0;
+  }
+
   protected getVisibleRange(
     viewport: ViewportMetrics,
     virtualItems: VirtualItem[],
@@ -44,10 +69,14 @@ export abstract class BaseScrollStrategy {
     const viewportStart = scrollOffset;
     const viewportEnd = scrollOffset + clientSize;
 
+    // Use extent along scroll axis (height for vertical, width for horizontal)
     let startIndex = 0;
     while (
       startIndex < virtualItems.length &&
-      (virtualItems[startIndex].offset + virtualItems[startIndex].height) * scale <= viewportStart
+      (virtualItems[startIndex].offset +
+        this.getItemSizeAlongScrollAxis(virtualItems[startIndex])) *
+        scale <=
+        viewportStart
     ) {
       startIndex++;
     }
@@ -75,7 +104,7 @@ export abstract class BaseScrollStrategy {
       visibleItems,
       viewport,
       scale,
-      totalContentSize.width,
+      totalContentSize,
     );
     const visiblePages = pageVisibilityMetrics.map((m) => m.pageNumber);
     const renderedPageIndexes = virtualItems
@@ -85,11 +114,11 @@ export abstract class BaseScrollStrategy {
     const first = virtualItems[range.start];
     const last = virtualItems[range.end];
     const startSpacing = first ? first.offset * scale : 0;
+    const lastItem = virtualItems[virtualItems.length - 1];
+    // End spacing = distance from last rendered item to end of content (uses extent along scroll axis)
     const endSpacing = last
-      ? (virtualItems[virtualItems.length - 1].offset + // end of content
-          virtualItems[virtualItems.length - 1].height) *
-          scale - // minus
-        (last.offset + last.height) * scale // end of last rendered
+      ? (lastItem.offset + this.getItemSizeAlongScrollAxis(lastItem)) * scale -
+        (last.offset + this.getItemSizeAlongScrollAxis(last)) * scale
       : 0;
 
     return {
@@ -107,19 +136,17 @@ export abstract class BaseScrollStrategy {
     virtualItems: VirtualItem[],
     viewport: ViewportMetrics,
     scale: number,
-    contentWidth?: number,
+    totalContentSize?: Size,
   ): ScrollMetrics['pageVisibilityMetrics'] {
     const visibilityMetrics: ScrollMetrics['pageVisibilityMetrics'] = [];
-    // Calculate max width for centering if not provided
-    const maxWidth = contentWidth ?? Math.max(...virtualItems.map((i) => i.width));
 
     virtualItems.forEach((item) => {
-      // Calculate horizontal centering offset for items narrower than max width
-      const centeringOffsetX = item.width < maxWidth ? (maxWidth - item.width) / 2 : 0;
+      const centeringOffsetX = this.getCenteringOffsetX(item, totalContentSize);
+      const centeringOffsetY = this.getCenteringOffsetY(item, totalContentSize);
 
       item.pageLayouts.forEach((page) => {
         const itemX = (item.x + centeringOffsetX) * scale;
-        const itemY = item.y * scale;
+        const itemY = (item.y + centeringOffsetY) * scale;
         const pageX = itemX + page.x * scale;
         const pageY = itemY + page.y * scale;
         const pageWidth = page.rotatedWidth * scale;
@@ -194,19 +221,13 @@ export abstract class BaseScrollStrategy {
     const pageLayout = item.pageLayouts.find((layout) => layout.pageNumber === pageNumber);
     if (!pageLayout) return null;
 
-    // Calculate centering offset for items that are narrower than the maximum width
-    let centeringOffsetX = 0;
-    if (totalContentSize) {
-      const maxWidth = totalContentSize.width;
-      if (item.width < maxWidth) {
-        centeringOffsetX = (maxWidth - item.width) / 2;
-      }
-    }
+    const centeringOffsetX = this.getCenteringOffsetX(item, totalContentSize);
+    const centeringOffsetY = this.getCenteringOffsetY(item, totalContentSize);
 
     return {
       origin: {
         x: item.x + pageLayout.x + centeringOffsetX,
-        y: item.y + pageLayout.y,
+        y: item.y + pageLayout.y + centeringOffsetY,
       },
       size: {
         width: pageLayout.width,
diff --git a/packages/plugin-scroll/src/lib/strategies/horizontal-strategy.ts b/packages/plugin-scroll/src/lib/strategies/horizontal-strategy.ts
@@ -1,4 +1,5 @@
 import { PdfPageObjectWithRotatedSize } from '@embedpdf/models';
+import { Size } from '@embedpdf/models';
 import { ViewportMetrics } from '@embedpdf/plugin-viewport';
 import { BaseScrollStrategy, ScrollStrategyConfig } from './base-strategy';
 import { VirtualItem, PageLayout } from '../types/virtual-item';
@@ -22,6 +23,7 @@ export class HorizontalScrollStrategy extends BaseScrollStrategy {
           height: page.size.height,
           rotatedWidth: page.rotatedSize.width,
           rotatedHeight: page.rotatedSize.height,
+          elevated: false,
         };
         pageX += page.rotatedSize.width + this.pageGap;
         return layout;
@@ -66,4 +68,27 @@ export class HorizontalScrollStrategy extends BaseScrollStrategy {
   protected getClientSize(viewport: ViewportMetrics): number {
     return viewport.clientWidth;
   }
+
+  /** Horizontal scroll: extent along scroll axis is width. */
+  protected getItemSizeAlongScrollAxis(item: VirtualItem): number {
+    return item.width;
+  }
+
+  /**
+   * No centering for horizontal layout. Items are laid out in a simple row;
+   * using total content width would produce incorrect offsets and shift pages.
+   */
+  /* eslint-disable no-unused-vars -- override intentionally ignores params */
+  protected getCenteringOffsetX(_item: VirtualItem, _totalContentSize: Size | undefined): number {
+    return 0;
+  }
+
+  /**
+   * Horizontal rows visually center shorter items within the tallest row height.
+   * Match that DOM layout so page-coordinate targeting lands on the rendered page.
+   */
+  protected getCenteringOffsetY(item: VirtualItem, totalContentSize: Size | undefined): number {
+    if (!totalContentSize || item.height >= totalContentSize.height) return 0;
+    return (totalContentSize.height - item.height) / 2;
+  }
 }
diff --git a/packages/plugin-scroll/src/lib/strategies/vertical-strategy.ts b/packages/plugin-scroll/src/lib/strategies/vertical-strategy.ts
@@ -2,7 +2,6 @@ import { PdfPageObjectWithRotatedSize } from '@embedpdf/models';
 import { ViewportMetrics } from '@embedpdf/plugin-viewport';
 import { BaseScrollStrategy, ScrollStrategyConfig } from './base-strategy';
 import { VirtualItem, PageLayout } from '../types/virtual-item';
-import { ScrollMetrics } from '../types';
 
 export class VerticalScrollStrategy extends BaseScrollStrategy {
   constructor(config: ScrollStrategyConfig) {
@@ -23,6 +22,7 @@ export class VerticalScrollStrategy extends BaseScrollStrategy {
           height: page.size.height,
           rotatedWidth: page.rotatedSize.width,
           rotatedHeight: page.rotatedSize.height,
+          elevated: false,
         };
         pageX += page.rotatedSize.width + this.pageGap;
         return layout;
@@ -67,4 +67,9 @@ export class VerticalScrollStrategy extends BaseScrollStrategy {
   protected getClientSize(viewport: ViewportMetrics): number {
     return viewport.clientHeight;
   }
+
+  /** Vertical scroll: extent along scroll axis is height. */
+  protected getItemSizeAlongScrollAxis(item: VirtualItem): number {
+    return item.height;
+  }
 }
