inertiajs / inertia

Inertia.js lets you quickly build modern single-page React, Vue and Svelte apps using classic server-side routing and controllers.
https://inertiajs.com
MIT License
6.3k stars 423 forks source link

React: ScrollReset is slightly out of sync with Page changes, causing a flash before navigating #1802

Closed oscarnewman closed 2 weeks ago

oscarnewman commented 7 months ago

Version:

Describe the problem:

I noticed an issue working on a new Inertia React/Laravel app where page navigations via Link would suffer what looked like a flash of content before moving to the next page.

(this is my fist time using Inertia and it's been an awesome experience, btw!)

Looking more closely, I found that the "scroll reset" step was occurring an instant before the page itself changed to the new content. This means that if a user is scrolled down at all on a page and they navigate to another page via Inertia, they'll see an instant where their current page scrolls all the way to the top before they see the next page.

On smaller/lighter weight pages, this is less noticeable. On heavier pages or on mobile, this was causing decent Jank.

I've attached a video of a simple reproduction in the React playground, and one from my own app.

It's less consistent/harder to notice in the React playground. I think this might be because the render pass is just faster with less content and is less likely to get out of sync. I recommend scrubbing through the video frame by frame slowly to see what's happening.

In the playground:

https://github.com/inertiajs/inertia/assets/4020585/ed6e9667-507a-4e1b-b772-3a056b73f09a

In my own app:

https://github.com/inertiajs/inertia/assets/4020585/7ad0381c-0d90-4cc5-b542-2bafe2dade87

Steps to reproduce:

  1. Create a page that causes the browser viewport to scroll
  2. Include a link to another page
  3. Click that link while scrolled down, and observe a flash of scroll-to-top before the transition

Reproduction Repo:

I've updated the "React Playground" in the repo to use a Sticky header -- this makes it possible to click a navigation link while scrolled down on a page.

That change is visible here: https://github.com/inertiajs/inertia/compare/master...oscarnewman:inertia:reproduction

I'm able to reproduce the issue in about 1/4 navigations from the bottom of the "Article" page to any other page.

Root cause / Solution

I dug into the Inertia code a bit to see if I could figure out what's happening. I think it primarily occurs in this router.setPage function (comments are my annotations):

protected setPage(
    page: Page,
    {
      visitId = this.createVisitId(),
      replace = false,
      preserveScroll = false,
      preserveState = false,
    }: {
      visitId?: VisitId
      replace?: boolean
      preserveScroll?: PreserveStateOption
      preserveState?: PreserveStateOption
    } = {},
  ): Promise<void> {
    return Promise.resolve(this.resolveComponent(page.component)).then((component) => {
      if (visitId === this.visitId) {
        page.scrollRegions = page.scrollRegions || []
        page.rememberedState = page.rememberedState || {}
        replace = replace || hrefToUrl(page.url).href === window.location.href
        replace ? this.replaceState(page) : this.pushState(page)

        // `swapCompnent` is set by the consuming package (react, vue, etc). It is responsible for actually rendering a new page
        this.swapComponent({ component, page, preserveState }).then(() => {
          if (!preserveScroll) {
            this.resetScrollPositions() // We seem to have some sort of race condition with this resetScrollPositions function
          }
          if (!replace) {
            fireNavigateEvent(page)
          }
        })
      }
    })
  }

I was curious how other frameworks solved this, and look into React React Router's Implementation.

The key seems to be that they use useLayoutEffect to ensure that the scroll position is reset before the browser repaints the new dom/vdom.

The fundamental difference then seems to be:

In Inertia:

This is a little awkward with Inertia's current model of what it delegates to the framework-specific packages and what it keeps in the core, as scroll management is entirely in the core. I'm not sure if this also happens with other frameworks yet, but at the very least I imagine the react package at least will need to become aware of when a scroll reset is necessary and handle calling reseScrollPositions() itself inside the react lifecycle.

I'm gonna try to work on a PR for this as well, but wanted to put the issue up for context

pedroborges commented 2 weeks ago

Fixed by #1980 ✅