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

<sl-button href target> ignores target when opening a new tab/window. #1200

Closed
CetinSert opened this issue Feb 16, 2023 · 7 comments
Closed
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@CetinSert
Copy link

CetinSert commented Feb 16, 2023

Describe the bug

<sl-button href target> ignores target on first click.

To Reproduce

Steps to reproduce the behavior:

  1. Go to sl.rt.ht/1200/? in a new browser window
  2. Click on SlButton N times
  3. You will see N tabs open ❌

vs

  1. Go to sl.rt.ht/1200/? in a new browser window
  2. Click on Anchor N times
  3. You will see 1 tab open ✔️

Demo

See above.

Screenshots

N/A.

Browser / OS

  • OS: all
  • Browser: all
  • Browser version: all

Additional information

<sl-button href target> cannot initiate a new tab with a new target set.
Once you open a tab with target properly defined, <sl-button href target> does manage to target it just fine.

@CetinSert CetinSert added the bug Things that aren't working right in the library. label Feb 16, 2023
@CetinSert
Copy link
Author

CetinSert commented Feb 16, 2023

Glad you are looking into this. I hope you can reproduce it easily. Let me know.
This has nothing to do with the playground and its output <iframe>.
You can just also visit https://sl.rt.ht/1020 for the pure output in its top level window. The issue stays the same.

@claviska
Copy link
Member

No problem. I'm trying to see if this is a Shoelace issue or a browser bug. The underlying anchor is rendered correctly, so I suspect it may be the latter or something related to the target id being referenced in a different document (shadow root rather than the main document).

Usually, browser bugs are only replicable in one browser. I'm seeing the same behavior in Chrome, Safari, and Firefox, so this may be per spec. Still looking into it.

@claviska
Copy link
Member

It seems to work fine in this example.

https://codepen.io/claviska/pen/dyqobOL?editors=1010

@CetinSert
Copy link
Author

Indeed, Chrome was showing a click event handler <sl-button> defined on the internal <a> if I remember correctly. Perhaps it is messing with something o__O? I might be totally wrong though.

@claviska
Copy link
Member

Looks like this happens because of rel="noreferrer noopener", which is a security measure added for convenience because of this vulnerability.

I don't think we want to stop doing this by default, as it's likely protecting a lot of people from exploits. Perhaps we should add rel to <sl-button> and default it to noreferrer noopener and let folks override it if they want a different value, including "".

@CetinSert
Copy link
Author

CetinSert commented Feb 16, 2023

Great find! Extended sl.rt.ht/1200/? with a light-DOM <a href target rel="noreferrer noopener"> and it exhibits the same behavior indeed.

@CetinSert CetinSert changed the title <sl-button href target> ignores target on first click. <sl-button href target> ignores target when opening a new tab/window. Feb 16, 2023
@claviska
Copy link
Member

Fixed in 1bc2a6e. The rel attribute will default to the existing, hardcoded value for security reasons. Users can choose to override it by providing their own value.

<sl-button href="https://example.com/" target="_blank">
  rel = noreferrer noopener
</sl-button>

<sl-button href="https://example.com/" target="_blank" rel="">
  rel = (empty)
</sl-button>

<sl-button href="https://example.com/" target="_blank" rel="test">
  rel = test
</sl-button>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

2 participants