Error highlighting and reporting problems for the current password on the user profile form

Like I mentioned in #1493324-363: Inline form errors for accessibility and UX, There are cases when we want to highlight the field without error message. If we need to solve those cases for inline error messages, then the title of the issue should be more generic.

I agree that "when adding dependent field error messages we should make the notification a link to the other field." should be a new issue. Seems like a useful pattern.

please see comment20 FORM System | When adding dependent field error messages

this patch addresses the linking concern, not enhancements to Form System. i think, If/when 2514730 is complete a follow up for this issue would then be best.

rebuilt patch

remove dup image. replace with related user/add UX change

linking to "Plural lists should use HTML lists" --> formatPlural 2 or more ('And' separated lists idea)

Patch no longer applies to 8.1.

error: core/modules/user/src/Tests/UserAccountFormFieldsTest.php: No such file or directory

This moved:
https://api.drupal.org/api/drupal/core!modules!user!tests!src!Kernel!Use...

Will move again it seems in 8.2.

It is looking a lot better. Why aren't these two the same:

Test with no current password but matching changed passwords:

Test with current wrong current password:

The error "The specified passwords do not match" comes from /core/lib/Drupal/Core/Render/Element/PasswordConfirm.php line102 (the basic render element) not the User Module's Validation Constraint.

Sounds like we should we open up a new issue so that validatePasswordConfirm() includes a message similar to when there is a matching password, but no current password.

Yes as to new issue, in general for this. keep in mind that it's a general render element which module developers could be using in custom forms -- it may be as well to leave this phrasing for that reason.

to note: with IFE on.. seems to work as well as can be expected

@mike
i've just saw your change comments in issue summary about 'how to i get this' [1] [2] -- is with inline Form Errors module on, of course (:

i've opened a new issue regarding the comma list formatting. it's been bothering me alot through working on this patch.

This patch contains only one change to switch form deprecated Drupal::l() to Link::createFromRoute(). This has no visual change from ongoing review.

First and foremost, a big thanks for digging deep into this. There are a lot of improvements which we should get into core somehow.
However in the context of inline form errors we should really refocus this issue. Because otherwise we really never keep inline form errors as an experimental module in core, let alone as a real core module.

Because the problem reported is really about the user profile form, I'm renaming this issue again and thereby reverting #3/#4. Because the current title "Handle error fields without a message" doesn't describe a real available problem.

Furthermore the patches supplied are hard to review because there are not interdiffs, please add them in the future. And even more important, these patches become too big to work with and introduce too many changes. I'm not saying these changes shouldn't get into core, just that with this amount of changes the patches will never be reviewed and considered.

What I'll do next: get only the really relevant changes from the patch of SKAUGHT and make a new patch.
For all other issue's around the user profile, please create separate issues if they do not exist already.

please use patch #27. Although all patches are whole to themselves..sorry, i was previously unaware of interdiff practice on d.org.

A big part to note about this patch is it does essential work with and without IFE enabled.

I would certainly be confident in saying: this issue (and this patch) is worth of being committed regardless of IFE related concerns.

@SKAUGHT, if you want to see this committed I advise you to break up the patch in different patches, with each their own clear issue.
The current patch includes too many changes unrelated to the focus of this issue: inline form errors + current password.

certainly this patch does introduce a couple of UX changes not related. I understand your point in that. I'll try to find some time to revisit this in the next couple of days to break those changes apart from the validation processing.

Other UX enhancements have been removed from this patch.

in keeping with old issue titling "Handle error fields without a message.." this does add jump links for both the Current Password and Email fields back-and-forth to Current Password.

• Current Password
  
  
  
• Email
  
  
  
• Both sets of error tripped
  

@SKAUGHT - your screenshots look awesome! I'm really excited to see this come through. As a disclaimer, I'm new to using SimplyTest.me, but I spun this up with your patch and wasn't able to see the changes shown in your screenshot. Am I missing something?

Woohoo! Manual test works!

Some notes:

1. This patch give strange results if the inline_form_errors module is not enabled.

   My experience is that if we want to get this in, we should add this functionality to the inline_form_errors module, and not to core directly. So only make changes to core, if currently core is broken, even without inline_form_errors turned on.

2. +    if ($routeName == 'entity.user.edit_form') {
   

   Is this safe to do? What is the route is changed by someone (which happens in custom sites)?

3. +++ b/core/modules/user/src/AccountForm.php
   @@ -371,10 +373,54 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
   +        // It is expected that no more than 2 errors will be joined this way.
   

   Nit: Replace "2" with "two".

4. +++ b/core/modules/user/src/Tests/UserEditTest.php
   @@ -42,7 +42,7 @@ function testUserEdit() {
   -    $this->assertRaw(t("Your current password is missing or incorrect; it's required to change the %name.", array('%name' => t('Email'))));
   +    $this->assertText(t("Your Current password is missing or incorrect; it's required to change the email address."));
   

   +++ b/core/modules/user/src/Tests/UserEditTest.php
   @@ -53,7 +53,7 @@ function testUserEdit() {
   -    $this->assertRaw(t("Your current password is missing or incorrect; it's required to change the %name.", array('%name' => t('Password'))));
   +    $this->assertText(t("Your Current password is missing or incorrect; it's required to change the current password."));
   

   Let's not change this. But if (1) is achieved, this is no problem.

@helenasue
please enable the 'experimental' module Inline Form Errors to get these results

@dmsmidt
here is where we have a unique side of this ticket..although this issue has been targetted as IFE related.. it's not at all. The issue about one field and a conditional error on the second. It is the validation of the Acccount Form that needs to be changed.

#35.point 1
i'm aware of what you say about IFE Off ( i think mikes comment pic #21 shows that). overall it is for the jumpto links and setting corrent passward to a false validation that need to be.

of course, our end goal is full inclusion of IFE..this oddness will be irrelevant when that is done. ..and without it, it still does pass the user to and between these related fields. Keep in mind the IFE module is only an on/off switch as it is #2578561: Move "Inline Form Errors" functionality to optional module and restore D7-style form errors by default

#35. point 2

• The route check is needed as the Account form is used in 3 or 4 contexts..user add, user password..
  note: line 117, 134, 159, 170, 178 -- these are conditions established by other routes and user access..
• If the route is not entity.user.edit_form (which happens under the above listed context) it follows it's first if condition..thereby completing the original validation sequence [see AccountForm.php lines 417-421]
• As for your point about custom module/project alterations it; of course, is up to that dev/team to handle their own changes. If they are going to alter the validation of this form, then.. c'est le vie. This is a ContentEntityForm, it can be altered or extended like anything else.

i'll check over the nit comments and address more tomorrow as i am on the road right now..

repair lost image in summary

@SKAUGHT:
I'm fully aware that currently the inline_entity_form module is just an on/off switch basically.
However that is because 'once' IFE wasn't an experimental module. But, as of now everything that is a fix specifically for IFE should go in the experimental module, since it is not clear if IFE will make it *sad face*.

You state there is also a problem in core, because the current password field isn't highlighted and the messages are a bit nasty. I can acknowledge that, so I'm changing the title and description (with new screenshot) of this problem. Note that for core we should thus only fix that part.

The rest of the new error reporting logic you created is mainly important for IFE, so let's move that to the IFE module.

If we manage this we gain two things: a better experience in core without IFE turned on and a better experience with IFE turned on.

So, currently the patch changes the logic of \Drupal\user\AccountForm, I think the Core specific fixes should be moved to: \Drupal\user\ProfileForm because the extra "current password" error logic is not needed on \Drupal\user\RegisterForm. This move also make the route check in the patch redundant.

For the IFE specific changes we can implement hook_entity_type_alter() in the inline_entity_form module to change the \Drupal\user\Entity\user plugin definition.

Here is a shot at what I mean (without updated tests). Screenshots added to the issue description.

The more I think about it, why do we set errors on the password and e-mail fields, while the problem is on the 'current password' field.
No changes are required to the password and e-mail field, the input can be correct.

So shouldn't we just highlight the 'current password' field? And don't highlight password and e-mail fields?

patch on #38: looks good for the UX side of it. The CI test fails are a mystery to me..

as it is now: the password field returns empty fields once is validated, so we would as least need to have the default_value restored for password so that a user doesn't get trapped in a loop where they keep needing to refill it out.

1. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -38,7 +38,9 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +      if (!empty($error)) {
   +        $this->drupalSetMessage($error, 'error');
   +      }
   

   This needs test coverage in FormErrorHandlerTest. When would $errors be full of falsey values? And wouldn't it be better then to do foreach (array_filter($errors) as $error) {

2. +++ b/core/modules/inline_form_errors/src/Form/InlineFormErrorsProfileForm.php
   @@ -0,0 +1,62 @@
   +      if (is_a($violation->getConstraint(), 'Drupal\user\Plugin\Validation\Constraint\ProtectedUserFieldConstraint')) {
   

   Should use this class and do ProtectedUserFieldConstraint::class here. Also, why the is_a() instead of instanceof?

3. +++ b/core/modules/inline_form_errors/src/Form/InlineFormErrorsProfileForm.php
   @@ -0,0 +1,62 @@
   +          throw new \LogicException('Violation parameters should include %name to indicate the problematic field.');
   

   Exception messages should not end in punctuation

4. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +68,61 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +  protected function flagViolations(EntityConstraintViolationListInterface $violations, array $form, FormStateInterface $form_state) {
   

   Missing doc block

5. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +68,61 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +      if (is_a($violation->getConstraint(), 'Drupal\user\Plugin\Validation\Constraint\ProtectedUserFieldConstraint')) {
   

   Same as above

6. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +68,61 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +        /* @var $violation \Symfony\Component\Validator\ConstraintViolation */
   

   Should be /** @var \Symfony\Component\Validator\ConstraintViolationInterface $violation */

7. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +68,61 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +            throw new \LogicException('Violation parameters should include %name to indicate the problematic field.');
   

   Same as above

8. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +68,61 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +    if (count($current_password_violations)) {
   +    }
   

   ?

@Tim, thanks for the review, much appreciated.

1) Test added! If you set form errors with an empty string. markup is generated for the empty message, this should not happen. See attached screenshot (has one list item too many, and therefore the error message looks odd: redundant top spacing).

2) Fixed, is_a() replaced by instanceof, this is indeed better (a bit faster).

3-8) Fixed.

9) Hmm, leftover removed.

Unrelated test fail?

I re-rolled the patch for 8.3.x, to see if what looked like unrelated errors still occur.

@manumilou thanks, still the same error it seems. So we need to look into this.

It has to be related in some way then. I'll have a look as well.

This fixes the test fail. In some cases user is not available during hook_entity_type_alter().

Good job. It seems good to me. The patch is working as expected.

Thanks everyone! The before-and-after screenshots both with and without IFE are helpful.

1. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +69,62 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +      case 1:
   +        $form_state->setErrorByName('current_pass', $current_password_violations[0]->getMessage());
   +        break;
   +
   +      case 2:
   +        foreach ($current_password_violations as $i => $violation) {
   

   I don't think hardcoding 1 and 2 violations is correct. Other modules could add fields that also required the password be re-entered. Let's handle 1 and many?

   Edit: Also, a switch statement with two cases and no default also seems suboptimal/potentially fragile.

2. +++ b/core/modules/user/src/ProfileForm.php
   @@ -67,4 +69,62 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
   +            throw new \LogicException('Violation parameters should include %name to indicate the problematic field');
   

   Throwing an exception from within the form controller feels fishy.

   Also, isn't this $violation currently "whatever the last violation in the previous foreach loop was"? It's outside the foreach loop where $violation was set.

There are some ideas to change this form and split some functionality off to different forms/pages.
If that happens this patch may become obsolete.
See: #2455933: Error highlighting and reporting problems for the current password on the user profile form.

I think you meant " See: #1259892: Users could not find the Change password fields ", as 2455933 is this issue.

The most recent comment in 1259892, points to #2883755: Introduce a submit confirmation step modal to Form API for usability but not sure if that would affect this.

Patch in #55 doesn't work anymore.

Here is the same issue fixed for Drupal 8.9 and 9.1. Please check.

https://www.drupal.org/project/drupal/issues/3157993

Thanks for cross referencing, however the scope of this issue is bigger as the underlying problem is bigger, and that patch doesn't seem to address the real issues.

Rerolled for 10.1

For feedback #57 I agree the specific cases like it should change but appears to have been done for a reason just trying to figure that out.
I did add a default break though.

Adding WCAG 3.3.3 tag.