Problem/Motivation

I think people know what a password is and that they need to enter one that matches their username.

Maybe it wasn't as self-explanatory in... 2001!!!!

git show f358893b52f364ae | grep accompanies
+  $output .= form_password(t("Password"), "pass", $pass, 20, 64, t("Enter the password that accompanies your username."));
+  $output .= form_password(t("Password"), "pass", $pass, 20, 64, t("Enter the password that accompanies your username."));

Steps to reproduce

Proposed resolution

Login form with no descriptions for the username or password fields

Remaining tasks

We could also remove the description from the username field, one caveat is that we don't allow e-mail logins by default so maybe it's actually useful. But also the field name is 'username' and e-mail login modules change the description anyway I think.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 interdiff_11-14.txt1.47 KBkushagra.goyal
#14 3376314-14.patch2.48 KBkushagra.goyal
#13 Success-test.png79.47 KBkushagra.goyal
#13 interdiff_11-13.txt2.29 KBkushagra.goyal
#13 3376314-13.patch2.54 KBkushagra.goyal
#11 After.png11.64 KBurvashi_vora
#11 Before.png15.32 KBurvashi_vora
#11 interdiff-2_11.txt528 bytesurvashi_vora
#11 3376314-11.patch953 bytesurvashi_vora
#2 3376314.patch556 bytescatch
Screenshot from 2023-07-22 16-42-17.png29.24 KBcatch
Screenshot from 2023-07-22 16-40-45.png41.27 KBcatch

Issue fork drupal-3376314

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3376314.patch556 bytes
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

The change seems fine. I think it might be worth writing a CR given that this page is something that might have some special styles and might be featured in a lot of screenshots.

I'm also wondering why not remove descriptions for both fields at once?

Status: Needs review » Needs work

The last submitted patch, 2: 3376314.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Title: Remove the password description from the login form » Remove the username and password descriptions from the login form

Both a CR and removing both descriptions here sounds good.

The test failure is real - locale test is using this as a string to translate.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

+1 on removing the description for the password.

About #4suggestion to remove the description for the user, I'm afraid that might need some more discussion: users can assume it can either be an email or a user name since there are different standard patterns out there. So this description is actually helping with that, and to remove that we should find an alternative way to help with any confusion.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

About #4suggestion to remove the description for the user, I'm afraid that might need some more discussion: users can assume it can either be an email or a user name since there are different standard patterns out there. So this description is actually helping with that, and to remove that we should find an alternative way to help with any confusion.

I agree that the username pattern is used less and less and may be confusing. However, I'm not sure I understand how the description helps with this, given that the label is already "Username", and the description text is "Enter your username". 🤔 Usually when login uses email, the username field is labelled as "Email". Even in this case you probably wouldn't need additional description because email as the label is already descriptive.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

Ah right, I guess that's true since the description is just adding the same key info as the label. I'm assuming though that in a situation where you can have both username and email, the label shouldn't be "Username or email" but it would ideally add description. But since that's not the default and the description can be added anyway, removing here the existing description for the username makes sense.

catch’s picture

catch
he/him
Primary language English
commented

fwiw #7 was why I originally left this out of the patch/title, but I agree with #8 and #9.

urvashi_vora’s picture

urvashi_vora commented
Status: Needs work » Needs review
StatusFileSize
new 3376314-11.patch953 bytes
new interdiff-2_11.txt528 bytes
new Before.png15.32 KB
new After.png11.64 KB

Hi everyone,

I went through the discussion going on here, from the summary based on the updated issue title and the points discussed in #8 and #9, I drew a conclusion that, since the field label mentions "Username", and the description for the Username field is also somewhat similar as "Enter your username", it could be easier to understand the purpose of the field from the label itself and thus the Username field description to be removed along with the description of Password field as per the updated issue title.

After applying the patch provide by @catch in #2, the description for Password field was successfully removed.

I would like to provide a patch for removing the description on Username field.

Please guide me if anything wrong with the patch. I tested the patch in my local system. Also attaching the screenshots of before and after applying the patch.

Thanks

Status: Needs review » Needs work

The last submitted patch, 11: 3376314-11.patch, failed testing. View results

kushagra.goyal’s picture

kushagra.goyal commented
StatusFileSize
new 3376314-13.patch2.54 KB
new interdiff_11-13.txt2.29 KB
new Success-test.png79.47 KB

After removing the description there is no longer necessary to write tests for description and test-cases as encountered failures in patch #11, So according updating the Patch to resolve failure.
Thanks..

kushagra.goyal’s picture

kushagra.goyal commented
StatusFileSize
new 3376314-14.patch2.48 KB
new interdiff_11-14.txt1.47 KB

Updating patch again as above mentioned patch in #13 have some issues.

sakthi_dev made their first commit to this issue’s fork.

sakthi_dev opened merge request !4472

sakthi_dev’s picture

sakthi_dev commented
Status: Needs work » Needs review

As the patch failed to apply, created a MR for the same.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Not ready for review

Why are we removing an entire test check?

catch’s picture

catch
he/him
Primary language English
commented

Yes that test can't be removed, it's testing translation functionality, it just happens to use the login page to do so. Instead, we need to translate a different string and visit the page that string appears on.

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +Needs change record
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
Issue tags: -Needs change record

- Hidden patches, since we're using the MR-flow
- Opted to translate "Password" in \Drupal\Tests\locale\Functional\LocaleTranslationUiTest::testStringTranslation with a lesser known zoological fact about Llamas (wink wink, nudge nudge).
- Added a (minimal) draft CR. That needs a look over by a native speaker since it's non-technical, also left "Introduced in version" open.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs review » Needs work

Switched the after screenshot in the issue summary and made a couple of edits to the change record.

A couple more things we need to sort out.

@Jillian Chueka brought up aria labels in slack - that the description is recommended when aria labels aren't implemented. So I double checked what the markup looks like at the moment.

We do have aria-labelledby and we even have some explicit code referencing it:

  public function build() {
    $form = \Drupal::formBuilder()->getForm('Drupal\user\Form\UserLoginForm');
    unset($form['name']['#attributes']['autofocus']);
    // When unsetting field descriptions, also unset aria-describedby attributes
    // to avoid introducing an accessibility bug.
    // @todo Do this automatically in https://www.drupal.org/node/2547063.
    unset($form['name']['#description']);
    unset($form['name']['#attributes']['aria-describedby']);
    unset($form['pass']['#description']);
    unset($form['pass']['#attributes']['aria-describedby']);
    $form['name']['#size'] = 15;
    $form['pass']['#size'] = 15;

This is in UserLoginBlock.

So we need to get rid of that, but also should we add aria-labelledby to replace it?

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

So we need to get rid of that, but also should we add aria-labelledby to replace it?

The removing bit is always the easy one...

I think we should add the aria-labelledby because it seems we're loosing accessibility now.
However:

1) aria-labelledby seems to want an id-selector, which the logical candidates (at least for me), the labels don't have.
2) I struggle to find a place where we can set aria-labelledby. \Drupal\user\Form\UserLoginForm::buildForm knows nothing about the labels.

Needs guidance/an actual brain to look at this, so putting on NR.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

We don't need the aria-labelledby on input elements because labels can reference the input with for attribute which is set automatically by Drupal.

spokje’s picture

spokje
Primary language Dutch
commented

Thanks @lauriii, so I was thinking the wrong way-round and this is already covered.
So now it's a proper NR :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

label for makes sense, thanks for stopping us going down a rabbit hole.

Crediting Jillian Chueka for the slack comments, I think we're fine here after all, but good to find the login block cruft and confirm things.

Translating 'Password' is a great idea to avoid having to completely redo that test.

Since my original patch here only removed one line and not any of the important bits, I think I'm OK to RTBC this one given the +1s from both @ckrina and @lauriii.

btw I realised we should make this change when working on #3323850: Set theme logo fetchpriority to high - on the login page for Umami, the description field is the largest contentful paint, and it was this that actually made me read the password description for the first time in probably years.

catch credited bostonjillian.

catch’s picture

catch
he/him
Primary language English
commented
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 85700fc and pushed to 11.x. Thanks!

  • lauriii committed 85700fc5 on 11.x 
    Issue #3376314 by Spokje, urvashi_vora, catch, lauriii, ckrina,...

lauriii closed merge request !4472

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.