Are we correctly using the icon for the password visibility action?

[dgdavid]

Before going into details, please make the effort to look at the picture below as if you were in front of this kind of form for the first time ever.

Look at the open, not crossed-out eye icon and answer sincerely: what is it telling you?

• a) the password input is in the "show" state, displaying introduced characters
• b) the password input is in the "hide" state, obscuring introduced characters

While I requested the use of this eye pattern in the code review, I found myself giving the first answer when doing manual testing. It was like, "Wait, is the dialog opening with the password inputs in the show state?".

Instead of opening the discussion in the PR itself and putting it at risk of getting blocked for a while, I decided to take notes and open a discussion here because there are a lot of web, desktop, and even mobile applications out there using the same "eye pattern". Without going any further, the Cockpit's login page uses it as we do. And we often don't question things that are widely used.

However, I think worth it to rethink the whole thing a bit. In the worst case (to me) we could end up keeping it as it is.

The first idea that came to my mind was proposing to use the icon the other way around: the crossed out variant when the input is obscuring the password and the other when not. But you could argue that it might be a bad idea to go against the grain. And you would be right: We shouldn't use a wide use "icon action" with a different meaning than the rest of the world... even though they are all wrong 😜

So, as an alternative and similar to the first approach sent by @balsa-asanovic when developing the feature, we could use a plain text action. I.e., to use the "Show" and "Hide" texts instead of icons. But we will be in the same dilemma: is the text representing the current input state, or is the action to be performed when clicking on it?

But, if the dilemma is about displaying the input status or the next action, could we solve it by displaying a visual hint in sync with the current status... 🤔 🤔 An old plain HTML checkbox with its label? A PatternFly Switch?

Inline, long label version	Inline, short label version	In the next line

Click to show/hide a rough diff

---

diff --git a/web/src/assets/styles/patternfly-overrides.scss b/web/src/assets/styles/patternfly-overrides.scss
index 6b43b8ac..2e4ad2a4 100644
--- a/web/src/assets/styles/patternfly-overrides.scss
+++ b/web/src/assets/styles/patternfly-overrides.scss
@@ -186,7 +186,8 @@ table td > .pf-v5-c-empty-state {
   }
 }

-// Center icon in the visibility button of password input form fields
-.password-toggler span.pf-c-button__icon {
-  display: flex;
+// Let's InputGroupText be as wide as its content (motivated by Show Password
+// switch)
+.pf-v5-c-input-group__item.pf-m-box {
+  min-inline-size: fit-content;
 }
diff --git a/web/src/components/core/PasswordInput.jsx b/web/src/components/core/PasswordInput.jsx
index 14a55ab5..b7452ea2 100644
--- a/web/src/components/core/PasswordInput.jsx
+++ b/web/src/components/core/PasswordInput.jsx
@@ -30,16 +30,15 @@
  */
 import React, { useState } from "react";
 import {
-  Button,
   InputGroup,
-  TextInput
+  InputGroupText,
+  TextInput,
+  Switch
 } from "@patternfly/react-core";
-import { Icon } from "~/components/layout";
 import { _ } from "~/i18n";

 export default function PasswordInput({ id, ...props }) {
   const [showPassword, setShowPassword] = useState(false);
-  const visibilityIconName = showPassword ? "visibility_off" : "visibility";

   if (!id) {
     const field = props.label || props["aria-label"] || props.name;
@@ -53,15 +52,15 @@ export default function PasswordInput({ id, ...props }) {
         id={id}
         type={showPassword ? 'text' : 'password'}
       />
-      <Button
-        id={`toggle-${id}-visibility`}
-        className="password-toggler"
-        aria-label={_("Password visibility button")}
-        variant="control"
-        onClick={() => setShowPassword((prev) => !prev)}
-        icon={<Icon name={visibilityIconName} size="15" />}
-        isDisabled={props.isDisabled}
-      />
+      <InputGroupText>
+        <Switch
+          id={`toggle-${id}-visibility`}
+          label={_("Show password")}
+          isChecked={showPassword}
+          onChange={() => setShowPassword((prev) => !prev)}
+          isDisabled={props.isDisabled}
+        />
+      </InputGroupText>
     </InputGroup>
   );
 }

Examples above inspired by https://joshwayne.com/posts/show-password/ article and a Friday afternoon chat with @joseivanlopez

---

Fun fact,

Do you know what? While doing a quick research (actually when writing this question) I found something interesting: a very big company was using the icon in the way I was about to suggest above, but if you go to its login form right now you will see a plain HTML "Show password" checkbox there (aka "Mostrar contraseña") . Which might mean something.

[joseivanlopez]

You see, Google already realized everyone was using it wrongly. And not to mention the password confirmation, but that is for another discussion :).

[dgdavid]

but that is for another discussion :)

Already started 🙄 #767

I fear dropping it is not so straightforward for our use case.

[balsa-asanovic]

Look at the open, not crossed-out eye icon and answer sincerely: what is it telling you?

a) the password input is in the "show" state, displaying introduced characters
b) the password input is in the "hide" state, obscuring introduced characters

Indeed, when looking at it with blank slate, the eye would intuitively indicate that you can see the password, while the crossed eye would be telling me that I can't.

And you would be right: We shouldn't use a wide use "icon action" with a different meaning than the rest of the world... even though they are all wrong 😜

Haha, agreed.

I personally like the approach you suggested, using switch/checkbox, since this makes it quite clear that if the switch is ON, the password should be visible, and the other way around.

I prefer the "Inline, short label version" and after it the "In the next line", while I like the least "Inline, long label version", though this is purely based on some subjective sense of taste, so I could be very wrong 😄 .

If it is decided to go on with this change, I'd of course be willing to work on it. However, you did already provide most of the code. 😅

[dgdavid]

I prefer the "Inline, short label version" and after it the "In the next line"

I had exactly the same preference as you, but the weird thing with the PF/InputGroup#aria-describedby attribute and the thing with the tab order (see #765) made me change my mind. Now I'd go with the "[ ] Show password" node in the next line whenever we manage to make it the last thing of the form.

But this is not about my preference 😉 That's why I opened the discussion.

Thanks a lot for your insights, much appreciated. Really.

[dgdavid]

If it is decided to go on with this change, I'd of course be willing to work on it.

🚀 🎉

However, you did already provide most of the code. 😅

Nah, just an example. There is work to do, like adjusting it properly and writing/doing some tests. And if you are willing to do so... 😉

But first, let's see what's the conclusion of this discussion. We have to wait a bit for gathering more feedback / ideas.

[balsa-asanovic]

Of course, let's see where the discussion takes us.