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] User edit form: conditionally show the "Current password" field #3389

Open
klonos opened this Issue Nov 21, 2018 · 16 comments

Comments

Projects
None yet
5 participants
@klonos
Copy link
Member

klonos commented Nov 21, 2018

Here's what this form currently looks like:

screen shot 2018-11-21 at 2 57 50 pm

There's quite a few things that annoy me here:

  • I would prefer the all password fields to be rendered together; one after the other, without any other field in between.
  • That "Current password" field is only required if the user needs to change their email and/or their password, but it is shown always.
  • The help text under the "New password" field explains that another field is required instead of explaining what the field it belongs to does.

Lets fix all these things 😄


PR by @klonos: backdrop/backdrop#2389

@klonos klonos added this to the 1.11.3 milestone Nov 21, 2018

klonos added a commit to klonos/backdrop that referenced this issue Nov 21, 2018

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Nov 21, 2018

PR up for review...

  • The "Current password" field is hidden by default:

    screen shot 2018-11-21 at 6 14 25 pm
  • If the value of the email has been edited (a dot added in this screenshot), then the "Current password" field is shown:

    screen shot 2018-11-21 at 6 14 38 pm
  • If any character is entered in the password field, then the "Current password" field is shown:

    screen shot 2018-11-21 at 6 14 53 pm
  • The help text for the "New password" field has been changed to:

    Use this field to change your current password to a new one.

  • A red "required" asterisk is added to the "Current password" field, but due to known issues with the Form API, #states, and the 'required' state this does not do anything validation-wise, so user_validate_current_pass() is still required. See https://www.drupal.org/project/drupal/issues/2855139 https://www.drupal.org/node/2405271 https://www.drupal.org/project/conditional_fields/issues/1561272

@herbdool

This comment has been minimized.

Copy link

herbdool commented Dec 6, 2018

I don't think this is a bug, strictly speaking. It's a UX improvement. I'm also not convinced it should be changed in a patch release.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 6, 2018

Thanks @herbdool ...I have to admit that "technically" this is not a bug per se ...or not an apparent one at least, but AFAIK historically we have been permitting fixes to UX issues to be committed in patch releases; whether these issues where tagged as bugs or mere tasks (UX improvements). I could be wrong, but that is my understanding. @jenlampton ??

Having said that, the actual bug around what is going on in this form UX-wise has been marked as a major bug in d.org: #states required is not working in form API. This issue here is not trying to address the underlying bug with the Form API, but it does prevent the following UX WTF (pretend that you are not a seasoned Drupal/Backdrop user for a moment 😄):

  1. Edit your user account and change your email address
  2. Since no other field is marked as required in that form (and you don't speak Drupal/Backdrop, remember?), scroll down the page and click "Save" -> a required, but also non-required field ...WFT?!?

...hence the assignment of the bug label for this issue.

@klonos klonos modified the milestones: 1.11.3, 1.12.0 Dec 6, 2018

@klonos klonos added the help wanted label Dec 6, 2018

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 6, 2018

I brought this up during the design meeting, and the general consensus seemed to be that we should have tests for this. Pushing to 1.12.0 because I don't speak tests, and I have also added the "help wanted" tag for anyone for whom tests are their thing 😅.

@herbdool

This comment has been minimized.

Copy link

herbdool commented Dec 7, 2018

@klonos that's what I understand too: that small UX improvements can be in a minor release. However, I don't think they need to be tagged as bugs. It helps when only things that are actually broken are tagged as bugs. This helps us prioritize.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 20, 2018

We also mentioned in that meeting that we don't have any way to do testing in JavaScript - so we'll need to determine what testing we can do on the PHP side, and see if those tests aren't already being done.

I have thoughts about the text in the PR:

Use this field to change your current password to a new one.

Does this add value beyond the "New password" label? IMHO it doesn't and we should remove the description text here entirely.

Required if you want to change the email address or specify a new password. If you have forgotten your current password, you can reset it via email.

How about ...

Required to change your email address or password. If you have forgotten your current password you must reset it by email.

But we should be careful to only add If you have forgotten your current password you must reset it by email. if you are looking at your own page. Will this text appear for admins?

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 3, 2019

@klonos where do you need help with this one? Around the required-ness of items? We may be able to get "close enough" by adding a required-indicator and a required HTML attribute with javascript. LMK if you want to give that a try.

@jenlampton jenlampton removed the help wanted label Jan 3, 2019

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jan 3, 2019

I like this issue. Ideas:
I think both password fields should be hidden; there should be a "Change password" link that toggles the new and current password fields to appear.
And use JS to toggle the current password field when JS detects youve typed a full valid email into the email field, not just a dot.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 3, 2019

And use JS to toggle the current password field when JS detects youve typed a full valid email into the email field, not just a dot.

I kinda like that the password field shows up when any change is made. It's very clear this way that changing the email requires a password confirmation.

I think both password fields should be hidden; there should be a "Change password" link that toggles the new and current password fields to appear.

Can you think of any examples of sites that do this? I'd like to give it a try and see how it feels (prefereably w/o writing a PR first). I like the current PR already (but I may like this suggestion more!)

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Jan 3, 2019

Thanks for the reviews @jenlampton and @docwilmot 👍

...I have used #states for the hiding/unhiding magic on this form @docwilmot, and my JS skills are still laughable. So if we were to implement a non-#states JS toggle here, then I might have to step back and leave this to someone else.

@jenlampton I would not mind filing a second, alternative PR that implements a JS toggle. This would give any reviewers the chance to compare the UX with both implementations, and then decide which one is the most painless.

...unless I cheat and use a checkbox styled as button (like https://stackoverflow.com/questions/7642277/css-styled-a-checkbox-to-look-like-a-button-is-there-a-hover for example). That would allow me to still use #states under the hood 😈. Would such a solution be acceptable?

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jan 3, 2019

I kinda like that the password field shows up when any change is made. It's very clear this way that changing the email requires a password confirmation.

Its also annoying when fields start popping in and out of existence while your interacting with one area of the UI; you get a what-the-hell-just-happened feeling when you sense theres a change before you figure out "oh I think a new password box just appeared". Better I think, when youre done, or you remove focus, to then guide you to the next step, ideally with a message .

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Jan 4, 2019

@docwilmot's suggestion from Gitter:

Oh can we add filler text to the passowrd field, instead of just blank?
"Change password" or **

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 15, 2019

Bumping to 1.13 milestone.

@jenlampton jenlampton closed this Jan 15, 2019

@jenlampton jenlampton modified the milestones: 1.12.0, 1.13.0 Jan 15, 2019

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Jan 15, 2019

I think both password fields should be hidden; there should be a "Change password" link that toggles the new and current password fields to appear.

PR updated to add a "Change current password" checkbox:

screen shot 2019-01-15 at 11 07 56 pm

...which hides both new and current password fields unless checked:

screen shot 2019-01-15 at 11 08 04 pm

The "Current password" field is still shown if the email field has changed:

screen shot 2019-01-15 at 11 08 25 pm

I have chosen to go with a checkbox field for now, since that works with #states, and does not require any JS.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 15, 2019

oops, sorry for closing @klonos. Why is there a close and comment button anyway!

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jan 15, 2019

Agreed this probably is too much for a bugfix release, so this may have to wait for 1.13, unless we come up with something that is very minor and low-impact.

I'm not a fan of the checkbox. The "checkbox shows another field" pattern is common, but I've not seen it used on an account page like this. The combination of the checkbox OR changing email address is also a strange one for me.

Something that I'm seeing more commonly is an entirely separate page for changing sensitive information like password or email address. Here on GitHub or on Facebook you have to enter your password before you can even access the form to change your password. Perhaps we could do something like that? Some more options:

  • Block access to the entire /user/x/edit form until you enter your current password.
  • Require your current password after submitting the form, in a whole-page display. Then after entering your password save the form.
  • Show fields for email and password but disabled, but link to a completely new page, like /user/x/edit-password or /user/x/edit-email, where a password is required to change those values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment