Password Field Style Update

Needs final design specs

Ready for implementation

Might want to prepare a show/hide version of this field, see related issue.

@nod_ it's updated, thanks

Initial implementation of Password field style update.

We're missing in Figma one of the levels of Password strength.
In Figma we have 4 options:

• (none)
• Weak
• Normal
• Strong

In Drupal setup we have 5 options:

• (none)
• Weak
• Fair
• Good
• Strong

Unless @saschaeggi says the opposite, I would use the same colour for good and for the strong one for now.

Re #8:

Checked only in Chrome.

1. RTL password tips are wrong.
2. It would be nice just reusing the small component from #3023304: Progress bar style updates (if it will be ready) instead of repeating it.

Agreed, would use the same green for Good and Strong for now.

Test module: https://github.com/zolhorvath/password

@huzooka i added patch in progress bar component with required states
https://www.drupal.org/project/claro/issues/3023304#comment-13034412
so maybe to postpone it until progress bar will merged?

Hello!
Here we have one small problem with reusing same progressbar classes because functional js is managed on core level
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/user/u...

From my point of view it is better to keep it in core. And 2 options we have now:
1. Use some postcss extend plugin to extend progressbar classes.
2. Reuse progressbar variables. (done in this patch).

Another problem i met is the wrong alignment in user form globally. Should be follow up issue for it.

Moved color definitions to top and used generic names. Removed password field styles from progress.css. Fixed initial state (zero width), is-weak now starts at 3%. Added reduce motion and RTL rules.

+++ b/css/src/base/variables.css
@@ -30,6 +30,19 @@
+  --color-border-is-fair: #977405;

Shouldn't this color code be a variable?

Aside from this I reviewed and tested #16 and all looks fine to me.

The color of the border for fair strength is a one-off color, so I think it's fine it's not on its own variable.

I already re-rolled adding a separate color, I think it is also valid to have that reusable if the need arises.

Also created an issue relative to the 'weak' indicator when the password field is empty and not being edited: https://www.drupal.org/project/drupal/issues/3060848

I'm reviewing this

1. +++ b/css/src/base/variables.css
   @@ -17,6 +17,7 @@
   +  --color-sunglow-shaded: #977405;
   
   @@ -30,6 +31,19 @@
   +  --color-border-is-fair: var(--color-sunglow-shaded);
   

   Since this color is out of Claro's color palette, I won't name it explicitly.

2. +++ b/css/src/base/variables.css
   @@ -201,22 +215,12 @@
   +  --font-weight-progress-bar-label: 600;
   
   +++ b/css/src/components/progress.css
   @@ -37,33 +37,16 @@
   -  font-weight: 600;
   +  font-weight: var(--font-weight-progress-bar-label);
   

   According to Figma, the weight of the progress bar label is bold (it's equal to 700).

3. +++ b/css/src/components/user.css
   @@ -0,0 +1,107 @@
   +  border: 1px solid;
   +  border-color: var(--color-progress-track-border);
   

   Why not using the shorthand here? 1px solid var(--color-progress-track-border)

4. +++ b/css/src/components/user.css
   @@ -0,0 +1,107 @@
   +  color: var(--color-progress-bar-description);
   ...
   +  color: var(--color-progress-bar-description);
   

   Wrong text color: the --color-progress-bar-description variable equals to #8e929c but we need Text Light – Davy's grey: #545560.

5. +++ b/css/src/components/user.css
   @@ -0,0 +1,107 @@
   +  margin-top: var(--space-l);
   

   Instead of this value we need a negative margin (or an equivalent solution) for this element to match the design. Also the bottom margin has to be reduced.

6. +++ b/css/src/components/user.css
   @@ -0,0 +1,107 @@
   +  border: var(--size-details-border) solid var(--color-details-border);
   

   Wrong border-color: the variable used here is equal to rgba(216,217,224,0.8), but according to Figma we need Light Gray: #d8d9e0.

7. +++ b/css/src/components/user.css
   @@ -0,0 +1,107 @@
   +.password-suggestions ul {
   +  margin-bottom: 0;
   +}
   

   According to Figma, we need a bigger top margin for this list.

8. +++ b/css/src/components/user.css
   @@ -0,0 +1,107 @@
   +.password-confirm,
   +.password-field,
   +.password-strength,
   +.password-confirm-match {
   +  width: 25rem;
   +}
   ...
   +.confirm-parent,
   +.password-parent,
   +.password-suggestions {
   +  max-width: 25rem;
   

   I'd define the width for the whole widget (.form-type--password-confirm) and not independently for its elements.

Thanks for the review, I adressed all points.

I'm reviewing the patch from #23.

Re #24:

I found an issue:

Accidentally, the password confirm input's font-size is decreased by the .password-confirm selector. Sadly, both the input and the password-match message div has that CSS class (even the js- prefixed CSS class).

It's obvious that the intention was to decrease the font-size of the message only.

Browsing the code of the user.es6.js it's obvious that:

• It should't use the same CSS classes for these different components.
• It should provide and use JavaScript themes (Drupal.theme functions) for it's components

I think that we have to create issues for these; and until those are fixed and committed, we have to replace the user(.es6).js with our own implementation that at least uses different CSS classes for the password match message (e.g. password-match-message or something like that).

I'll have a look on this one.

@huzooka As I totally agree with you and see the downsides that this is not such a good behavior, I would not wipe our JS logic and add two missing values which are fixing the problem right on the input itself. As we have a lot of custom code for the password field anyway, I don't see a problem to go the easy way and add them. The specificity of the selector get the work done and overrides the lower specific definition as usual.

This approach alters the password-confirm class on the confirm message to password-confirm-message and applies the styling to this new selector.

I just created an issue in core with @huzooka suggestions: #3061265: Use specific class for password confirm message

I'm confused about the scope of this issue. I don't think the problem is well explained.

Drupal functionality have moved on since then.

What functionality does this refer to, and how/why does the new design need to address it?

Re #31: I guess that sentence refers to the whole Claro theme, and not this issue.

This is only a re-roll of #28, no fixes/improvements.

Todo:

1. Fix CS of the new js/user.es6.js
2. Replace the original JavaScript with the new one (both patch from #28 and the attached one provides it as an orphan / unused asset)
3. Improve the replacement JS with additional CSS classes and use them instead of element selector(s) .password-confirm-message span, .password-suggestions ul or .form-type--password-confirm input.
4. Apply the new CSS variable naming scheme

This should match our standards.

Fixes #34.1 and #34.2.

Prepared new Drupal.theme functions for password confirm.

Todo besides #34:

1. Function documentation
2. Real BEM-friendly custom templates

Whoops

This patch addresses #34.3, #38.1 and #38.2.

Only #34.4 is missing.

Addresses everything from #34 and #38.

Additionals:

1. Minor fixes for form element related to font size variables
2. High-contrast additions for password confirm + fixes for progress

This is now ready for a review.

Screenshots attached.

Re-rolled again, no interdiff.

I found one small issue with the last patch. The colour of the field labels are inherited from the body so they are #222330 but according to the design, they should be #545560.

If it is a more global decision that form labels should inherit the body colour and the design is not up to date than the patch is RTBC I think as I haven't found any other issue. If it is not the case, the colour of the labels still should be fixed.

I'm doing a detailed review so there may be more items, but wanted to mention this now as it seems like something that could potentially take a bit of time:
I noticed that on RTL, the punctuation position changes based on the presence of content in the fields.

Setting back to Needs Work since this will need to be addressed, but I'll still continue with my review of the patch.

It looks like relying on :watch to build from a newly applied patch is not sufficient. Switched to :build and the issue reported above is no longer occurring.

Pretty much just nits here. Also did a quick check on all OSX browsers + IE11 (incl high-contrast) + mobile simulators to confirm I got the same results as the very-comprehensive set of screenshots in #43.

1. +++ b/css/src/components/form--password-confirm.css
   @@ -0,0 +1,195 @@
   +  margin-bottom: 0;
   

   Is this rule necessary since it collapses margin with the confirm field that appears after it? (perhaps it's necessary on some browsers)

2. +++ b/css/src/components/form--password-confirm.css
   @@ -0,0 +1,195 @@
   +.js .is-initial:not(.form-item--error) .form-item__description {
   ...
   +.js .is-initial.is-password-empty.is-confirm-empty:not(.form-item--error) .password-confirm__confirm {
   +  max-height: 0;
   +  margin-top: 0;
   +}
   

   This approach to hiding the field still allows it to be focused and accessed via keyboard navigation.

3. +++ b/css/src/components/form--password-confirm.css
   @@ -0,0 +1,195 @@
   +  margin-bottom: var(--progress-bar-spacing-size);
   

   These two rules might not be necessary due to collapsing against the margins of elements they will always be adjacent to. There may be other reasons for this or some browsers may need it, so like my earlier comment not 100% sure they can be removed.

4. +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +    `<div aria-live="polite" aria-atomic="true" class="password-strength__title">${
   

   More often than not, despite the aria-atomic, only "Password Strength" is read by screenreaders, and not the strength level inside the span with .password-strength__text. It seems to work any time the password strength changes, and occasionally when the change status is the same.
   This seems to be the case with Seven as well, so if it can't be easily addressed here I think it would be fine to just open a followup issue.

5. +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +    `<div aria-live="polite" aria-atomic="true" class="password-strength__title">${
   

   More often than not, despite the aria-atomic, only "Password Strength" is read by screenreaders, and not the strength level inside the span with .password-strength__text. It seems to work any time the password strength changes, and occasionally when the change status is the same.
   This seems to be the case with Seven as well, so if it can't be easily addressed here I think it would be fine to just open a followup issue.

   +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +      if ($passwordInput.length) {
   +        const translate = settings.password;
   +        const $passwordInputParent = $passwordInput.parent();
   +        const $passwordWidget = $passwordInput
   +          .closest('.js-form-type-password-confirm');
   +        const $passwordConfirmInput =
   +          $passwordWidget.find('input.js-password-confirm');
   +        const $passwordConfirmMessage =
   +          $(Drupal.theme.passwordConfirmMessage(translate.confirmTitle));
   +        const $passwordConfirmMatch = $(Drupal.theme.passwordConfirmMatch());
   +        const $passwordSuggestionsWrapper =
   +          $(Drupal.theme.passwordSuggestionsWrapper()).hide();
   +        const $passwordInputStrengthMessageWrapper =
   +          $(Drupal.theme.passwordInputStrengthMessageWrapper());
   +        const $passwordInputStrengthBar =
   +          $(Drupal.theme.passwordInputStrengthBar());
   +        const $passwordInputHelp = $(Drupal.theme.passwordInputHelpWrapper())
   +          .append($(Drupal.theme.passwordInputStrengthTrack())
   +            .append($passwordInputStrengthBar))
   +          .append($(Drupal.theme.passwordInputMessage(translate.strengthTitle))
   +            .append(' ')
   +            .append($passwordInputStrengthMessageWrapper));
   +
   +        $passwordConfirmMatch.appendTo($passwordConfirmMessage.append(' '));
   +        $passwordConfirmInput.after($passwordConfirmMessage);
   

   There's a good description in the function doc and variable naming here is very good so it's pretty much self-documenting, but this could use a a few additional comments. I'm thinking just enough so if someone were troubleshooting an issue with this functionality they'd be able to quickly find the lines relevant to their issue.

6. +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +  /**
   +   * Evaluate the strength of a user's password.
   +   *
   +   * Returns the estimated strength and the relevant output message.
   +   *
   +   * @param {string} password
   +   *   The password to evaluate.
   +   * @param {object} translate
   +   *   An object containing the text to display for each strength level.
   +   *
   +   * @return {object}
   +   *   An object containing strength, message, indicatorText and indicatorClass.
   +   */
   +  Drupal.evaluatePasswordStrength = (password, translate) => {
   

   I see why it's necessary to add this, since the JS from the user module is not loaded, but I think it would be good to document that its essentially a copy of the JS from the that module (with a few cosmetic-only changes). I noticed Big Pipe does something similar without mentioning that it's a copypaste, but I still think it would be beneficial to document this here.

7. +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +   * Wrapper around password strength feedbacks.
   

   nit - make singular

8. +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +   * Formats the message that preceedes the password strength evaluation.
   

   s/preceedes/precedes
   (there's a few instances of this in the file)

9. +++ b/js/user.es6.js
   @@ -0,0 +1,353 @@
   +   *   The message.
   

   This param is not in the method

10. user.es6.js does a really nice job of avoiding the readability problems that come with JS that adds a bunch of markup and wrappers  👌

This new patch addresses most of #49.

Re #47: You're right, this is a difference. But I don't really want to make an exception just for the password form elements. (Every other form elements label color is #222330 and I'd like to keep this consistent.)

Re #49:

1. I don't know what selector you want to target with this comment; if this is about password-confirm__password or password-confirm__confirm, this zero-margin is definitely neeeded:

   

2. Added a display: none; for this
3. I know this behavior and I rely on it really often 🙂. I just want to provide default margins for these, regardless of their context.
4. After checking https://accessibilityresources.org/aria-atomic https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions I think this is fine. I've changed bit the code so it only updates the confirm message and the strength message if they have to be updated (and hope that it will be better now).
5. ^^ (if this is a dup...)
6. Done.
7. Done.
8. Done.
9. Done.
10. Thanks 🙂

I'm pretty much ready to RTBC this, I just need some clarification on if the "show password" feature styling is in scope. Figma and comment #7 suggest it is in scope, but I think it's better to open a followup that is dependent on #2293803: Replace confirm password element with a new password element with show/hide functionality.

As a precaution, I did test this patch with the most recent patch from #2293803: Replace confirm password element with a new password element with show/hide functionality and the results are acceptable enough that it's probably not worth the headache of styling something that isn't part of core yet.

I don't think we should include the show password in the scope of this issue 😇

I don't think we should include the show password in the scope of this issue

that's the confirmation I needed - setting to RTBC.

+++ b/js/user.es6.js
@@ -0,0 +1,390 @@
+          .append($(Drupal.theme.passwordInputStrengthTrack())
...
+            .append($passwordInputStrengthBar))
...
+          .append($(Drupal.theme.passwordInputMessage(translate.strengthTitle))
+            .append(' ')
...
+            .append($passwordInputStrengthMessageWrapper));

Instead of appending HTML elements here, we could pass on the child element to the theme function so that it's up to the theme function to decide where the child gets rendered.

The Needs reroll tag is valid actually.

The patch that addresses #55.

We have to cast the objects mentioned om #55 instead of appendong them

It seems that casting these object is impossible. We have to find an another acceptable soultion.

Discussed this on Slack with @lauriii.

maybe instead of returning jQuery object, we should return strings
and the we should create the jQuery reference separately
huzooka 1:05 PM
As i did in the patch before the last one, right? (edited) 
lauriii 1:12 PM
we should try to mix them both; not return jQuery objects but also not construct markup outside of the theme functions
huzooka 1:19 PM
so in the end we have to hardcore js- prefixed classes?
lauriii 1:22 PM
that might be a better solution
huzooka 1:22 PM
okay, but is it an acceptable solution as well?
I don't want to loop on the possible solutions again and again
so I kindly ask you to provide some AC for this task.
lauriii 1:24 PM
I think it is, I think it’s being used in other places as well

So we'll use much less templates here, but those will contain some hard-coded CSS classes used by our JavaScript.

New patch addressing #63.

This looks great. Thank you everyone! 🙏Next step is to incorporate these improvements to the JavaScript in core as part of #3067523: Add Drupal JavaScript theme functions for password confirm widget.