fix(#8625): smooth scrolling in SPA mode on iOS

[sanman1k98]

Changes

Fixes #8625.

Instead of adding an event listener for hundreds of scroll events (since Safari doesn't support scrollend events), I used requestAnimationFrame() to check if the window stopped scrolling and update accordingly.

I'm not experiencing stuttering with this change but I'm not sure if it's the "correct" way to do it. Thoughts?

Testing

Built a separate project with the <ViewTransitions /> router and previewed on an iPad Pro 11" M1 (ProMotion 120Hz) and 2015 MacBook Pro 13" (60Hz).

Placed an iPhone 15 Pro on a flat surface and watched inertial scrolling 6 inches away from my face for 45 seconds.

Docs

N/A just a bug fix.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4096ff5

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[martrapp]

Hi @sanman1k98, thank you for this PR!
However, I am afraid that we will not be able to merge it as it is.

• We don't really understand why the stuttering occurs on some iPhones and not others.
• On most devices and browsers that don't support scrollend, listening to single scroll events works fine. I'm not even sure if throttling would be necessary, as recording the scroll position in the history state shouldn't be a real burden.
• The current implementation is throttled to about 3 calls per second. The code executed on each scroll event is rather minimal.
• I see that your solution avoids listening to scroll events, but I don't see why hooking into high frequency frame events makes a difference. Your callback is not triggered by scrolling. It runs on every single frame, regardless of scrolling?

What complicates this further is the fact that in most situations we don't need to dynamically record the scroll position: Every time we do a forward navigation to the next page, we synchronously update the last scroll position for the old page and we're done. The whole event-driven recording of scroll positions is only necessary for history navigation, where the history entry for the next page is set before the popstate event is triggered. For this reason, we have no way to record the current scroll position for the page we left in the history state of the browser.

A solution that works completely without events could use a data store that is independent of the history state. However, it makes little sense to re-implement the browser's history API for this purpose. I hope that the navigation API can help here.

The current solution with events and throttling is not really good: in rare cases we can still lose events for the page we left. The solution proposed in this PR might suffer from the same weakness, as it also might ignore scroll position changes up to 350 ms and a history-back navigation might well fall into this interval.

I would be very interested in getting rid of the iOS scrolling issues. Personally, I have been reluctant to "improve" this part of the code without the certainty that it is better (or at least not worse) for all users than before. If we change this, it should be for good. Maybe you can find another solution that directly addresses the iOS issue without degrading the currently working solution for other environments?

[martrapp]

if a popstate event took place in the meanwhile, history.state has changed to the state of the new page ...

[martrapp]

Once started on the first page load, this runs forever on each frame?
Even when the browser throttles down animation frames due to inactivity, a new animation frame might well occur 60, 120 or even 240 times a second.

[sanman1k98]

Thank you very much @martrapp for the feedback!

However, I am afraid that we will not be able to merge it as it is.

No worries, I’m not gonna give up anytime soon! First, I would like to try to address the points you brought up.

On most devices and browsers that don't support scrollend, listening to single scroll events works fine. I'm not even sure if throttling would be necessary, as recording the scroll position in the history state shouldn't be a real burden.

The current implementation is throttled to about 3 calls per second. The code executed on each scroll event is rather minimal.

When the throttle time was increased in #8981, it is mentioned that Safari logs a warning when exceed 100 calls to history.replaceState within 30 seconds. As I was typing this out, I learned that it had to do with preventing DoS attacks on the browser process:

• comment #5 on WebKit Bug 156115
• Chromium Issue 40080060
• Chromium Issue 41356126

TL;DR calling at a high frequency history.pushState or history.replaceState does a surprising amount of work.

I see that your solution avoids listening to scroll events, but I don't see why hooking into high frequency frame events makes a difference. Your callback is not triggered by scrolling. It runs on every single frame, regardless of scrolling?

Yeah... it does run on every frame...

I was tunnel visioned on the fixing the stuttering and thought that requestAnimationFrame was the magical solution. Once it started scrolling like butter again, I went straight here lol.

I’ll do it properly and add the logic to run the callback only when the user is scrolling, call replaceState only when the scroll positions have stopped changing, and I should be able to shorten that 350ms interval.

Hopefully it will be an improvement for all users!

[martrapp]

Thank you @sanman1k98! I had a first quick look at your new version and this looks in deed like a big improvement over the current throttling. I'm a bit busy right now, but I'm already looking forward to have more time for your PR.

[martrapp]

@sanman1k98 Great! I really appreciate your approach!

This is in deed a big improvement and my suggestions will gradually go down to nitpicking ;-)

1. Currently, the last* values are initialized on the first page that uses view transitions and they are only re-initialized at the end of an interval. If you navigate to the next page without scrolling and than scroll < 200 ms (e.g. by pressing PageDown) we will not record anything because the index doesn't match. it would be better to initialize the last* values together with the call to setInterval()`.

2. I stumbled across the combined condition lastY === scrollY && lastX === scrollX && (lastY !== history.state.scrollY || lastX !== history.state.scrollX) as the guarded block does two things: updates the history state and stops the interval
   I guess you added lastY !== history.state.scrollY || lastX !== history.state.scrollX as an optimization to skip replaceState calls? If so, you should move it to guard the onScrollEnd() call. And we would break the interval on lastY === scrollY && lastX === scrollX independent of the history state.

3. With this slim design, I'm sure you could experiment with reducing the interval's ms to something around 20 instead of 200 without performance penalties.

4. last, no need to update lastIndex at the end of the interval if we do No. 1) from above

I'll leave suggestion in the code for those points...

Might have overseen something, but that would be my review comments.

Again, I'm really happy that you came up with this solution!

[sanman1k98]

@martrapp Thank you for your suggestions and helping me fix the errors in my code! I will get to implementing those changes later today!

[martrapp]

I think it's really great what you've done with it!
Thank you so much!

[glib-0]

This fix throws a TypeError when history.state is null - I would submit a separate bug report but it doesn't seem to really break much (just pollutes my console with errors) and I can't reproduce it in Stackblitz because it seems to initialise history.state as an empty object. To reproduce locally I just import navigate from astro:transitions/client in a React component and have a window that scrolls either direction; don't even need to call navigate().

[martrapp]

Hi @glib-0,
do you have a stack trace for me?

[martrapp]

Ah, never mind, see it...

[martrapp]

Hi @glib-0, thank you very much for pointing out!
Hope that #10456 fixes the issue for you!