Add Drupal JavaScript theme functions for password confirm widget

Reroll for working from 9.1.x

1. +++ b/core/modules/user/tests/src/FunctionalJavascript/PasswordConfirmWidgetTest.php
   @@ -0,0 +1,149 @@
   +    //
   +    // Now reload the page, and fill only the main input for first.
   +    //
   

   There are a few places in this test, where inline comments begin and/or end with empty lines. Those empty lines can be removed. I can see the appeal of using them for emphasis, but blank inline comments should only be used for separating paragraphs.

2. +++ b/core/modules/user/tests/src/FunctionalJavascript/PasswordConfirmWidgetTest.php
   @@ -0,0 +1,149 @@
   +    // Password tips should be refreshed and get visible.
   +    $this->assertNotNull($this->assert->waitForElement('css', "$password_confirm_selector + .password-suggestions > ul > li"));
   +    $this->assertTrue($password_confirm_item->find('css', "$password_confirm_selector + .password-suggestions > ul > li")->isVisible());
   +    // Password match message must become invisible.
   +    $this->assertFalse($password_confirm_item->find('css', 'input.js-password-confirm + .js-password-confirm-message')->isVisible());
   +    // Password strength message should be updated.
   +    $this->assert->elementContains('css', "$password_confirm_widget_selector $password_parent_selector", '<div aria-live="polite" aria-atomic="true" class="password-strength__title">Password strength: <span class="password-strength__text js-password-strength__text">Weak</span></div>');
   +    // Deleting the input must not change the element above.
   +    $password_confirm_widget->fillField('Password', 'o');
   

   This one isn't in the code standards, but I had a committer state a strong preference for adding a blank line above inline comments if the previous line has the same indentation. I don't mind making those folks happy.

3. +++ b/core/modules/user/user.es6.js
   @@ -16,103 +16,189 @@
   +          const addWidgetClasses = () => {
   +            $passwordWidget
   +              .addClass(
   +                $mainInput.val()
   +                  ? settings.password.cssClassPasswordFilled
   +                  : settings.password.cssClassPasswordEmpty,
   +              )
   +              .addClass(
   

   This and a few other function definitions need to be JSDoc'd.

4. +++ b/core/modules/user/user.es6.js
   @@ -16,103 +16,189 @@
   +                // keep all the classes, but let's make sure.
   

   The phrasing here sounds uncertain. Could this be changed to something more definite sounding? If there *is* some uncertainty, it would be good to confirm the exact behavior before rewriting the comment.

5. +++ b/core/modules/user/user.es6.js
   @@ -16,103 +16,189 @@
   +        .each(function processPasswordConfirm() {
   

   This can just be an anonymous () =>

6. +++ b/core/modules/user/user.theme.es6.js
   @@ -3,13 +3,111 @@
   +   * Constucts a password confirm message element.
   

   s/Constucts/Constructs

7. +++ b/core/modules/user/user.theme.es6.js
   @@ -3,13 +3,111 @@
   +   *   - If a string is returned, then it has to contains a <span>, but only
   

   could change "has to contains a" to "must contain a"

8. +++ b/core/modules/user/user.theme.es6.js
   @@ -3,13 +3,111 @@
   +   *     Drupal.theme.passwordConfirmMessage = passwordSettings => {
   +   *       const confirmMessage = {};
   +   *       confirmMessage.item = $(
   +   *        `<div aria-live="polite" aria-atomic="true">` +
   +   *          `${passwordSettings.confirmTitle}` +
   +   *        `</div>`,
   +   *       ).append((confirmMessage.textWrapper = $('<span></span>')));
   +   *       return confirmMessage;
   

   Not sure it would pass prettier, but is there a way to put the population of confirmMessage.textWrapper on its own line? It would be helpful to have the creation of both required keys be explicit in the example.

9. +++ b/core/modules/user/user.theme.es6.js
   @@ -3,13 +3,111 @@
   +   * Constucts a password strenght message.
   

   s/Constucts/Constructs
   s/strenght/strength

   Look for these typos throughout the patch, some appear in Claro's JS too.

10. +++ b/core/modules/user/user.theme.es6.js
    @@ -3,13 +3,111 @@
    +   *   - item: The root jQuery element that contains the whole message,
    

    Maybe change "jQuery element" to "jQuery object" for consistency with the other property definitions.

11. +++ b/core/modules/user/user.theme.es6.js
    @@ -3,13 +3,111 @@
    +   *   - bar: The jQuery object of the password strength bar that's width will
    ...
    +   */
    

    r/that's/whose

12. +++ b/core/modules/user/user.es6.js
    @@ -16,103 +16,189 @@
    + const $passwordWidget = $mainInput.closest(
    + '.js-form-type-password-confirm',
    + );
    + const $confirmInput = $passwordWidget.find(
    + 'input.js-password-confirm',
    + );
    + const passwordConfirmMessage = Drupal.theme(
    + 'passwordConfirmMessage',
    + settings.password,
    + );
    +
    + const $confirmMessage =
    + typeof passwordConfirmMessage === 'string'
    + ? $(passwordConfirmMessage)
    + : passwordConfirmMessage.item;
    + const $confirmTextWrapper =
    + typeof passwordConfirmMessage === 'string'
    + ? $confirmMessage.find('span')
    + : passwordConfirmMessage.textWrapper;
    + const $confirmInputParent = $confirmInput
    + .parent()
    + .addClass('confirm-parent')
    + .append($confirmMessage);
    + const password = {};
    + const passwordStrengthBarClassesToRemove = [
    + settings.password.cssClassPasswordWeak || '',
    + settings.password.cssClassPasswordFair || '',
    + settings.password.cssClassPasswordGood || '',
    + settings.password.cssClassPasswordStrong || '',
    + ]
    + .join(' ')
    + .trim();
    + const confirmTextWrapperClassesToRemove = [
    + settings.password.cssClassPasswordsMatch || '',
    + settings.password.cssClassPasswordsNotMatch || '',
    + ]
    + .join(' ')
    + .trim();
    + const widgetClassesToRemove = [
    + settings.password.cssClassWidgetInitial || '',
    + settings.password.cssClassPasswordEmpty || '',
    + settings.password.cssClassPasswordFilled || '',
    + settings.password.cssClassConfirmEmpty || '',
    + settings.password.cssClassConfirmFilled || '',
    + ]
    + .join(' ')
    + .trim();
    

    This is a pretty large chunk of logic without any comments. The variable names are good, which is helpful, but this should get commented at least as well as what it replaced.

13. +++ b/core/modules/user/user.theme.es6.js
    @@ -3,13 +3,111 @@
    +   *     Example:
    

    Move the indentation of "Example:" back two spaces.

14. +++ b/core/modules/user/user.theme.es6.js
    @@ -3,13 +3,111 @@
    +   * @param {bool} outputEmtpyList
    

    s/outputEmtpyList/outputEmptyList (this happens a few times in this patch)

This should address #13.

This should fix the failing test

All of my feedback in #13 is addressed.

I'd like to see a change record before I'd RTBC as that would help me do a final round of review on a pretty large patch.

Also tagging with "Needs issue summary update" as the issue summary has this scoped to just creating the theme functions, but this patch also leverages those in Claro. The IS either needs to include the Claro changes, or they should be split to another issue.

Change record has been added

All my review items have been addressed , and the change record is consistent with my understanding of what the patch is doing, so this can RTBC.

Looks like it no longer applies (already).

Reroll attached

1. +++ b/core/modules/user/user.libraries.yml
   @@ -3,6 +3,19 @@ drupal.user:
   +  drupalSettings:
   +    password:
   +      cssClassPasswordsMatch: 'ok'
   

   I'm not sure if we should use the drupalSettings for overriding CSS classes 🤔 I'm inclined to solving this in a follow-up, but on the other hand, solving this in a follow-up would be more difficult because we would have to mindful of breaking the API.

2. +++ b/core/modules/user/user.libraries.yml
   @@ -3,6 +3,19 @@ drupal.user:
   +      cssClassWidgetInitial: false
   +      cssClassPasswordEmpty: false
   +      cssClassPasswordFilled: false
   +      cssClassConfirmEmpty: false
   +      cssClassConfirmFilled: false
   

   If we decide to go with this approach, should we convert these to empty strings? Currently this is relying on jQuery processing boolean values correctly. As far as I can see in their documentation, this behavior isn't documented so it might not be a good idea to rely on it.

Lot of comments! thanks for that :)

The passwordStrength and passwordConfirmMessage theme function are too smart. And it is not integrator-friendly.

+            password.$strengthBar = passwordStrength.bar;
+            password.$strengthTextWrapper =
+              passwordStrength.strengthTextWrapper;

I would rather add data attributes in the html tags (and get rid of the js-* classes, for some history #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings) and do

+            password.$strengthBar = $passwordStrength.find('[data-password-strength="indicator"]');
+            password.$strengthTextWrapper = $passwordStrength.find('[data-password-strength="text"]');

Theme functions should return strings, not objects.

Also there is #2293803: Replace confirm password element with a new element that allows toggling to view the typed password to keep in mind

After all these years I finally got dreditor on my main browser :p

1. +++ b/core/modules/user/user.es6.js
   @@ -16,103 +16,209 @@
   +            settings.password,
   ...
   +            settings.password.cssClassPasswordWeak || '',
   +            settings.password.cssClassPasswordFair || '',
   +            settings.password.cssClassPasswordGood || '',
   +            settings.password.cssClassPasswordStrong || '',
   ...
   +            settings.password.cssClassPasswordsMatch || '',
   +            settings.password.cssClassPasswordsNotMatch || '',
   ...
   +            settings.password.cssClassWidgetInitial || '',
   +            settings.password.cssClassPasswordEmpty || '',
   +            settings.password.cssClassPasswordFilled || '',
   +            settings.password.cssClassConfirmEmpty || '',
   +            settings.password.cssClassConfirmFilled || '',
   ...
   +                  ? settings.password.cssClassPasswordFilled
   +                  : settings.password.cssClassPasswordEmpty,
   ...
   +                  ? settings.password.cssClassConfirmFilled
   +                  : settings.password.cssClassConfirmEmpty,
   ...
   +              ? settings.password.cssClassPasswordsMatch
   +              : settings.password.cssClassPasswordsNotMatch;
   ...
   +              ? settings.password.confirmSuccess
   +              : settings.password.confirmFailure;
   

   Better to make a variable like const settingsPassword = settings.password;

2. +++ b/core/modules/user/user.libraries.yml
   @@ -3,6 +3,19 @@ drupal.user:
   +  drupalSettings:
   +    password:
   +      cssClassPasswordsMatch: 'ok'
   +      cssClassPasswordsNotMatch: 'error'
   +      cssClassPasswordWeak: 'is-weak'
   +      cssClassPasswordFair: 'is-fair'
   +      cssClassPasswordGood: 'is-good'
   +      cssClassPasswordStrong: 'is-strong'
   +      cssClassWidgetInitial: false
   +      cssClassPasswordEmpty: false
   +      cssClassPasswordFilled: false
   +      cssClassConfirmEmpty: false
   +      cssClassConfirmFilled: false
   

   We can add a level here: password.css.match, password.css.notMatch, etc.

   feels a bit old-school to go with that much capitalization :)
   Like lauriii a bit conflicted by putting class names in the settings, but it is a setting so... why not. It could also be put in a theme function, it'd be closer to the experience of overring CSS/JS, than for a themer to try to remember how to override libraries settings.

3. +++ b/core/modules/user/user.module
   @@ -1106,7 +1106,9 @@ function user_form_process_password_confirm($element) {
   -  $element['#attached']['drupalSettings']['password'] = $password_settings;
   +  foreach ($password_settings as $config_key => $config_value) {
   +    $element['#attached']['drupalSettings']['password'][$config_key] = $config_value;
   +  }
   

   $element['#attached']['drupalSettings']['password'] += $password_settings;?

Thinking about it, shouldn't be in drupalsettings (it's for PHP to JS data) and shouldn't be in a theme function, have the list somewhere in the JS file and expose it through Drupal.password.css or something.

Started with a reroll

Theme functions should return strings, not objects.

I had some reservations on the approach too. However, based on Drupal.theme documentation, it seemed like it would be an accepted return value for theme functions:

   * @return {string|object|HTMLElement|jQuery}
   *   Any data the theme function returns. This could be a plain HTML string,
   *   but also a complex object.

We could probably change the approach to use the data-drupal-selector if we agree that it's a preferred approach, but I just wanted to point out that the way I understand the documentation, this would be allowed. Maybe the documentation is incorrect or misleading and needs to be updated.

The docs are definitely not correct, this is wording from at least 2007. Definitely not in line with what we've been doing for a few years now.

Let's open an issue for discussing and changing that

This should address #25. We will still have to address #24.

Changes in this patch:

• Addressed #24 by making theme functions in the patch only return strings
• Target elements with data-drupal-selector
• Removed js- prefixed classes in the theme functions, added theme function overrides in Stable. We should write change record for this.
• Added new test for Claro to ensure that there are not regressions caused by the theme function overrides in Claro.
• Removed the BC layer for adding empty li element inside the password suggestions since it was already removed in Claro. We could add it back to Stable if we think it would be useful but I feel like it might not be necessary.

Remaining work:

• Open follow-up for addressing #29
• Write change record for the removal of the js- prefixed classes

This should address the test failure

Opened follow-up #3152042: Update Drupal.theme documentation to only allow returning string.

Quick review:

1. +++ b/core/modules/user/user.es6.js
   @@ -3,7 +3,53 @@
   +   * @type {object}
   +   * @property {string} passwordParent - A CSS class for the parent element.
   +   * @property {string} passwordsMatch - A CSS class indicating password match.
   

   The rest of core use @prop, not @property.

2. +++ b/core/modules/user/user.es6.js
   @@ -193,29 +336,32 @@
   +    // Assemble the final message while keeping the original message array.
   +    const messageTips = msg;
   +    msg = `${passwordSettings.hasWeaknesses}<ul><li>${msg.join(
   

   Shouldn't that use Drupal.theme('passwordSuggestions') ?

3. +++ b/core/modules/user/user.theme.es6.js
   @@ -3,13 +3,82 @@
   +   * @return {string}
   +   *   This function returns markup for password strength message.
   +   */
   +  Drupal.theme.passwordStrength = passwordSettings => {
   +    const strengthIndicator =
   +      '<div class="password-strength__indicator" data-drupal-selector="password-strength-indicator"></div>';
   +    const strengthText =
   +      '<span class="password-strength__text" data-drupal-selector="password-strength-text"></span>';
   +    return $('<div class="password-strength"></div>')
   +      .append(
   +        `<div class="password-strength__meter" data-drupal-selector="password-strength-meter">${strengthIndicator}</div>`,
   +      )
   +      .append(
   +        `<div aria-live="polite" aria-atomic="true" class="password-strength__title">${passwordSettings.strengthTitle} ${strengthText}</div>`,
   +      );
   +  };
   

   It says returns string but it return an jQuery object.

4. +++ b/core/themes/claro/claro.libraries.yml
   @@ -219,11 +219,8 @@ form.password-confirm:
   -    js/user.js: {}
   +    js/user.theme.js: {}
      dependencies:
   -    - core/jquery
   -    - core/drupal
   -    - core/jquery.once
        - claro/global-styling
   

   A bit too eager, this still depends on core/drupal since it uses the main Drupal object.

5. +++ b/core/themes/claro/js/user.theme.js
   --- /dev/null
   +++ b/core/themes/stable9/js/user.theme.es6.js
   

   missing the compiled file no?

I've added another CR draft to address #32.

This should also address #36, and also adds BC layer for rest of the selectors where we switched from a js- prefixed class to data-drupal-selector.

Nice I didn't know we had that for deprecated JS :)

Understood why we kept the code I pointed out in #36.2

Still an issue with the dependencies, core/jquery is not used in the theme file anymore :) some estlint stuff as well as a typo in the claro theme file.

+++ b/core/themes/claro/js/user.theme.es6.js
@@ -0,0 +1,81 @@
+(($, Drupal) => {
+  $.extend(Drupal.user.password.css, {

I added the jQuery dependency for this. Maybe we should use Object.assign instead?

oh right didn't saw that one. objec assign would be better yes

Changed to using Object.assign instead

Looks good :)

Is possible to test the deprecations? There seem to be quite a few new deprecations added by the patch. Also the list of things that have been deprecated doesn't appear to tally with what's listed in the change records.

1. +++ b/core/modules/user/user.es6.js
   @@ -16,103 +62,225 @@
   +          if ($confirmTextWrapper.length === 0) {
   +            $confirmTextWrapper = $passwordConfirmMessage.find('span').first();
   +            Drupal.deprecationError({
   +              message:
   +                'Returning <span> without data-drupal-selector="password-confirm-text" attribute is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3152101',
   +            });
   +          }
   

   There doesn't appear to be test coverage of this deprecation.

2. +++ b/core/modules/user/user.es6.js
   @@ -16,103 +62,225 @@
   +                'Returning <span> without data-drupal-selector="password-confirm-text" attribute is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3152101',
   

   This doesn't seem to have any detail in the CR.

3. +++ b/core/modules/user/user.es6.js
   @@ -123,13 +291,14 @@
       * @return {object}
       *   An object containing strength, message, indicatorText and indicatorClass.
       */
   -  Drupal.evaluatePasswordStrength = function(password, translate) {
   +  Drupal.evaluatePasswordStrength = (password, passwordSettings) => {
   
   @@ -193,37 +362,46 @@
   +    return Drupal.deprecatedProperty({
   +      target: {
   +        strength,
   +        message: msg,
   +        indicatorText,
   +        indicatorClass,
   +        messageTips,
   +      },
   +      deprecatedProperty: 'message',
   +      message:
   +        'The message property is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. The markup should be constructed using messageTips property and Drupal.theme.passwordSuggestions. See https://www.drupal.org/node/3130352',
   +    });
   
   

   The return now contains messageTips and message is deprecated. This deprecation is not mentioned in the CR's as far as I can see.

4. +++ b/core/modules/user/user.js
   @@ -5,67 +5,139 @@
   -        var $confirmResult = $passwordInputParentWrapper.find('div.js-password-confirm-message');
   ...
   +        var $confirmInput = $passwordWidget.find('input.js-password-confirm');
   +        var $passwordConfirmMessage = $(Drupal.theme('passwordConfirmMessage', settings.password));
   +        var $confirmTextWrapper = $passwordConfirmMessage.find('[data-drupal-selector="password-confirm-text"]').first();
   

   The CR mentions password-confirm-message but not password-confirm-test - having both is confusing - what's the difference?

Working on #43

#43.1

There doesn't appear to be test coverage of this deprecation.

Added a test that covers all deprecations introduced in this patch.

#43.2 CR rewritten to make this clearer.

#43.3 Created a new CR for this as it did not fit into the others https://www.drupal.org/node/3160593

#43.4 Once pointed out, I agree this is confusing. Did some rewriting of the CR and password-confirm-message/password-confirm-text and some associated variables are renamed to make their purpose clearer.

+1 on adding test coverage for the deprecations. Reviewed the interdiff between #45 and #41 The test coverage looks sufficient for me. I also think that the password-match-status-text is clearer than password-confirm-text so +1 for that change 👍

I've started to review this, and haven't finished yet, but so far, I'm confused about these 2 small things:

+++ b/core/modules/user/user.es6.js
@@ -16,103 +62,225 @@
+              Drupal.theme('passwordSuggestions', settings.password, [], false),

What's the false argument for?

--- a/core/themes/claro/css/components/form--password-confirm.pcss.css
+++ b/core/themes/claro/css/components/form--password-confirm.pcss.css

How are these CSS changes related to the scope of this issue?

1. +++ b/core/modules/user/tests/themes/password_theme_function_test/js/password-theme-functions.es6.js
   @@ -0,0 +1,91 @@
   +  Drupal.theme.passwordSuggestions = ({ hasWeaknesses }, tips) =>
   
   +++ b/core/modules/user/user.js
   

   Should we add the tips to the same object?

2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -140,6 +140,7 @@ public static function isDeprecationSkipped($message) {
   +      'hello',
   
   +++ b/core/themes/claro/claro.libraries.yml
   @@ -1,3 +1,4 @@
   +
   

   Are these changes actually needed?

#47.1

What's the false argument for?

, looks like it's for an arg that was present in an earlier iteration of the function. It's no longer used so I removed it from the patch.

#47.2

How are these CSS changes related to the scope of this issue?

I had assumed they were added to maintain behavior that would otherwise be regressed by this patch, but after comparing to HEAD this does appear to be out of scope. I added #3167508: [PP-1] Add transition to password confirm reveal as a followup to add these transitions within an appropriate scope.

#48.1 Regarding adding tips to the same object in ({ hasWeaknesses }, tips) =>, the { hasWeaknesses } is a string that is destructured from the full passwordSettings object, while tips is a value that varies depending on what the aspiring password needs. It seems more appropriate to me to keep the passwordSettings and dynamic values separate, but open to hearing why it may be more beneficial to combine them in a single object.

#48.2 items are reviewed.

Thank you for explaining #48.1 and sorry for the rushed review. I somehow thought it was an options object and I thought we could have used it for passing the tips too.

Confirmed that #49 addresses #47 and #48 so moving back to RTBC.

#49 looks great. This just fixes some coding standards violations.

Removing credit from myself (#53 interdiff is too trivial), and adding credit to @alexpott for #43.

Pushed to 9.1.x. Great work on this!