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

[UX] Tab index: Password toggle should come after password field #4524

Closed
klonos opened this issue Aug 7, 2020 · 6 comments · Fixed by backdrop/backdrop#3220
Closed

[UX] Tab index: Password toggle should come after password field #4524

klonos opened this issue Aug 7, 2020 · 6 comments · Fixed by backdrop/backdrop#3220

Comments

@klonos
Copy link
Member

klonos commented Aug 7, 2020

This is a follow-up to #3048

Before that change, you could do this:

  1. Focus the username field (won't be needed after [UX] Automatically focus username field on login page #4183 😉 )
  2. Type your username.
  3. Press tab -> the password field gets focus
  4. Type your password
  5. Press enter -> log in

After #3048 was merged, you need to tab twice after step 2:

  1. Focus the username field
  2. Type your username.
  3. Press tab -> the "show password" link gets focus
  4. Press tab again -> the password field gets focus
  5. Type your password
  6. Press enter -> log in
@klonos
Copy link
Member Author

klonos commented Aug 7, 2020

...I'm thinking that this may need to be a wider change 🤔 ...do we in general (not specifically to the login form) want the tab index for the show/hide toggle to have priority over the actual password text field, or should we have it so that the toggle always comes after the password field?

@klonos klonos changed the title Regression in login form: Make sure that the tab order of the password field comes after the username [UX] Tab index: Password toggle should come after password field Aug 7, 2020
@klonos
Copy link
Member Author

klonos commented Aug 7, 2020

Simple one-line PR up for review: backdrop/backdrop#3220

Before, the .password-toggle link is rendered before the password input:

<div class="form-item form-type-password form-item-pass">
  <label for="edit-pass">Password
    <abbr class="form-required" title="This field is required.">*</abbr>
  </label>
  <span class="password-toggle-wrapper password-hidden">
    <a href="#" class="password-toggle">Show password</a>
    <input data-password-toggle="..." type="password" id="edit-pass" name="pass" size="60" maxlength="128" class="form-text required password-toggle-processed" autocomplete="off" style="...">
  </span>
</div>

After, the .password-toggle link is rendered after the password input (visually, CSS-wise nothing changes - the toggle is still shown at the top-right of the password field):

<div class="form-item form-type-password form-item-pass">
  <label for="edit-pass">Password
    <abbr class="form-required" title="This field is required.">*</abbr>
  </label>
  <span class="password-toggle-wrapper password-hidden">
    <input data-password-toggle="..." type="password" id="edit-pass" name="pass" size="60" maxlength="128" class="form-text required password-toggle-processed" autocomplete="off" style="...">
    <a href="#" class="password-toggle">Show password</a>
  </span>
</div>

@ghost
Copy link

ghost commented Aug 8, 2020

should we have it so that the toggle always comes after the password field?

Yes, I believe so.

The PR works nicely and is beautifully simple :-) RTBC from me.

@ghost
Copy link

ghost commented Aug 8, 2020

I added the milestone candidate - bug label as this fixes a slight UX regression introduced by the other linked issue.

@klonos klonos added this to the 1.16.3 milestone Aug 8, 2020
@quicksketch
Copy link
Member

quicksketch commented Aug 13, 2020

This sounds great! I frequently accidentally focus "Show password" when tabbing between the username and password field.

@quicksketch
Copy link
Member

I merged backdrop/backdrop#3220 into 1.x and 1.16.x. Thanks @klonos and @BWPanda!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants