Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better control over scroll restoration during traversal #187

Closed
domenic opened this issue Nov 16, 2021 · 7 comments
Closed

Better control over scroll restoration during traversal #187

domenic opened this issue Nov 16, 2021 · 7 comments
Labels
addition A proposed addition which could be added later without impacting the rest of the API feedback wanted

Comments

@domenic
Copy link
Collaborator

domenic commented Nov 16, 2021

Talking with a few web developers, we've pinpointed a few issues with scroll restoration during traversal, that seem relevant to any history/navigation API.

The essential problem we've heard so far is that scroll restoration happens unpredictably and often at the wrong times. For example:

  • The browser tries to restore scroll position, but your app is still setting up the DOM and the relevant elements aren't ready yet
  • The browser tries to restore scroll position, but the page's contents have changed and scroll restoration doesn't work that well (e.g., in a listing of files in some shared folder, someone deleted a bunch of the files)
  • You need to make some measurements in order to do a proper transition, but the browser does scroll restoration during your transition which messes it up. (Demo)

Note that these problems are present for both cross-document traversals and same-document traversals.

The developers I've talked to have expressed that they're looking for something between the two extremes we currently have available:

  • Disabling scroll restoration and handling it yourself (via history.scrollRestoration = "manual")
  • Letting the browser perform scroll restoration at an unpredictable time and in an unpredictable manner, after traversal

Here's an initial stab at a solution for this problem space. Note that the core proposal is not really tied to the rest of the app history API, and might end up advancing as a separate proposal on the whatwg/html repo. But I wanted to start with this community, since it fits into the general problem space of "ideal new APIs for handling history/navigation". And I do have an app history specific bonus proposal below the main one.

Proposal

Introduce a new event, beforescrollrestoration. It can be targeted at any scrollable container, with the <html> element being the one for the overall viewport. (Or, the <body> element, in quirks mode...) But e.g. if you were implementing your page as one large scrollable <div> inside a non-scrolling viewport, then you'd listen for the event on that <div>.

This event would fire whenever the browser would normally perform scroll restoration. Per the current spec and tests, is after popstate/before hashchange. (App history's currentchange comes before popstate, so the total sequence woul be: currentchange, popstate, beforescrollrestoration, hashchange.) But, see below...

The event has the following methods:

  • preventDefault(): stops scroll restoration from happening, for now
  • getDestination(): returns a { scrollLeft, scrollTop } pair corresponding to where the browser would currently restore the scroll position, if scroll restoration were to happen right now.
  • restore(): performs scroll restoration, essentially equivalent to
    const { scrollLeft, scrollTop } = event.getDestination();
    event.target.scrollLeft = scrollLeft;
    event.target.scrollTop = scrollTop;

The key thing about this event is that its getDestination() and restore() methods can still be used, in the future, even after preventDefault() was called. So here is an example use:

document.documentElement.addEventListener("beforescrollrestoration", e => {
  if (!everythingIsLoadedAndClientSideRendered()) {
    e.preventDefault();
    setClientSideRenderingFinishedCallback(() => doScrollRestoration(e));
  }
});

function doScrollRestoration(event) {
  const { scrollLeft, scrollTop } = event.getDestination();

  if (!shouldStillDoScrollRestoration(scrollLeft, scrollTop)) {
    // Don't do any scroll restoration if something dramatic has changed,
    // e.g. if a bunch of files were deleted and we're going back to the
    // file list view. Maybe scrollLeft/scrollTop could help with this logic.
    return;
  }
  
  event.restore();
}

Note in particular that the value getDestination() returns can change over time, as a result of this sort of delay. E.g., if the browser has stored internally something like "100px from the top of the #foo element", if the #foo element is not in the DOM at beforescrollrestoration time, it will probably return (0, 0) or some other inaccurate value. But if, by the time we get to the doScrollRestoration function, the page has done all its client-side rendering and put #foo into the DOM, getDestination() will return some useful value, and restore() will restore the scroll position to that useful location.

Potentially at some point (such as if the user ever starts scrolling?) these methods should stop working, e.g. returning null for getDestination() and doing nothing when you call restore(). At least in Chromium we abort any attempts at scroll restoration if the user starts scrolling plus a few other conditions I haven't fully understood yet.

Also note that this event fires both for same-document navigations, and cross-document navigations.

App history-specific bonus proposal

The above seems like the right primitive that should allow developers all the control they need. (At least, in cases where they don't want to just handle scroll restoration completely manually, using history.scrollRestoration = "manual".)

But for the specific case of same-document transitions, we can leverage the navigate and the promise passed to transitionWhile() to handle things a bit more automatically. I'm thinking that by default, whenever you use transitionWhile(), we delay scroll restoration until the promise settles. Since the promise should ideally be settled when everything is loaded and the DOM is ready, this will probably work well in most cases.

Doing so takes the scroll restoration process, as well as its corresponding beforescrollrestoration event, out of the normal ordering mentioned above.

We'd let you opt out of this, using something like the following:

navigateEvent.transitionWhile(promise, { scrollRestoration: "immediate" });
// Default value: "after-transition". Another possible value: "manual".

This doesn't solve all the scroll restoration problems:

  • It does nothing for cross-document cases, since those don't involve transitionWhile() (and, more generally, the navigate event fires in the context of the old document, not the new one)
  • It doesn't help with cases like https://nifty-blossom-meadow.glitch.me/legacy-history/transition.html , where the concern is not about setting up the DOM, but instead performing correct measurements that don't get borked by scroll restoration.

To solve the latter, you could mix in the beforescrollrestoration event in a somewhat messy way:

navigateEvent.transitionWhile((async () => {
  let bsrEvent;
  document.documentElement.addEventListener("beforescrollrestoration", e => {
    e.preventDefault();
    bsrEvent = e;
  }, { once: true });

  await fetchDataAndSetUpTheDOM();
  bsrEvent.restore();
  await measureAndDoTransition();
})(), { scrollRestoration: "immediate" });

We could make this nicer by introducing navigateEvent.scrollRestoration, with getDestination() and restore() methods, whenever scrollRestoration: "manual" is set. Then the above messy code could be rewritten as

navigateEvent.transitionWhile((async () => {
  await fetchDataAndSetUpTheDOM();
  navigateEvent.scrollRestoration.restore();
  await measureAndDoTransition();
})(), { scrollRestoration: "manual" });
@domenic domenic added addition A proposed addition which could be added later without impacting the rest of the API feedback wanted labels Nov 16, 2021
@domenic domenic added this to the Might block v1 milestone Nov 16, 2021
@domenic
Copy link
Collaborator Author

domenic commented Nov 16, 2021

Labeling "Might block v1" specifically for the part

I'm thinking that by default, whenever you use transitionWhile(), we delay scroll restoration until the promise settles.

since changing that default would probably cause compat issues.

@posva
Copy link
Contributor

posva commented Nov 17, 2021

I really like the idea of being able to attach scroll restoration events to any element. That's one of the problems we have now: we can only restore the scroll on the window element. If the user wants to scroll a different element, they have to do it manually.

Something that worries me is not being able to fully control when the event triggers: I can delay the transitionWhile but in a data driven approach, the navigation finishes to update the route location, which in turn triggers the DOM update, and only then the scrollBehavior (a custom function passed to the router instance in the case of vue router) would trigger. I could definitely update the route location that triggers the DOM update and wait to finish the navigation but that will mean that the DOM update would happen before the navigation has finished, potentially letting the user read from old history entries.

Currently, in Vue Router we rely on manual scroll restoration because it gives us the flexibility to integrate it with Vue. I imagine other frameworks do the same.

@domenic
Copy link
Collaborator Author

domenic commented Nov 18, 2021

I'm not sure I understand the concern in your second paragraph, probably because of lack of familiarity with Vue Router.

First, the intent is that you do kinda get full control over scroll restoration: for the beforescrollrestoration event, you can call event.preventDefault() and then call event.restore() later. And for the app history-specific proposal, you could use navigateEvent.scrollRestoration.restore() in combination with the { scrollRestoration: "manual" } option. So maybe all the tools are there in the proposal?

Second, in general we want to encourage the promise passed to respondWith() to not settle until the DOM updates are done. That is, the browser spinner should not stop spinning, and accessibility tech should not announce the navigation is finished, and so on, until all the appropriate content is in the DOM and settled down. But it sounds like that might cause some concern about "letting the user read from old history entries"? I don't understand that concern.

Looking forward to your thoughts.

@posva
Copy link
Contributor

posva commented Nov 19, 2021

Second, in general we want to encourage the promise passed to respondWith() to not settle until the DOM updates are done.

I think this clears up everything to me and it makes total sense.

About navigateEvent.scrollRestoration, I couldn't find that one here or in the d.ts declaration, is it somewhere else?

PS: you are referring to transitionWhile(), aren't you? Wasn't respondWith() the old naming?

@domenic
Copy link
Collaborator Author

domenic commented Nov 19, 2021

About navigateEvent.scrollRestoration, I couldn't find that one here or in the d.ts declaration, is it somewhere else?

It is being newly-proposed in this post :)

PS: you are referring to transitionWhile(), aren't you? Wasn't respondWith() the old naming?

Yes, sorry about that! transitionWhile() is indeed correct.

@domenic
Copy link
Collaborator Author

domenic commented Nov 19, 2021

Apparently the beforescrollrestoration type event has been tried before! It failed, per majido/scroll-restoration-proposal#3 , because for cross-document navigations there's not a good time to fire the event: script likely won't be set up by the time the event needs to fire.

This proposal has the same problem! If we fire beforescrollrestoration like the proposal says, nobody will be listening for cross-document cases :(

I will try to think if there is a way around this...

@domenic
Copy link
Collaborator Author

domenic commented Jan 10, 2022

I've specced out a minimal version of this, with only app history integration and only "after-transition" and "manual", for now. See #197. But per #197 (comment) there are some issues with that API surface; any ideas would be welcome.

domenic added a commit that referenced this issue Feb 1, 2022
Plus, expand and organize the stuff about accessibility technology announcements and loading spinners.

Closes #25. Closes #187. Closes #190.
@domenic domenic closed this as completed in 8a3d47b Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition A proposed addition which could be added later without impacting the rest of the API feedback wanted
Projects
None yet
Development

No branches or pull requests

2 participants