From db15821d37fb1f44842666a3dcb04111b9c9a2d2 Mon Sep 17 00:00:00 2001 From: EthanHealy01 Date: Thu, 6 Nov 2025 23:57:25 +0000 Subject: [PATCH] improve the click to scroll --- .../tools/compare/CompareWorkbenchView.tsx | 19 +- .../hooks/useCompareChangeNavigation.ts | 187 ++++++++++++------ 2 files changed, 146 insertions(+), 60 deletions(-) diff --git a/frontend/src/core/components/tools/compare/CompareWorkbenchView.tsx b/frontend/src/core/components/tools/compare/CompareWorkbenchView.tsx index ae24dee5b..7cd200dae 100644 --- a/frontend/src/core/components/tools/compare/CompareWorkbenchView.tsx +++ b/frontend/src/core/components/tools/compare/CompareWorkbenchView.tsx @@ -101,9 +101,26 @@ const CompareWorkbenchView = ({ data }: CompareWorkbenchViewProps) => { getRowHeightPx, } = useCompareHighlights(result, basePages, comparisonPages); + const temporarilySuppressScrollLink = useCallback((fn: () => void, durationMs = 700) => { + const wasLinked = isScrollLinked; + if (wasLinked) setIsScrollLinked(false); + try { + fn(); + } finally { + window.setTimeout(() => { + if (wasLinked) { + // recapture anchors to keep panes aligned when relinking + captureScrollLinkDelta(); + setIsScrollLinked(true); + } + }, Math.max(200, durationMs)); + } + }, [isScrollLinked, setIsScrollLinked, captureScrollLinkDelta]); + const handleChangeNavigation = useCompareChangeNavigation( baseScrollRef, - comparisonScrollRef + comparisonScrollRef, + { temporarilySuppressScrollLink } ); const processingMessage = t('compare.status.processing', 'Analyzing differences...'); diff --git a/frontend/src/core/components/tools/compare/hooks/useCompareChangeNavigation.ts b/frontend/src/core/components/tools/compare/hooks/useCompareChangeNavigation.ts index f5240cc17..089283fab 100644 --- a/frontend/src/core/components/tools/compare/hooks/useCompareChangeNavigation.ts +++ b/frontend/src/core/components/tools/compare/hooks/useCompareChangeNavigation.ts @@ -2,12 +2,25 @@ import { RefObject, useCallback } from 'react'; type Pane = 'base' | 'comparison'; +type SuppressOptions = { + temporarilySuppressScrollLink?: (fn: () => void, durationMs?: number) => void; +}; + export const useCompareChangeNavigation = ( baseScrollRef: RefObject, - comparisonScrollRef: RefObject + comparisonScrollRef: RefObject, + options?: SuppressOptions, ) => { return useCallback( (changeValue: string, pane: Pane, pageNumber?: number) => { + const suppress = (fn: () => T) => { + if (options?.temporarilySuppressScrollLink) { + options.temporarilySuppressScrollLink(fn, 700); + } else { + fn(); + } + }; + const targetRef = pane === 'base' ? baseScrollRef : comparisonScrollRef; const container = targetRef.current; if (!container) { @@ -27,67 +40,82 @@ export const useCompareChangeNavigation = ( ) as HTMLElement | null; if (!pageEl) return false; const top = pageEl.offsetTop - Math.round(container.clientHeight * 0.2); - container.scrollTo({ top: Math.max(0, top), behavior: 'auto' }); + suppress(() => { + container.scrollTo({ top: Math.max(0, top), behavior: 'auto' }); + }); return true; }; - let nodes = findNodes(); - if (nodes.length === 0) { - scrollToPageIfNeeded(); - } - - let attempts = 0; - const ensureAndScroll = () => { - nodes = findNodes(); - if (nodes.length === 0 && attempts < 12) { - attempts += 1; - scrollToPageIfNeeded(); - window.requestAnimationFrame(ensureAndScroll); - return; - } - if (nodes.length === 0) { - // Fallback: ensure we at least scroll both panes to the page if available - if (pageNumber) { - // Main container already handled via scrollToPageIfNeeded; replicate for peer - const peerRef = pane === 'base' ? comparisonScrollRef : baseScrollRef; - const peer = peerRef.current; - if (peer) { - const peerPageEl = peer.querySelector( - `.compare-diff-page[data-page-number="${pageNumber}"]` - ) as HTMLElement | null; - if (peerPageEl) { - const peerMaxTop = Math.max(0, peer.scrollHeight - peer.clientHeight); - const top = Math.max(0, Math.min(peerMaxTop, peerPageEl.offsetTop - Math.round(peer.clientHeight * 0.2))); - peer.scrollTo({ top, behavior: 'auto' }); - } - } - } - return; - } - - const containerRect = container.getBoundingClientRect(); - let minTop = Number.POSITIVE_INFINITY; - let minLeft = Number.POSITIVE_INFINITY; - let maxBottom = Number.NEGATIVE_INFINITY; - let maxRight = Number.NEGATIVE_INFINITY; - - nodes.forEach((element) => { - const rect = element.getBoundingClientRect(); - minTop = Math.min(minTop, rect.top); - minLeft = Math.min(minLeft, rect.left); - maxBottom = Math.max(maxBottom, rect.bottom); - maxRight = Math.max(maxRight, rect.right); + const scrollPeerPageIfPossible = () => { + if (!pageNumber) return; + const peerRef = pane === 'base' ? comparisonScrollRef : baseScrollRef; + const peer = peerRef.current; + if (!peer) return; + const peerPageEl = peer.querySelector( + `.compare-diff-page[data-page-number="${pageNumber}"]` + ) as HTMLElement | null; + if (!peerPageEl) return; + const peerMaxTop = Math.max(0, peer.scrollHeight - peer.clientHeight); + const top = Math.max( + 0, + Math.min( + peerMaxTop, + peerPageEl.offsetTop - Math.round(peer.clientHeight * 0.2) + ) + ); + suppress(() => { + peer.scrollTo({ top, behavior: 'auto' }); }); + }; - const boxHeight = Math.max(1, maxBottom - minTop); - const boxWidth = Math.max(1, maxRight - minLeft); - const absoluteTop = minTop - containerRect.top + container.scrollTop; - const absoluteLeft = minLeft - containerRect.left + container.scrollLeft; - const maxTop = Math.max(0, container.scrollHeight - container.clientHeight); - const desiredTop = Math.max(0, Math.min(maxTop, absoluteTop - (container.clientHeight - boxHeight) / 2)); - const desiredLeft = Math.max(0, absoluteLeft - (container.clientWidth - boxWidth) / 2); + const proceedWithNodes = (nodes: HTMLElement[]) => { + if (nodes.length === 0) return; - container.scrollTo({ top: desiredTop, left: desiredLeft, behavior: 'smooth' }); + // Prefer a percent-in-page based vertical scroll, which is resilient to transforms. + const anchor = nodes[0]; + const pageEl = anchor.closest('.compare-diff-page') as HTMLElement | null; + const inner = anchor.closest('.compare-diff-page__inner') as HTMLElement | null; + const topPercent = parseFloat((anchor as HTMLElement).style.top || '0'); + if (pageEl && inner && !Number.isNaN(topPercent)) { + const innerRect = inner.getBoundingClientRect(); + const innerHeight = Math.max(1, innerRect.height); + const absoluteTopInPage = (topPercent / 100) * innerHeight; + const maxTop = Math.max(0, container.scrollHeight - container.clientHeight); + const desiredTop = Math.max( + 0, + Math.min(maxTop, pageEl.offsetTop + absoluteTopInPage - container.clientHeight / 2) + ); + suppress(() => { + container.scrollTo({ top: desiredTop, behavior: 'auto' }); + }); + } else { + // Fallback to bounding-rect based centering if percent approach is unavailable. + const containerRect = container.getBoundingClientRect(); + let minTop = Number.POSITIVE_INFINITY; + let minLeft = Number.POSITIVE_INFINITY; + let maxBottom = Number.NEGATIVE_INFINITY; + let maxRight = Number.NEGATIVE_INFINITY; + + nodes.forEach((element) => { + const rect = element.getBoundingClientRect(); + minTop = Math.min(minTop, rect.top); + minLeft = Math.min(minLeft, rect.left); + maxBottom = Math.max(maxBottom, rect.bottom); + maxRight = Math.max(maxRight, rect.right); + }); + + const boxHeight = Math.max(1, maxBottom - minTop); + const boxWidth = Math.max(1, maxRight - minLeft); + const absoluteTop = minTop - containerRect.top + container.scrollTop; + const absoluteLeft = minLeft - containerRect.left + container.scrollLeft; + const maxTop = Math.max(0, container.scrollHeight - container.clientHeight); + const desiredTop = Math.max(0, Math.min(maxTop, absoluteTop - (container.clientHeight - boxHeight) / 2)); + const desiredLeft = Math.max(0, absoluteLeft - (container.clientWidth - boxWidth) / 2); + + suppress(() => { + container.scrollTo({ top: desiredTop, left: desiredLeft, behavior: 'auto' }); + }); + } // Also scroll the peer container to the corresponding location in the // other PDF (same page and approximate vertical position within page), @@ -114,12 +142,16 @@ export const useCompareChangeNavigation = ( 0, Math.min(peerMaxTop, peerPageEl.offsetTop + absoluteTopInPage - peer.clientHeight / 2) ); - peer.scrollTo({ top: peerDesiredTop, behavior: 'smooth' }); + suppress(() => { + peer.scrollTo({ top: peerDesiredTop, behavior: 'auto' }); + }); } else if (peerPageEl) { // Fallback: Scroll to page top (clamped) const peerMaxTop = Math.max(0, peer.scrollHeight - peer.clientHeight); const top = Math.max(0, Math.min(peerMaxTop, peerPageEl.offsetTop - Math.round(peer.clientHeight * 0.2))); - peer.scrollTo({ top, behavior: 'smooth' }); + suppress(() => { + peer.scrollTo({ top, behavior: 'auto' }); + }); } } } @@ -169,7 +201,44 @@ export const useCompareChangeNavigation = ( }); }; - ensureAndScroll(); + let nodes = findNodes(); + if (nodes.length > 0) { + proceedWithNodes(nodes); + return; + } + + // Page-level fallback immediately so the user sees something happen + const scrolledPage = scrollToPageIfNeeded(); + if (scrolledPage) { + scrollPeerPageIfPossible(); + } else { + // Even if the page element is not present yet, try to nudge peer pane + scrollPeerPageIfPossible(); + } + + // Wait for highlights to mount (pages/images render progressively) + let settled = false; + const observer = new MutationObserver(() => { + if (settled) return; + const n = findNodes(); + if (n.length > 0) { + settled = true; + observer.disconnect(); + proceedWithNodes(n); + } + }); + try { + observer.observe(container, { childList: true, subtree: true }); + } catch { + // noop + } + // Safety timeout to stop waiting after a while + window.setTimeout(() => { + if (settled) return; + settled = true; + observer.disconnect(); + // We already scrolled to the page above; nothing else to do. + }, 5000); }, [baseScrollRef, comparisonScrollRef] );