Problem/Motivation
When changing the e-mail and/or password fields on the user profile form without entering a correct 'current password' the error reporting is off. Multiple similar messages are thrown if both fields are changed and the 'current password' field is not marked as an error.
The problem is also apparent when the inline_form_errrors (IFE) module is enabled.
Original problem identified in #1493324-362: Inline form errors for accessibility and UX.
Proposed resolution
The 'current password' field should be highlighted because it is actually the problematic field. Also instead of two almost similar message, one single message should be shown, when both e-mail and password are being changed.
In case of IFE all fields need a helpful error message below the fields.
Before screenshot (Core 8.2.x without inline_form_errors module)
After screenshot (Core 8.2.x without inline_form_errors module)
Before screenshot (Core 8.2.x with inline_form_errors module)
After screenshot (Core 8.2.x with inline_form_errors module)
Comment | File | Size | Author |
---|---|---|---|
#75 | 2455933-75.patch | 6.59 KB | smustgrave |
| |||
#75 | diff-55-75.txt | 3.12 KB | smustgrave |
#45 | 2455933-45-current_password_error_TEST_ONLY.patch | 1.07 KB | dmsmidt |
#45 | empty_error_message_markup_mess.png | 57.21 KB | dmsmidt |
#38 | 2455933-38-current_password_error.patch | 6.93 KB | dmsmidt |
Comments
Comment #1
crasx CreditAttribution: crasx commentedComment #2
crasx CreditAttribution: crasx commentedComment #3
vijaycs85Like 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.
Comment #4
vijaycs85Comment #5
crasx CreditAttribution: crasx commentedAfter committing #1493324: Inline form errors for accessibility and UX there is no highlighting around the field. I'm not sure if there are other cases of the original issue, but I'm wondering if when adding dependent field error messages we should make the notification a link to the other field. That could be a follow up issue I think
Comment #6
crasx CreditAttribution: crasx commentedComment #7
crasx CreditAttribution: crasx commentedComment #8
mgiffordI 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.
Comment #9
crasx CreditAttribution: crasx commentedno fix needed. Opened follow up for link - #2514730: Inline form error reporting problems for the current password on the user profile form
Comment #10
mgiffordComment #11
SKAUGHTplease 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.
Comment #14
SKAUGHTrebuilt patch
Comment #15
SKAUGHTComment #17
SKAUGHTremove dup image. replace with related user/add UX change
Comment #18
SKAUGHTlinking to "Plural lists should use HTML lists" --> formatPlural 2 or more ('And' separated lists idea)
Comment #19
mgiffordPatch no longer applies to 8.1.
Comment #20
mgifforderror: 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.
Comment #21
mgiffordIt 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:
Comment #22
SKAUGHTThe 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.
Comment #23
mgiffordSounds 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.
Comment #24
SKAUGHTYes 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
Comment #25
SKAUGHT@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 (:
Comment #26
SKAUGHTi've opened a new issue regarding the comma list formatting. it's been bothering me alot through working on this patch.
Comment #27
SKAUGHTThis patch contains only one change to switch form deprecated Drupal::l() to Link::createFromRoute(). This has no visual change from ongoing review.
Comment #28
dmsmidtFirst 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.
Comment #29
SKAUGHTplease 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.
Comment #30
dmsmidt@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.
Comment #31
SKAUGHTComment #32
SKAUGHTcertainly 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.
Comment #33
SKAUGHTOther 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.
Comment #34
helenasue CreditAttribution: helenasue commented@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?
Comment #35
dmsmidtWoohoo! Manual test works!
Some notes:
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.
Is this safe to do? What is the route is changed by someone (which happens in custom sites)?
Nit: Replace "2" with "two".
Let's not change this. But if (1) is achieved, this is no problem.
Comment #36
SKAUGHT@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
note: line 117, 134, 159, 170, 178 -- these are conditions established by other routes and user access..
i'll check over the nit comments and address more tomorrow as i am on the road right now..
Comment #37
SKAUGHTrepair lost image in summary
Comment #38
dmsmidt@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.
Comment #39
dmsmidtThe 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?
Comment #42
SKAUGHTpatch on #38: looks good for the UX side of it. The CI test fails are a mystery to me..
wow. I do see what you mean in #39. although, i do see the point of having a note on the original field..
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.
Comment #43
tim.plunkettThis 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) {
Should
use
this class and doProtectedUserFieldConstraint::class
here. Also, why the is_a() instead of instanceof?Exception messages should not end in punctuation
Missing doc block
Same as above
Should be
/** @var \Symfony\Component\Validator\ConstraintViolationInterface $violation */
Same as above
?
Comment #44
dmsmidtComment #45
dmsmidt@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.
Comment #46
dmsmidtComment #49
dmsmidtUnrelated test fail?
Comment #51
manumilou CreditAttribution: manumilou commentedI re-rolled the patch for 8.3.x, to see if what looked like unrelated errors still occur.
Comment #53
dmsmidt@manumilou thanks, still the same error it seems. So we need to look into this.
Comment #54
manumilou CreditAttribution: manumilou commentedIt has to be related in some way then. I'll have a look as well.
Comment #55
dmsmidtThis fixes the test fail. In some cases
user
is not available duringhook_entity_type_alter()
.Comment #56
manumilou CreditAttribution: manumilou commentedGood job. It seems good to me. The patch is working as expected.
Comment #57
xjmThanks everyone! The before-and-after screenshots both with and without IFE are helpful.
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.
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.Comment #59
dmsmidtThere 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.
Comment #60
mgiffordI 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.
Comment #62
oeklesund CreditAttribution: oeklesund commentedPatch in #55 doesn't work anymore.
Comment #68
arti.singh CreditAttribution: arti.singh commentedHere is the same issue fixed for Drupal 8.9 and 9.1. Please check.
https://www.drupal.org/project/drupal/issues/3157993
Comment #69
dmsmidtThanks 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.
Comment #75
smustgrave CreditAttribution: smustgrave at Mobomo commentedRerolled 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.
Comment #76
mgiffordAdding WCAG 3.3.3 tag.