Move "show password" checkbox on login form

[di]

From #6040:

@anowlcalledjosh:

This means that the password field now takes two Tab-presses to reach from the username field rather than one, which breaks at least one popular password manager.

If the accessibility issue is more important than avoiding some manual configuration of password managers, then fine (I'm not qualified to make that decision), but the original change to add tabindexes was intentional – see #2824, #2917.

@di:

This has actually been bugging me too. I think we can fix it without adding tab indexes by moving the "Show password" box under the password field.

@nlhkabu @woodruffw Thoughts?

@woodruffw:

Yep, I think this would be a good solution.

---

Good First Issue: This issue is good for first time contributors. If you've already contributed to Warehouse, work on another issue without this label instead. If there is not a corresponding pull request for this issue, it is up for grabs. For directions for getting set up, see our Getting Started Guide. If you are working on this issue and have questions, feel free to ask them here, #pypa-dev on Freenode, or the pypa-dev mailing list.

[woodruffw]

Taking a stab at this.

[di]

@woodruffw Looks like we might already have a PR: #6755

[woodruffw]

Whoops! Ignore me then 😅

[di]

Although heads up @ddaan, that doesn't quite do what was described in this issue. We don't want to use tabindex.

[woodruffw]

Dropping a non-tabindex diff for reference (the other pages also need to be changed):

diff --git a/warehouse/templates/accounts/login.html b/warehouse/templates/accounts/login.html
index 56a5e327..ab676b3a 100644
--- a/warehouse/templates/accounts/login.html
+++ b/warehouse/templates/accounts/login.html
@@ -64,17 +64,13 @@
         </div>
 
         <div data-controller="password" class="form-group">
-          <div class="split-layout">
+          <div>
             <label for="password" class="form-group__label">
               {% trans %}Password{% endtrans %}
               {% if form.password.flags.required %}
               <span class="form-group__required">{% trans %}(required){% endtrans %}</span>
               {% endif %}
             </label>
-            <label for="show-password">
-              <input data-action="change->password#togglePasswords" data-target="password.showPassword"
-                id="show-password" type="checkbox">&nbsp;{% trans %}Show password{% endtrans %}
-            </label>
           </div>
           {{ form.password(placeholder=gettext("Your password"), required="required", class_="form-group__input", autocomplete="current-password", spellcheck="false", data_target="password.password", **{"aria-describedby":"password-errors"}) }}
           <div id="password-errors">
@@ -86,6 +82,12 @@
             </ul>
             {% endif %}
           </div>
+          <div>
+            <label for="show-password">
+              <input data-action="change->password#togglePasswords" data-target="password.showPassword"
+                id="show-password" type="checkbox">&nbsp;{% trans %}Show password{% endtrans %}
+            </label>
+          </div>
         </div>
 
         <div class="form-group">

[ddaan]

Although heads up @ddaan, that doesn't quite do what was described in this issue. We don't want to use tabindex.

Oops, I totally missed the word "without" 🙈 . Moving the checkbox down would still make you tab an extra time to the login button. This could be avoided by hitting the enter button, but it will be an issue for the registration form.

[bittner]

I'd suggest to use an "eye" icon (visually) placed inside the password field to show and hide the typed password (example). Not sure if you already have this on the radar; I can't see it from the code sample.

[SanketDG]

I will be taking this up. Since Warehouse already uses FontAwesome, it'd be easy to integrate this as part of the password input itself.

[di]

I think we want to keep the existing checkbox and "Show password" label for accessibility reasons. We just want to move it. The diff in #6722 (comment) looks like what we're looking for if someone can turn it into a PR.

[bittner]

It should be feasible to both keep the checkbox, technically, for accessibility reasons and show a clickable 👁️ icon, which operates the checkbox, to everyone else who is not a screen reader application. I think we're looking for that as a solution.

The visual representation of a checkbox misleads people into thinking "Remember on this computer" (the common use case for login forms involving a checkbox). This is dangerous, because users may automatically check the control as a habit, which will in turn show the password -- not nice in a screen sharing session! 💣

[nlhkabu]

This sounds like a good solution @bittner.

I agree that the eye icon is ideal, so if we can implement this without affecting accessibility (screen readers, tab order for keyboard users) that would be great.

[bittner]

If you need any help with the implementation please let me know!

[nlhkabu]

If you want to make a start @bittner, that would be great.

@di is also offering pairing sessions, if you want to walk through this with another dev: https://twitter.com/di_codes/status/1245079525156847616 :)

[bittner]

Thanks for the hints on pairing. (I installed the software he mentions in the tweet for now and tried to bring up the project with docker-compose.) I took a look at the code. If we could use plain CSS the solution could be elegant:

• #password[type=text] + ... ➜ class="fa fa-eye-slash"
• #password[type=password] + ... ➜ class="fa fa-eye"

With FontAwesome, since we must use their classes in the HTML, we'd need to set the classes with JavaScript, which most likely will mix data logic and presentation (e.g. in password_controller.js).