kit icon indicating copy to clipboard operation
kit copied to clipboard

fix: ignore target=_blank links

Open dummdidumm opened this issue 2 years ago • 3 comments

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

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

dummdidumm avatar Jan 17 '23 10:01 dummdidumm

🦋 Changeset detected

Latest commit: 1ce625e50b944541a06fa368e18b72b3b611f2a0

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

changeset-bot[bot] avatar Jan 17 '23 10:01 changeset-bot[bot]

Are there other targets we should also be ignoring? I'm aware of _self and _blank and (I think) _top plus whatever other custom names you want to have if you're using frames. What do we want to have happen when someone uses target='_self'

I know we probably don't want to think too hard about people using frames, but is there another check here we could perform that would cover more cases a little better? (I'm not saying this because I'm already pretty sure there is one. I'm just asking.)

Conduitry avatar Jan 17 '23 13:01 Conduitry

I thought about this very briefly, too. From reading https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target it sounds like we should just ignore _self because it's the default, and for _top and _parent we would have to somehow find out if there's window.parent - I'll adjust the code. Better safe than revisit this later.

dummdidumm avatar Jan 17 '23 14:01 dummdidumm

Is this logic right? We want to skip beforeNavigate when links open in a new tab, which IIUC means this:

<!-- should skip beforeNavigate -->
<a href="/other" target="_blank">...</a>
<a href="/other" target="other">...</a>

<!-- should NOT skip beforeNavigate -->
<a href="/other" target="_self">...</a>
<a href="/other" target="_top">...</a>
<a href="/other" target="_parent">...</a>

Rich-Harris avatar Jan 17 '23 20:01 Rich-Harris

Actually on a closer read, I think the only change we need is to respect user-defined targets

Rich-Harris avatar Jan 17 '23 20:01 Rich-Harris

Oh right, I forgot about those, yeah the if condition needs to be adjusted

dummdidumm avatar Jan 17 '23 20:01 dummdidumm