beforeNavigate doesn't trigger default browser behaviour as-per the docs

[oodavid]

Describe the bug

A follow-up from #8482.

The beforeNavigate documentation states that:

"calling cancel will trigger the native browser unload confirmation dialog. In these cases, navigation.willUnload is true."

When linking to an external site, we're told that { ... navigation.willUnload: true }, but the native browser unload confirmation dialog doesn't appear as expected.

Reproduction

https://github.com/oodavid/sveltekit-beforeUnload - reproduction

https://www.loom.com/share/4e3b04fc7def408c9010285e960e0577 - walkthrough video (1m 45s)

Logs

No response

System Info

System:
    OS: macOS 12.3.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 153.75 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.18.1 - ~/.nvm/versions/node/v16.18.1/bin/node
    npm: 8.19.2 - ~/.nvm/versions/node/v16.18.1/bin/npm
  Browsers:
    Chrome: 109.0.5414.87
    Edge: 109.0.1518.52
    Firefox: 105.0.1
    Safari: 15.4
  npmPackages:
    @sveltejs/adapter-auto: ^1.0.2 => 1.0.2 
    @sveltejs/kit: ^1.1.4 => 1.1.4 
    svelte: ^3.55.1 => 3.55.1 
    vite: ^4.0.4 => 4.0.4

Severity

annoyance

Additional Information

No response

[dummdidumm]

Probably because of the rel which triggers a different path. Giving it both the bug and documentation label until we know which of those it is.

[oodavid]

Is there anything I can do to help figure that out?

I've not defined [rel] on my repo, IIRC it has no default value

[eltigerchino]

When we click a link and call cancel in beforeNavigate, we are preventing navigation by cancelling the click event. Since the click event is cancelled, the beforeunload event never occurs and the confirmation dialog never appears. This is contrary to the documentation.

If the navigation would have directly unloaded the current page, calling cancel will trigger the native browser unload confirmation dialog.

We can't fix this without allowing the click event to go through and letting beforeunload handle cancelling the navigation. However, this would prevent user-defined confirmation dialogs when cancelling navigation in beforeNavigate #9747 (comment) . Calling cancel() when the document is unloading would then mean the native confirmation dialog is guaranteed to show.

I think we should keep the behaviour consistent. So either: cancel() always uses the native browser unload confirmation dialog or it never does. Then we can document that users should listen to <svelte:window onbeforeunload /> and call event.preventDefault to get the native browser dialog or extend the cancel api to do so

[dummdidumm]

I slightly lean twoards never showing the native dialog by default, you likely want to provide your own slick UI instead, but in the case of closing the tab (not clicking a link) it's just not possible to do anything else other than showing the native confirmation dialog (for security etc reasons). That leaves us only with the other option of always showing the native UI on external link clicks, which has the drawback of #9747 (comment) . I'm not sure how big of a deal this is in practise.

A way out of the inconsistency would be #8625, in other words beforeNavigate would not care about the "I'm just closing the browser tab" use case anymore. It would potentially introduce a "double confirmation" though, because you could say "show fancy UI" in beforeNavigate after clicking an external link, where the user presses "yes" only to be greeted by another browser alert (if the dev also added beforeUnload logic) - a way around this would be to somehow check that beforeUnload is a direct result of a beforeNavigate confirmation but it would only work in certain cases. It's also still not fully correct semantically because beforeNavigate would now also ignore things like location.href = .. . Which tips me back towards not splitting this up.

Gah, this is impossible to get right in all cases... My response at this point is "browsers are complicated, it isn't completely consistent, sorry".

[eltigerchino]

That leaves us only with the other option of always showing the native UI on external link clicks, which has the drawback of #9747 (comment) . I'm not sure how big of a deal this is in practise.

Thinking about it again, it would be easy enough for a user to check if the navigation type is a link and if the page will unload.

beforeNavigate(({ type, cancel, willUnload }) => {
  if (type === link && willUnload) {
    // show custom ui
  } else {
    // show native browser dialog
    cancel()
  }
});

What do you think? They can only show the custom dialog on link clicks anyway. Maybe cancel needs a more fitting name for its behaviour too?

[Rich-Harris]

It would potentially introduce a "double confirmation" though

I don't think that's quite right — If I understand #8625 correctly, the proposal is that clicking on an external link wouldn't invoke beforeNavigate callbacks in the first place. They would only be called when performing a client-side navigation to a SvelteKit-owned route.

There's something very appealing about that solution — it would allow us to get rid of willUnload and type: 'leave' and provides more conceptual simplicity. But it does rob us of the ability to provide custom 'unsaved changes, are you sure?' UI in the case where the user clicked an external link, which is a sacrifice I don't think we want to make. (We can't show custom UI in the case where the user closes the tab or clicks 'back' to a different document, but inconsistency is preferable to not having the ability at all.)

It also means that you'd need to have the cancellation logic in two places:

let unsaved_changes = false;

beforeNavigate(({ cancel }) => {
  if (unsaved_changes) cancel();
  show_custom_ui();
});

beforeUnload(({ cancel }) => {
  if (unsaved_changes) cancel();
});

(Side-note: there's really no point in having a beforeUnload function. Just add a beforeunload handler to <svelte:window>.)

Thinking about it again, it would be easy enough for a user to check if the navigation type is a link and if the page will unload.

It's simpler than that — we just need to check whether type === 'leave':

beforeNavigate(({ type, cancel }) => {
  cancel();

  if (type !== 'leave') {
    show_custom_ui();
  }
});

It is arguably a little bit confusing that willUnload doesn't mean 'cancel() will show a browser confirmation dialog' while type === 'leave' does, since the dialog is associated with unloading. Perhaps if it was type === 'unload' instead? I'll have to try and track down the conversation that led to type === 'leave' to see if there's a good reason for it being that way.

So I think there are two possibilities:

• we rename 'leave' to 'unload'
• we update the docs to say something like 'If navigation.type === 'leave', calling cancel will trigger the native browser unload confirmation dialog. The navigation may or may not be cancelled, depending on the user's response.'

[Rich-Harris]

Ok, here's the PR where leave was introduced. (Prior to that, it was unload, and we changed it for the reasons discussed at length therein.)

So I think this is just a documentation issue. I'll open a PR.