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-input ] sl-input with toggle-password set and disabled or readonly still allows the password toggle #745

Closed
michaelwarren1106 opened this issue May 4, 2022 · 6 comments · Fixed by #746
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@michaelwarren1106
Copy link
Contributor

michaelwarren1106 commented May 4, 2022

Describe the bug

The toggle-password button is still operational even when the sl-input is disabled or readonly which implies that no interaction is available. Imo, the toggle password feature should also be disabled in this case.

To Reproduce

Create an sl-input with toggle-password and disabled

1. <sl-input type="password" toggle-password disabled> </sl-input>
2. click the toggle icon button
3. see that it works even tough the input is supposed to be disabled.

### Demo

https://codepen.io/michaelwarren1106/pen/KKQwdow?editors=1000


### Screenshots
If applicable, add screenshots to help explain the bug.

### Browser / OS
 - OS: Mac
 - Browser Chrome
 - Browser version 100

### Additional information
Provide any additional information about the bug here.
@michaelwarren1106 michaelwarren1106 added the bug Things that aren't working right in the library. label May 4, 2022
@bunesk
Copy link
Contributor

bunesk commented May 5, 2022

In my opinion this is not a bug on a readonly input.

@michaelwarren1106
Copy link
Contributor Author

@Buni48 imo that’s worth a deeper discussion. iirc, the only real difference between disabled and readonly is that readonly inputs are still focusable whereas disabled inputs aren’t. the assumption of “non-interactive” imo is the same between these states so my thought was that being non-interactive meant that clickable other elements that appear to be “part of the input” shouldn’t be interactive either

@claviska
Copy link
Member

claviska commented May 5, 2022

the assumption of “non-interactive” imo is the same between these states so my thought was that being non-interactive meant that clickable other elements that appear to be “part of the input” shouldn’t be interactive either

In both disabled and readonly form controls, the text remains selectable and copyable. I figure those count as interactive, so hiding the clear icon makes sense, but I'm not sure about the password toggle. 🤔

@michaelwarren1106
Copy link
Contributor Author

yeah it’s a tough one. The way I tend to think about it is that the toggle is about double checking what you typed to make sure that you typed the right thing. So if the input is disabled or readonly and you can’t type anything what sense does it make to have a clickable button that allows you to double check what you typed?

@claviska
Copy link
Member

claviska commented May 5, 2022

Edge has a built-in password toggle that doesn't show when readonly. But then again, it only shows when you start typing.

The case for it showing/working in a readonly input can probably be made. For example, I may want to expose an API key that I've generated so the user can show/hide/copy it from the input.

Based on that, I'd be OK with:

  • readonly inputs: show the password button; hide the clear button
  • disabled inputs: hide both

Keep in mind the user would be able to opt-out of showing the password toggle by simply removing the attribute. They won't have a way to opt-in if we make that decision for them.

@bunesk
Copy link
Contributor

bunesk commented May 5, 2022

I also think that this solution makes sense. I've updated the PR.

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.

3 participants