reset focus synchronously

[Rich-Harris]

As of #8466, we wait for a setTimeout to complete before navigation can finish. This causes a flash between the page being updated and a subsequent snapshot.restore(...), which feels somewhat janky (scroll positions aren't immediately recovered, etc).

I'm honestly not sure if the tests will pass on all browsers if we just do the setTimeout without awaiting its completion, but opening this PR is the easiest way to find out.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 83fabe9

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

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

[dummdidumm]

I vaguely remember that one of these can only be tested manually, so we'd also have to see if the original bug that was fixed stays fixed by trying out ourselves again.

Edit: This will reintroduce the bug in #8439 because getSelection()?.removeAllRanges() will be called after the afterNavigate callback. Also this PR likely does not change the janky behavior because the await restore_focus() needs to be restore_focus().
This makes me wonder - is the order of "restore snapshot, then call afterNavigate" more correct than what's there now with "call afterNavigate, then restore snapshot"? If we were to change that order then we could fix both issues: first onMount, then restore snapshot, then reset selection, then do the afterNavigate call. Also feels semantically more fitting to me, since restoring the snapshot feels part of the navigation to me (which means it isn't finished yet when snapshots haven't been restored yet).

[Rich-Harris]

Reordering seems reasonable, yeah. It does fix the janky behaviour — even though I forgot to remove the await, it doesn't matter because the browser will never repaint until the microtask queue is exhausted

[Rich-Harris]

Recapping discussion we had just now: the current order of events upon navigation is

update page
reset scroll
reset focus

<wait for setTimeout>

reset selection
after navigate callbacks
snapshot restoration

The problem here, which is fixed by this PR, is the <wait for setTimeout> — this causes an unwanted flash. But not waiting for it reintroduces #8439.

The solution I'm going to try is to keep the behaviour in this PR, but only reset the selection range if we detect that it didn't change (as a result of an element being (auto)focused, or a programmatic selection change in afterNavigate). I think that's possible.

At the same time, I'll move snapshot restoration before after navigate callbacks — it's a bug that it happens the other way around. So the new order will be

update page
reset scroll
reset focus
snapshot restoration
after navigate callbacks

<wait for setTimeout>
reset selection if necessary

[dummdidumm]

Are you sure we need this level of comparison? I did getSelection().getRangeAt(0) === getSelection().getRangeAt(0) in the browser console and it spits out true. If I save one to a variable, select something else, it's false

[Rich-Harris]

Oh, interesting. Yeah I think I'm overcomplicating things. Simplified it

[Rich-Harris]

gah this causes a whole bunch of failures in safari, but only in safari

GODDAMMIT SAFARI NOT NOW

[dummdidumm]

Mhm so seems like the object is not the same on safari then..? 😓 Meany we gotta go back to that comparison. Although I'm wondering if there's any value in code-golfing it to a function and then doing isSame('propertyName1') && isSame('propertyName2') && ..

[Rich-Harris]

Yeah, this code returns true in Chrome/Firefox and false in Safari:

getSelection().getRangeAt(0) === getSelection().getRangeAt(0)

So dumb. I think after gzip there's probably not much value in making the function

[dummdidumm]

I think we can code golf a bit at least by turning this into one big if(.. !== .. || ..) return