fix: wrong scroll offset when dynamic mode in window scroll

[piecyk]

No description provided.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-virtual/95cy713NJYijQ3dE43gb597Lbjnn
✅ Preview: https://react-virtual-git-fork-piecyk-fix-scroll-offset-135ee1-tanstack.vercel.app

[piecyk]

Fixed for multiple virtual list, when next getting wrong scroll offset as perv is changing it total size.

[piecyk]

@tannerlinsley please have a look, would be good to fix this.

[bipinrajbhar]

@tannerlinsley please have a look, would be good to fix this.

Any updates?

[bryan-hunter]

can you add the below to lines 107-108?

        width: element.innerWidth ? element.innerWidth : element.offsetWidth,
        height: element.innerHeight ? element.innerHeight : element.offsetHeight,

When I add this and your changes in this PR, it allows me to use any outer container, not just Window, because innerWidth/height only exist on window and not other elements

The change makes it some 'outerSize' calculates correctly, so it knows what the start/end indexes are

[bryan-hunter]

also, thank you for this! This update came right when I needed it :)

[bryan-hunter]

I actually ended up having to add one more thing to the scrollToFn in useWindowScroll:

    const scrollElementKey = horizontal ? 'scrollLeft' : 'scrollTop'
    // grab different properties if an element and not 'window'
    const refScrollKey = windowRef.current[scrollKey] === undefined ? scrollElementKey : scrollKey
    const refRectKey = windowRef.current[rectKey] === undefined ? scrollElementKey : rectKey

    const delta = ['ToIndex', 'SizeChanged'].includes(reason) ? windowRef.current[refScrollKey] + parentRef.current.getBoundingClientRect()[rectKey] : 0
    windowRef.current.scrollTo({ [refRectKey]: offset + delta })

[bryan-hunter]

and if anyone else stumbles into this somehow - I had to swap all of the useLayoutEffect that were doing setElement based on ref.current with just useEffect

This was due to using React Suspense - the ref.currents were null on the useLayoutEffect, but somehow weren't on the useEffect - I do not know why, but I think React 18 will solve that - didn't seem like there were any issues or performance differences using useEffect on these either

[piecyk]

@bryan-hunter thanks, imho we should merge this first, then we can improve.

This looks interesting

I had to swap all of the useLayoutEffect that were doing setElement based on ref.current with just useEffect

Could you create a codesnabdox with the bug and add new issue.

[bryan-hunter]

first off, thanks for responding and being so helpful on everything in this repo :)

I think I am seeing where my thinking was flawed.

I use a fallback skeleton that is 100% width/height for most things like this - and that was assuming the wrapper container was giving it the space it needed

On my Fixed lists, it worked great because the estimateSize was always a real, fixed size so the container wrappers always provided the skeletons the right space to fill to 100%.

the Dynamic lists are actually the opposite - the container can estimate the size and provide it, but ultimately we're asking the library to re-measure it once it renders because the child knows its own height, and that's where the Suspense issue was coming in.

I'm actually wondering...if I just let each row's Suspense carry all the way up above where the measureRef is, maybe it won't execute the measureRef until it's done suspending and has a height.. 🤔 🤔

[bryan-hunter]

eureka - https://codesandbox.io/s/toend-forked-cphpc?file=/src/index.js

that seemed to work, just had to make sure Suspense and the promise were executed above the div with the measureRef

[bryan-hunter]

ended up going with the ResizeObserver - too many other things were suspending on the same page above the list on initial render and no guarantees on display none not messing with the height of a row

I believe I will also need this ResizeObserver on my "container ref" that isn't the windowRef - but again I feel like that is only necessary because of Suspense and the display none.

Thanks for all of the help and making the library so flexible to handle these anomalies - it's even flexible enough to handle React issues :p

[bryan-hunter]

If you want to check out this Library used heavily in action with a lot of infinite scrolls, check out the RYSE app :p - https://apps.apple.com/us/app/ryse/id1560237752 - it's new and free and wouldn't be nearly as smooth without this nice library

[piecyk]

@bryan-hunter glad to help, awesome that it worked for you!

[tannerlinsley]

We have some conflicts here from reverting the breaking change, so a fixup is necessary. Also, I'm not sure if this PR needs others merged first, or if it needs to be pointed at the next branch? @piecyk ?

[tannerlinsley]

What exactly are we going to use the next branch for now that we have the Alpha branch?

[piecyk]

What exactly are we going to use the next branch for now that we have the Alpha branch?

So next we should rebased next with alpha, and sort things in what direction we want to go 😉 The hook idea for supporting window scroll was not that bad, plus revert changes that triggers re-render on every scroll event.

So to re-cap main idea behind useScroll hook we can replace those two

  onScrollElement?: React.RefObject<HTMLElement>
  scrollOffsetFn?: (event?: Event) => number

and because scroll offset should not trigger re-render we need to pass a callback for it

useScroll?: (
  onScrollRef: React.MutableRefObject<(offset: number) => void>,
  options: {
    parentRef: React.RefObject<T>
    horizontal: boolean
  },
) => {
  scrollToFn: (offset: number) => void
}

For simplicity I would only allow to pass custom useScroll when we are not in window scroll, so types for useVirtual could be

interface BaseOptions<T> {
  size: number
  parentRef: React.RefObject<T>
  estimateSize?: (index: number) => number
  overscan?: number
  horizontal?: boolean
  scrollToFn?: (
    offset: number,
    defaultScrollToFn?: (offset: number) => void,
  ) => void
  paddingStart?: number
  paddingEnd?: number
  initialRect?: Rect
  keyExtractor?: (index: number) => Key
  rangeExtractor?: (range: Range) => number[]
  measureSize?: (el: HTMLElement, horizontal: boolean) => number
}

interface ElementOptions<T> extends BaseOptions<T> {
  useScroll?: (
    onScrollRef: React.MutableRefObject<(offset: number) => void>,
    options: {
      parentRef: React.RefObject<T>
      horizontal: boolean
    },
  ) => {
    scrollToFn: (offset: number) => void
  }
  useObserver?: (ref: React.RefObject<T>, initialRect?: Rect) => Rect
  windowRef?: undefined
}

interface WindowOptions<T> extends BaseOptions<T> {
  useScroll?: undefined
  useObserver?: (ref: React.RefObject<Window>, initialRect?: Rect) => Rect
  windowRef: React.RefObject<Window>
}

export type Options<T> = ElementOptions<T> | WindowOptions<T>

Then implementation for element scroll can look like

export const useElementScroll = <T extends HTMLElement>(
  onScrollRef: React.MutableRefObject<(offset: number) => void>,
  { parentRef,  horizontal }: { parentRef: React.RefObject<T>; horizontal: boolean },
) => {
  const updateCounter = useCounterOnRefChange(parentRef)

  const scrollKey = horizontal ? 'scrollLeft' : 'scrollTop'

  useIsomorphicLayoutEffect(() => {
    const element = parentRef.current

    if (!element) {
      return
    }

    const handleScroll = () => {
      onScrollRef.current(element[scrollKey])
    }

    handleScroll()

    element.addEventListener('scroll', handleScroll, {
      capture: false,
      passive: true,
    })

    return () => {
      element.removeEventListener('scroll', handleScroll)
    }
  }, [updateCounter, scrollKey, onScrollRef, parentRef])

  const scrollToFn = React.useCallback(
    (offset: number) => {
      if (parentRef.current) {
        parentRef.current[scrollKey] = offset
      }
    },
    [parentRef, scrollKey],
  )

  return {
    scrollToFn,
  }
}

Then we can export it, allowing user to re-use that for custom scroll elements

import { useVirtual, useElementScroll } from '@tanstack/react-virtual'

const parentRef = React.useRef<HTMLDivElement | null>(null) 
const scrollerRef = React.useRef<HTMLDivElement | null>(null)

useVirtual({
  size
  parentRef,
  useScroll: (onScrollRef, options) => useElementScroll(onScrollRef, { ...options, parentRef: scrollerRef })
})

<div>...</div>