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

Disabled <sl-button> allows execution of href #2151

Closed
lindsaym-fa opened this issue Aug 27, 2024 · 7 comments · May be fixed by TaylorBundy/swiper#3
Closed

Disabled <sl-button> allows execution of href #2151

lindsaym-fa opened this issue Aug 27, 2024 · 7 comments · May be fixed by TaylorBundy/swiper#3
Labels
bug Things that aren't working right in the library.

Comments

@lindsaym-fa
Copy link
Collaborator

Describe the bug

When sl-button has an href and is disabled, activating the button still executes the href.

To Reproduce

Steps to reproduce the behavior:

  1. Create a button with href and disabled attributes, e.g. <sl-button href="https://shoelace.style" disabled>Linked Button</sl-button>
  2. Click on the button
  3. See the href is executed

Demo

https://codepen.io/omnifides/pen/xxozBab

Browser / OS

  • OS: Mac
  • Browser: Chrome, Firefox, Safari, Edge
@lindsaym-fa lindsaym-fa added the bug Things that aren't working right in the library. label Aug 27, 2024
@claviska
Copy link
Member

claviska commented Sep 4, 2024

This could be fixed by updating the docs, as links weren't intended to be disabled and can't be (at least per spec). But if you prefer to have a disable link behavior, we should probably do what @ankitmhn proposed in #2157 to revert the link to a button.

@lindsaym-fa it's your call — which way do you want to go with this one? :)

Disclaimer: I haven't looked into the pros and cons of disabling a link (or reverting a tag which might be unexpected) from an accessibility perspective.

@claviska
Copy link
Member

claviska commented Sep 4, 2024

FWIW I already updated the WA docs to say disabled links aren't supported so, as a note to myself, I'll need to update that to match whatever we decide here.

@KonnorRogers
Copy link
Collaborator

Just gonna drop this nugget here:

https://www.scottohara.me/blog/2021/05/28/disabled-links.html

we could leave the link, and never render the href on the underlying <a> if it's disabled.

@claviska
Copy link
Member

claviska commented Sep 4, 2024

@lindsaym-fa
Copy link
Collaborator Author

lindsaym-fa commented Sep 5, 2024

Given disabled isn't a valid attribute on <a>, I'm totally happy with just updating the docs. If we made it so that the disabled styles don't apply to buttons with an href, that'd be even better. (Is there a reason we shouldn't do that?)

My biggest concern here is meeting end user expectations: users (except those with screen readers or the like) don't know when a button is a button or a link, but they would expect a disabled element to be inert. Helping devs learn that disabled link buttons aren't supported is key. Preventing it from getting to end users would be icing on the cake.

@lindsaym-fa
Copy link
Collaborator Author

lindsaym-fa commented Sep 5, 2024

#2157 is definitely an interesting alternative — we avoid subverting end user expectations and we might better meet developer expectations. The potential consequences aren't clear to me, but it seems worth looking into, imo.

ETA: Folks using assistive tech wouldn't have any way to perceive, let alone get to, a disabled button, right? I'd guess that changing the tag shouldn't have any ill effect re:accessibility, but maybe I'm missing something.

@ankitmhn
Copy link

ankitmhn commented Sep 6, 2024

we avoid subverting end user expectations and we might better meet developer expectations. The potential consequences aren't clear to me, but it seems worth looking into, imo.

That was my reasoning for reverting the tag - we don't affect the accessibility while at the same time the dev experience is met. Maybe we could update the docs to indicate that a disabled link button is rendered as a disabled ?

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

Successfully merging a pull request may close this issue.

4 participants