Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

beforeNavigate shouldn't trigger when <a target="_blank"> #8482

Closed
oodavid opened this issue Jan 12, 2023 · 2 comments · Fixed by #8563
Closed

beforeNavigate shouldn't trigger when <a target="_blank"> #8482

oodavid opened this issue Jan 12, 2023 · 2 comments · Fixed by #8563
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@oodavid
Copy link

oodavid commented Jan 12, 2023

Describe the bugs

I've grouped together two behavioural issues as they feel related:

  1. Links that open in a new tab / window should not trigger beforeNavigate.

  2. It would be useful to be notified whether navigation.cancel() will trigger the browser beforeunload behaviour.
    Or, if we could choose whether to trigger that behaviour.
    As it stands it's not possible to know whether to handle the beforeUnload event with our own UI. In my first (naive) use of navigation.cancel(), I created my own confirmation UI in Svelte. The final result was that some navigation events would have just my UI, and others would have the native UI and my UI.

Reproduction

https://github.com/oodavid/sveltekit-beforeUnload/tree/2af0ef6271242397da1491f74608750f7c28e886 - repo (note that this is pointing at a specific commit!)

https://www.loom.com/share/3fbc000bbbaa4e65b1e64f304d0fddee - video explanation

Logs

No response

System Info

System:
    OS: macOS 12.3.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 32.05 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: 108.0.5359.124
    Edge: 108.0.1462.76
    Firefox: 105.0.1
    Safari: 15.4
  npmPackages:
    @sveltejs/adapter-auto: ^1.0.0 => 1.0.0 
    @sveltejs/kit: ^1.0.0 => 1.0.11 
    svelte: ^3.54.0 => 3.55.1 
    vite: ^4.0.0 => 4.0.4

Severity

annoyance

Additional Information

No response

@dummdidumm
Copy link
Member

dummdidumm commented Jan 16, 2023

Thanks for the thorough reproduction and the video!

beforeNavigate firing for links opening in a new tab is definitely a bug.

Navigation links not triggering an alert dialog could be found out by looking at the type property which is different for these scenarios (link/goto/form would not trigger the native dialog, and popstate only if the link in question is external). We should probably document this.

@dummdidumm dummdidumm added bug Something isn't working documentation Improvements or additions to documentation labels Jan 16, 2023
dummdidumm added a commit that referenced this issue Jan 17, 2023
Also some docs on how to know when native confirmation dialog appears
fixes #8482
Rich-Harris added a commit that referenced this issue Jan 17, 2023
* fix: ignore target=_blank links

Also some docs on how to know when native confirmation dialog appears
fixes #8482

* handle iframe case

* handle custom targets, simplify

* Update packages/kit/src/runtime/client/client.js

* Update packages/kit/types/ambient.d.ts

Co-authored-by: Rich Harris <[email protected]>
Co-authored-by: Rich Harris <[email protected]>
@AbstractFruitFactory
Copy link

Hi, I actually really want this to work for target=_blank links. Is this still possible somehow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants