`beforeNavigate` doesn't fire after clicking on anchor tag that has `content-disposition: attachment` in header response

lukearray commented 1 year ago

Describe the bug

This application has two pages - a home page at / and an about page at /about. You can click between the two pages using the links in the header, or the links that appear on each page. Each time you click, BEFORE NAVIGATE IS RUNNING logs to the console. If you click the PDF Url link at the bottom, beforeNavigate runs as normal, logging BEFORE NAVIGATE IS RUNNING to the console. The next link you click, beforeNavigate does not run.

I believe this happens because the content-disposition:attachment header is set in the response, causing the browser to download the file. By this point, I guess SvelteKit assumes the page navigation happens, and so missed the next beforeNavigate.

If you change the link from <a href="https://dl.dropbox.com/s/xw52umod2vx850t/pdf-test.pdf?dl=0">PDF Url</a> to <a href="https://www.dropbox.com/s/xw52umod2vx850t/pdf-test.pdf?dl=0">PDF Url</a> (ie, change subdomain to www from dl), then the header is not set, and the behaviour works as expected.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-skdwaq?file=src/routes/+layout.svelte

You need to use 'Open in New Tab' in the topright of the stackblitz link to get this demonstration to work.

Logs

No response

System Info

❯ npx envinfo --system --binaries --browsers --npmPackages "{svelte,@sveltejs/*,vite}"
success Install finished in 1.129s

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
  npmPackages:
    @sveltejs/adapter-auto: ^2.0.0 => 2.0.0 
    @sveltejs/kit: 1.15.7 => 1.15.7 
    svelte: ^3.54.0 => 3.58.0 
    vite: ^4.2.0 => 4.2.1

Severity

serious, but I can work around it

Additional Information

No response

lukearray commented 1 year ago

@s3812497 As we continue to test the fixes, we keep finding more issues!

Now that the download attribute issue has been resolved, we are still seeing issues with beforeNavigate. I have provided a reproduction for this that doesn't rely on the download attribute at all, and hopefully have now identified the root cause.

eltigerchino commented 1 year ago

Thanks for reporting this again! This issue is due to an oversight on my part. I didn't know downloads could occur with the content-dispositon header, and was only checking for the download attribute + same origin URL. https://github.com/sveltejs/kit/blob/3a8195dfe0f55c024b7b347e74c6e21480bda5bd/packages/kit/src/runtime/client/utils.js#L138

I'm thinking a solution could be to reset the internal navigating variable whenever the URL has changed. This would ensure that beforeNavigate runs on any navigation. Currently, navigating is only reset when a full navigation completes or is cancelled. That's why we're having so much trouble with downloads (it doesn't navigate anywhere).

sveltejs / kit