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

[teemingc]

closes https://github.com/sveltejs/kit/issues/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:

• [x] 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
• [x] This message body should clearly illustrate what problems it solves.
• [x] Ideally, include a test that fails without this PR but passes with it.

Tests

• [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• [x] 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[bot]]

🦋 Changeset detected

Latest commit: a9c18010af9b9cf74787bc0a8e652b1fe4d7d2d0

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

[teemingc]

Shoot the webkit checks are failing. Probably broke something

[teemingc]

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.

[teemingc]

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)?

[teemingc]

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)?

[teemingc]

~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.

[Rich-Harris]

Unfortunately the test is flawed — because /before-navigate/download is an internal link, the failure state isn't reached. I've tried to update the test to point at an external URL (that we manage inside the test) but I can't seem to get it to run.

[teemingc]

I've tried to update the test to point at an external URL (that we manage inside the test) but I can't seem to get it to run.

I was incorrectly preventing cancel() from being called for the download link, so the download would never occur. After fixing the test, it shows that the issue was not actually fixed.

I've added a simple fix of resetting the navigation flag after the beforeunload event to ensure that future navigations run the beforeNavigate callbacks. I've just learned that the beforeunload event is essential for the download to occur.

[teemingc]

The Ubuntu tests are failing because Firefox is navigating to the download URL instead of only downloading the response body. I tried reproducing this on an actual Ubuntu machine and it does not do this. Not sure how to proceed from here. @Rich-Harris would replacing the download URL with the external one help get around this quirky behaviour?

[dummdidumm]

Stupid idea: Have a timeout running on external link clicks which resets the navigation after x seconds if it wasn't updated in the meantime. If the user is still there, it's probably correct to reset. If not, the whole thing is unloaded anyway.

[teemingc]

I've no idea why the tests are so flaky even with the hacky "wait for idle network" statements. Would like some help with this or maybe I'll take another stab at it after learning how to properly write playwright tests :/ I suspect the tests are flaky because they're checking the navigation results too early. Maybe I should add a conditional element that only appears when $navigating === false so that the test locator will wait the right amount of time?

Also:

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. It should be fine to treat <a href="/somewhere"> as a navigation link even if it really is a download because of a response header.

[teemingc]

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).