fix: ensure beforeNavigate works on other navigations after clicking a download link

[eltigerchino]

closes #9744

Clicking on a link without the download attribute, but returns the response header content-disposition: attachment, will trigger a download instead of an external navigation. Currently, Kit thinks it's an external navigation and that the page will unload.
EDIT: The beforeunload event is necessary for the implicit download to occur (either that or you have to add target="_blank".

This fix moves the before_navigate call out of the click event handler and solely into the beforeunload event handler (there was already one there but would check the navigating flag to prevent it from stacking with the one from the click event handler). Hence, only links that will actually unload the page (external links) will be treated as such, and not get mixed up with these implicit download links (they trigger a download but have no download attribute).
EDIT: It seems to be working fine after merging the master branch.

Notes:
Don't know if there's a way to determine if an implicit download link is a download before receiving the response headers. Therefore, SvelteKit will always run beforeNavigate and treat it as a 'link' type navigation. Maybe this is fine? The dev can always choose to make the download explicit by adding the download attribute to the link.

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: a9c1801

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

[eltigerchino]

Shoot the webkit checks are failing. Probably broke something

[eltigerchino]

Cancelling beforeunload is currently broken in Safari. It only works when the page is in an iframe.
EDIT: I have filed a bug report in the Webkit forums https://bugs.webkit.org/show_bug.cgi?id=255837

[dummdidumm]

Regarding the Safari bug: Does this introduce a new bug for Safari users that wasn't there before, or does it "just" not fix it for them? In the latter case I'd be ok with not executing that test until it's fixed in Safari.

[eltigerchino]

or does it "just" not fix it for them? In the latter case I'd be ok with not executing that test until it's fixed in Safari.

Yes, exactly that. All websites are unable to cancel beforeunload in Safari. On the bright side, it's been marked as "on their radar" and I'm hoping it's fixed soon.

[dummdidumm]

Yes, exactly that. All websites are unable to cancel beforeunload in Safari. On the bright side, it's been marked as "on their radar" and I'm hoping it's fixed soon.

If this means that this PR fixes the problem for all but safari users and there's no way to solve it differently then I'm ok to skip this test for MacOs. I'm a bit surprised though that apparently .preventDefault() of beforeunload really does not work at all? Do we have no other tests currently that check that simply closing the browser triggers our beforeunload (which would then already have failed on Safari)?

[eltigerchino]

Do we have no other tests currently that check that simply closing the browser triggers our beforeunload (which would then already have failed on Safari)?

We do have several tests. In hindsight, this changes the way navigation gets cancelled. Currently, it cancels the click event when cancel is called in beforeNavigate. This PR changes that to cancel the navigation during the beforeunload event instead (which is broken in Safari)

[dummdidumm]

Ok so that means that it does break Safari in ways it didn't before ... shit

[Rich-Harris]

Presumably this would also prevent people from providing their own cancellation UI in the case where an external link is clicked (and there's unsaved work or whatever)?

I think we need to keep the existing behaviour but somehow determine more reliably whether a beforeunload event is a direct consequence of a link click. Perhaps if we checked that document.activeElement was equal to the clicked <a> (and that no focusout events had taken place in the meantime)?

[eltigerchino]

After merging the latest changes from master this seems to be working just fine? (tested on local). Not sure which change was responsible for fixing this.
Feel free to close this PR or merge for the tests.
EDIT: I was mistaken.

[eltigerchino]

Ok I think the page.waitForEvent('download') fixes reduces the flakiness of the download test.

Small note, the current behaviour for an implicit download link is:

1. Clicking on the link is treated as a link navigation.
2. beforeNavigate runs, but we fallback to a native navigation because the endpoint route is not a page.
3. The native navigation triggers and the browser receives the response with the content-disposition: attachment header.
4. From the header, the browser knows it's a download, so the browser doesn't navigate. We also know this in SvelteKit because we changed the location.href to the download URL when navigating natively, but afterwards, the location.href is still the same. (might be bad if it wasn't a download URL but the URL we navigated to actually redirected us back to the same page?).
5. We cancel our client-side navigation and reset the navigation flags. We don't run afterNavigate because now we know it wasn't a real navigation (not sure if this is right).

[dominikg]

this has me a bit uneasy. Are there any situations where this can be true but it is not a download?

[eltigerchino]

only if native_navigation is being used to navigate to the same page. However, it's currently only used as a fallback when navigating to another page. e.g., clicking a link that has a different href than the current page

[dominikg]

hmm, what if for some reason the response redirected you back to where you came from?
Are we unable to detect that something is a download before starting a navigation that really isn't one?

[eltigerchino]

It's tricky. Some downloads are only detectable once the response headers have been downloaded and Content-Disposition: attachment is present. Another option is to revert this and let the developer be explicit about which links are downloads with the download attribute so they don't run into this issue.