Created PasswordInput component with visibility button (eye icon)

[balsa-asanovic]

Problem

When typing the Wi-Fi password user does not have a way to see what was actually typed in, since the password is hidden by default.

fixes #242

Solution

A new component, PasswordInput, has been created. It includes both a text input field and a visibility button with an eye icon.

When the visibility button is clicked, it toggles the text input's form type from 'password' to 'text,' making the characters visible to the user.

This component is now utilized wherever the input type 'password' was previously employed.

As of the time of writing, this applies to the following components:

src/components/storage/iscsi/AuthFields.jsx
src/components/questions/LuksActivationQuestion.jsx
src/components/network/WifiConnectionForm.jsx
src/components/core/PasswordAndConfirmationInput.jsx

Also this PR now fixes #760 after the issue was raised while making reviews here.

Testing

Added unit test for the new component in file PasswordInput.test.jsx.

Also tested manually.

Screenshots

Password input in Users menu

Wi-Fi password input

iSCSI storage password input

Luks activation question password input

[dgdavid]

Hi Balsa!

To begin with, thanks a lot for this new contribution! Clearly, it resolves #242.

There are, however, a few changes I would like to suggest:

• Use the "eye icon pattern"

  I.e., to put an eye icon close to the password input for revealing or hiding its content. Similar to the Cockpit's login page implements it.

  An example of how to do it with PatternFly is here and you can use the "Visibility" and "Visibility off" icons provided by material symbols importing them in the Icon component.

• Opting for a toggle action rather than a "momentary" one

  I.e., make the password input visible (or not) upon clicking.

Hope you don't mind, but I think it's a well-known, slightly better approach.

[balsa-asanovic]

Hope you don't mind, but I think it's a well-known, slightly better approach.

Hey David,

Don't mind at all, in fact thank you for taking the time to point me in the right direction.

I've made some commits in accordance with your comments, so check it out and let me know if there is anything more I should do regarding this.

I noticed something that bothered me a bit but wasn't sure how to fix it—the horizontal alignment of the eye icon within the button doesn't seem quite centered; it's slightly shifted towards the top. It might not be a big deal, but I thought I'd point it out.

Testing pics:

[dgdavid]

Don't mind at all, in fact thank you for taking the time to point me in the right direction.

I've made some commits in accordance with your comments, so check it out and let me know if there is anything more I should do regarding this.

Wow! That was really fast! Thank you!

I've added a few comments more. Once you address them, we can continue working to go a a bit further 😇 is you're willing to do so 😉

I noticed something that bothered me a bit but wasn't sure how to fix it—the horizontal alignment of the eye icon within the button doesn't seem quite centered; it's slightly shifted towards the top. It might not be a big deal, but I thought I'd point it out.

The quick fix that comes to my mind now is to add a "display: flex" to the span.pf-c-button__icon wrapping the icon. Just add below lines to the end of src/assets/styles/patternfly-overrides.scss file

button#password_visibility span.pf-c-button__icon {
  display: flex;
}

We can go for a better solution later, once the migration to PatternFly 5 is done.

[dgdavid]

Hey David,

I've went through your comments, I think I covered them all.

Nitpicking: what about using an internal component instead?

Here, I chose the second option, without the internal component. No particular reason, I've just went with the option which had a line of code less 😄 If you think that using internal component is a better choice, feel free to mention it and I'll update code accordingly.

No strong opinion from my side. Your choice is ok. Let's save lines of code until they are really needed 😉

Once you address them, we can continue working to go a a bit further 😇 is you're willing to do so 😉

Certainly, keep 'em coming. 😄

Great!

So, to be clear: from my point of view, the PR is ready to be merged as it is now. However, with very little effort, we can improve it a lot for extending that behavior to all password inputs. The idea is

• Create a src/components/core/PasswordInput.jsx component

  • And move the logic dealing with the password input and its reveal button there.

  • By now, I'd just forward received props to the internal TextInput but overriding the ones needed, like type. Something like

    export default function PasswordInput({ props }) {
       ...
    
      return (
         <>
           <TextInput {...props} type={type} />
      ...

• Add simple tests for that new component at src/components/core/PasswordInput.test.jsx file

  • Note that, although the philosophy of React Testing Library is about testing the application components in the way the user would use it, here we do not have a way to satisfy it (or at least I do not know how to do it 😜).

    Thus, instead of checking that the entered password is not in the screen before clicking the "Show Password" we will have to directly check the type attribute of the input before and after such an action.

    Definitely no in the way the user would use it, but looks like there are some caveats with the password input yet :) See [Role Parity] What do we do about input (type=password)? w3c/aria#935 and What do we recommend people do for elements that have no implicit role (like input[type=password]) testing-library/dom-testing-library#567.

    Something like below should work

    const passwordInput = screen.getByLabelText("Password");
    const button = screen.getByText("Show Password");
    
    expect(passwordInput).toHaveAttribute("type", "password");
    await user.click(button);
    expect(passwordInput).toHaveAttribute("type", "text")

• Replace all password inputs by the new PasswordInput component to make the UI more consistent and allow the user to reveal desired password, not only these related to a WiFi network.

  ❯ grep -rl "type=\"password" src/
  src/components/storage/iscsi/AuthFields.jsx
  src/components/questions/LuksActivationQuestion.jsx
  src/components/network/WifiConnectionForm.jsx
  src/components/core/PasswordAndConfirmationInput.jsx

• Check everything works

• Change the PR title

• Update the PR description

• Adapt the changes file

Do you accept the challenge?

[dgdavid]

Do you accept the challenge?

Of course, you can ping us at any moment you have doubts or need help. We can even jump into the PR for writing the unit test if you get stuck there.

[balsa-asanovic]

Do you accept the challenge?

Accepted, I'll get on it. Also thanks for the pointers, they certainly help a lot.

[joseivanlopez]

I'm just not sure if using this widget makes sense in the first screenshot. The one with "password" and "password confirmation". It makes a lot of sense in all the others (iSCSI and WiFi), in which you are providing a previously set password. But I find combining a "password confirmation" with a password reveal to be weird.

Just my 2 cents about this topic. The "password confirmation" ensures you have typed the same in both inputs, and the "visibility icon" allows you to check you have typed what you really wanted. For me they are complementary. And IMHO, the "visibility icon" is even more important than the confirmation input. Reading the password is the safest way to check that I have really entered it correctly . In fact, having a confirmation is usually useless for me. Many times I write the password correctly but the confirmation wrongly. So having only the password input and the option to read it would be even faster and more secure for me.

And using autogenerated password would make the confirmation even more useless (you probably copy-paste in both inputs :-P).

It is only a reflection. I am not suggesting to do this ;-).

[balsa-asanovic]

Believe me, I don't want to bother you with the code-review and suggestions. Far from it. However, I requested a new set of changes 😅 😅 😅

Haha, why not, glad to clean up 😄

I think I covered all suggested changes.
Regarding the unit tests where you believed they are probably tested as a part of components' library, I removed them.

Also, I addressed #760 while working on PasswordConfirmation component, so split prop is gone along with its wrapper.

Additionally, with the help of @joseivanlopez comment

I think the easiest way is to encrypt any device (disk or partition). If you are using a virtual machine, you can simply add a new disk (e.g., /dev/sdb) and then run the following command:

cryptsetup -y -v luksFormat /dev/sdb

After that, restart Agama (or change the product in the product selection page) and the encryption password popup should be shown.

Note: if you either skip or introduce the correct password, then the dialog is not shown anymore (you have to restart Agama to see it again).

I succeeded in reaching Luks Activation Question component and testing the password input.
Attached screenshots for this in PR description.

[dgdavid]

@balsa-asanovic

From my point of view, it looks pretty good now. Thanks a lot for such a GREAT job

I've added a few minor suggestions, but the PR is ok as it is.

Also thanks to @nobkd for reviewing and contributing to make the PR even better.

[balsa-asanovic]

Thanks a lot for such a GREAT job

Don't mention it, anytime.

Added documentation comments and extracted the id prop.