Problem/Motivation

Background

This proposal stems from the Form Error Design Wiki where we gathered feedback from accessibility stakeholders (such as Everett Zufelt) as well as UX stakeholders (yoroy, bojhan). This issue was a new attempt to build consensus around an old problem that started in the issue queue in 2009 and had 265 comments without resolution. Let's solve this for D8!

Accessibility considerations

More than color
Drupal *must use more than color alone* to indicate which fields have errors.
This is a major WCAG violation that we didn't solve for Drupal 7.
Links jump to errors
The $messages area at the top of the page will provide links to jump to errors as WCAG suggests.
Full error text inline
At the location of each form field, its error text will be presented in full. The markup order will likely put the error message between the field's label and the field itself.* The links from the $messages area will jump to the field label first so users can review the label, then the error, then the field * where they can fix the submission.
*) See Related / Child issues for crossed out parts.
ARIA described-by
Use ARIA described-by to semantically associate the error message with the field in a machine-readable, assistive technology-friendly way.

Usability considerations

  • Improve readability
  • "Prevents flooding the top of the page with long error messages" - yoroy
  • On small screens and devices, users will be able to find relevant error messages for each field without scrolling up to the top.

Stakeholders

This issue involves accessibility, usability, all four themes, and the Form system, so there are a lot of stakeholders involved:

Accessibility topic coordinators
mgifford and jessebeach
Bartik theme maintainers
jensimmons and emma.maria
Classy theme maintainers
Officially none, however, mparker17 has been talking to mortendk and JohnAlbin for direction.
Form system maintainers
There are five, three of which are active in Drupal, and one who worked on this issue:
tim.plunkett
Seven theme maintainer
LewisNyman
Stark theme maintainer
JohnAlbin
User experience and usability topic coordinators
yoroy and Bojhan

Proposed resolution

  • Move form error messages next to the form field and label.
  • Provide a brief indication of the number of errors with a link to each error in the messages area at the top of the page.

References and prior work

You should take a look at these before starting work:

Notes

  • The Seven styleguide has a design for an error with a single form control, but not a design for a complex form control (e.g.: checkbox / radio group, fieldset, password confirm). See Related / Child issues
  • emma.maria has indicated that she wants to see how Seven looks before proposing any changes to Bartik.
  • mortendk has provided feedback on the styles for Classy in #388.
  • The maintainer of the Stark theme has not yet provided any feedback on this issue.

Remaining tasks

  • Decide on a resolution
    • The resolution was "Case 3" from the Form Error Design Wiki.
      Note this wiki page has many more screen mockups of form errors in different situations such as combined form elements and mobile sized displays.
      Also note that we now move forward without error containers, because the design got this issue stuck for ages.
  • Style Seven's single-form-element errors
  • Update Seven's styles to address the ugly-background-and-border problems by changing background-color: #fcf4f2;, color: #a51b00 and border-color: #f9c9bf;, and changing the width of the border / border-radius as per #186 and #188.
  • Post screenshots for Seven so we can get a UX review.
  • Get feedback from @Bojhan or another UX expert on whether the error on the imagefield reported in #435 is acceptable or not. Disussed with @LewisNyman
  • Fix/Add inline errors for file/image fields.
  • Fix errors showing up seperately for nested elements at the top of the page, they should become links.
  • Get correct error link title for managed files, multiple value fields (and possibly others)
  • Update tests
  • Fix the styling of errors for textareas with formatting options.
  • Get a general UX review from @Bojhan or another UX expert.
  • Get feedback from Bartik theme maintainers and make appropriate changes.
  • Migrate styles common to both Seven and Bartik back to the Form Subsystem.
  • Get RTBC sign-off from all offical stakeholders.

Historic notes on compound-element errors

All compound-elements now have working inline errors.

  • Some form elements consist of multiple controls. Additional problems with these elements will be handled in separate issues (see below). The ones we know of in Drupal core are:
    • Datelist
    • Checkbox groups
    • Radiobutton groups
    • Fieldsets
    • Password-confirm widgets
  • A number of people have expressed concern about the ugliness of errors in Seven, especially with the fact that there are big/multiple red borders.
    • The current, ugly look and feel came out of the Form Error Design Wiki, case 3, which the resolution that the community chose to move forward with.
    • emma.maria pointed out that comments #186 and #188 address the ugly-border problems making it thinner and using a lighter color.

    The element styling with a red wrap went missing, but this actually makes it easier to move forward now.

  • emma.maria has reached out to the designers who came up with the Seven Styleguide to ask for a design for this type of control.
  • mparker17 has some other questions / concerns / considerations about compound-element errors. See Related / Child issues.

How to manually test this issue

  1. Pull 8.0.x HEAD and apply the latest patch
  2. Install Drupal
  3. Log in as user 1
  4. Download and install dmsmidt's Errorstyle module from GitHub https://github.com/dmsmidt/errorstyle (forked from vijaycs85's original)
  5. Choose your preferred theme from /admin/appearance
  6. Go to /error-style/form
  7. Submit the form
  8. See all the errors.

User interface changes

  • Form error messages are displayed below to the form field.
  • A brief indication of the number of errors, and a link to each error is displayed in the messages area at the top of the page

API changes

  • In previous versions of Drupal, calling form_set_error() and form_error():
    • During validation would prevent submission, mark the offending element, and use drupal_set_message() to display an error message.
      & During submission would only use drupal_set_message() to display an error message, and the element would not be marked in any way.
  • In Drupal 8, calling form_set_error() and form_error() (now FormBuilderInterface::setErrorByName() or FormBuilderInterface::setError()) will only have an effect during validation.
    Calling them during submission will do nothing. Those instances should be replaced by a drupal_set_message() call directly.

Credit

Not all of the following people might have committed code yet, but have provided extremely helpful help:

  • mparker17 for managing this issue a long time, patching and creating screenshots
  • emma.maria who has been invaluable in helping to organize and co-ordinate this issue.
  • vijaycs85 for throwing together and maintaining the errorstyle module to help test.
  • YesCT who has been invaluable in helping to organize and co-ordinate this issue.
  • dmsmidt for patching, creating a lot of screenshots and doing extensive testing and improving the test module
  • bowersox for all his initial work on this issue on GDO initially and framing the discussion here.

This issue supersedes #447816: WCAG violation: Relying on a color by itself to indicate a field validation error, which has been marked as duplicate but can be referenced for the previous discussion.

It has been decided to move the following issues outside of the current scope you can ignore them while testing:

... however, they do not block this issue from being committed.

Latest screenshots

Before applying the patch:

A screenshot of a form with lots of errors before applying the patch in this issue.

After applying the patch:

A screenshot of a form with lots of errors after applying the patch in comment #432.

Files: 
CommentFileSizeAuthor
#578 Screen Shot 2015-06-10 at 2.17.35 PM.png31 KBwebchick
#578 Screen Shot 2015-06-10 at 2.11.46 PM.png21.11 KBwebchick
#578 Screen Shot 2015-06-10 at 2.11.04 PM.png33.51 KBwebchick
#576 errors.mp43.6 MBManjit.Singh
#573 inline_form_errors_for-1493324-573.patch67.65 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,542 pass(es).
[ View ]
#559 interdiff-558-559.txt1.51 KBstefan.r
#559 1493324-inline-form-errors-559.patch67.7 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1493324-inline-form-errors-559.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#558 interdiff-557-558.txt1.51 KBstefan.r
#558 1493324-inline-form-errors-558.patch66.46 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,193 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#557 1493324-inline-form-errors-557.patch64.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,201 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#556 1493324-556.patch57.97 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#544 interdiff.txt2.8 KBtim.plunkett
#544 1493324-inline-form-errors-544.patch66.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,457 pass(es).
[ View ]
#538 1493325-review.png76.39 KBkattekrab
#524 interdiff-1493324-522-524.txt1.61 KBdmsmidt
#524 inline_form_errors_for-1493324-524.patch68.96 KBdmsmidt
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,477 pass(es).
[ View ]
#505 1493324-505-inline-errors-after.png384.99 KBdmsmidt

Comments

bowersox’s picture

StatusFileSize
new103.88 KB
mgifford’s picture

This really is ready to get into core for D8. A sandbox would be useful for this, but mostly we just need some patches to start considering.

Brandon, thanks so much for pushing through with this.

chertzog’s picture

Just in case people have not seen these yet, there are two modules in contrib that attempt to do this.

http://drupal.org/project/inline_messages
http://drupal.org/project/ife

I have not yet tried these out, but they both have D7 branches.

bleen18’s picture

StatusFileSize
new5.28 KB
FAILED: [[SimpleTest]]: [MySQL] 35,260 pass(es), 27 fail(s), and 1,820 exception(s).
[ View ]
new5.28 KB
FAILED: [[SimpleTest]]: [MySQL] 35,243 pass(es), 34 fail(s), and 1,822 exception(s).
[ View ]

Ok ... here is a first swing at a patch.

There are few things that are not good/not ready for prime-time:

  • We should really be checking if an element has an error long before we get to the theme layer (right now I'm checking in theme_form_element) but I'm not sure where the right place might be. I suppose a preprocess function would be better, but still not good. Ideas?
  • I have not tested this at all if a single element has more than one error.
  • The link in the top error message does not work in overlay at present
  • Currently I am adding "messages" and "error" classes to the form element but that is not correct. Should I add these class definitions to system.css? If not, where?

This needs tests, and I'm fairly certain that this will fail some tests...

bleen18’s picture

Assigned:Unassigned» bleen18
Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new79.09 KB

oops .. uploaded the patch twice instead of the screenshot
screenshot

bleen18’s picture

StatusFileSize
new5.31 KB
FAILED: [[SimpleTest]]: [MySQL] 35,252 pass(es), 38 fail(s), and 1,820 exception(s).
[ View ]

This patch fixes the anchor links so that they work in the overlay, but if there are multiple anchor links and you click one of them, then scroll up and click another everything dies.

Not sure how to create those links so that they always ... ya know ... work

bleen18’s picture

StatusFileSize
new36.36 KB
new2.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1493324_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

EDIT: ignore the patch in this comment, see #10

This patch should solve many of the test fails (but not all). In addition it puts the css in the correct place and adds the "X" icon to help separate the error message from the title... Still havent figured out a better place to grab the error than the them_form_element(). Must figure that out...

As for the issue with multiple anchor links in the overlay: #1542472: Clicking on multiple anchor links while in overlay causes a page refresh potentially causing form data to be lost

error

bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new6.54 KB
FAILED: [[SimpleTest]]: [MySQL] 35,271 pass(es), 24 fail(s), and 1,816 exception(s).
[ View ]

Oooops .. wrong patch. Lets try this

bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new6.53 KB
FAILED: [[SimpleTest]]: [MySQL] 35,352 pass(es), 21 fail(s), and 0 exception(s).
[ View ]

Ok .. this patch solves a bunch of the tests locally. Lets see what testbot says

bleen18’s picture

Here is an update on the current patch and the remaining issues:

  • We should really be checking if an element has an error long before we get to the theme layer (right now I'm checking in theme_form_element) but I'm not sure where the right place might be. I suppose a preprocess function would be better, but still not good. Ideas?
  • I have not tested this at all if a single element has more than one error.
  • The link in the top error message does not work in overlay at presentThere is a bug in overlay when you click an anchor link after you have already clicked one. See #1542472: Clicking on multiple anchor links while in overlay causes a page refresh potentially causing form data to be lost
  • Currently I am adding "messages" and "error" classes to the form element but that is not correct. Should I add these class definitions to system.css? If not, where? Fixed
  • A from that redirects the user on submit even if an error occurs is not handled properly. For an example, try using the search block without entering any keywords. You should be directed to /search/node and see this error: Please enter some keywords. Since we are no longer using drupal_set_message at the time of the error is discovered (via form_set_error) the error is no longer stored in a session early enough.
  • I would like to add "next/prev error" anchor links below each element that has an error. This should be relatively easy once we figure out the issues above.
bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new8.4 KB
FAILED: [[SimpleTest]]: [MySQL] 35,232 pass(es), 77 fail(s), and 1,117 exception(s).
[ View ]

Here is an update on the current patch and the remaining issues:

  • We should really be checking if an element has an error long before we get to the theme layer (right now I'm checking in theme_form_element) but I'm not sure where the right place might be. I suppose a preprocess function would be better, but still not good. Ideas?
  • I have not tested this at all if a single element has more than one error.
  • The link in the top error message does not work in overlay at presentThere is a bug in overlay when you click an anchor link after you have already clicked one. See #1542472: Clicking on multiple anchor links while in overlay causes a page refresh potentially causing form data to be lost
  • Currently I am adding "messages" and "error" classes to the form element but that is not correct. Should I add these class definitions to system.css? If not, where? Fixed
  • A from that redirects the user on submit even if an error occurs is not handled properly. For an example, try using the search block without entering any keywords. You should be directed to /search/node and see this error: Please enter some keywords. Since we are no longer using drupal_set_message at the time of the error is discovered (via form_set_error) the error is no longer stored in a session early enough. Should be fixed by this patch.
  • I would like to add "next/prev error" anchor links below each element that has an error. This should be relatively easy once we figure out the issues above.
bleen18’s picture

StatusFileSize
new7.75 KB
FAILED: [[SimpleTest]]: [MySQL] 35,343 pass(es), 13 fail(s), and 129 exception(s).
[ View ]

A couple fixes for stuff I broke in #15

Bojhan’s picture

Oops, I didn't follow this one. Sorry!

Can anyone tell me why we have a bounding box + icon? I thought the consensus was that we had a signal like "!" or so.

bleen18’s picture

@Bojhan: I added the icon because the error and the title looked confusingly similar

In other news, the current patch has issues with checkboxes/radios ... both the group and the individual form elements get the error state. Wont have time to look until later this week.

mgifford’s picture

These are definitely good patterns. I do wonder if on a code level we should also be looking at adding HTML5′s required attribute as well as ARIA’s "aria-required".
http://www.w3.org/TR/2011/WD-html5-20110525/common-input-element-attribu...
http://www.w3.org/TR/wai-aria/states_and_properties#aria-required

This coming from @feather's articles on required field best practices:
http://simplyaccessible.com/article/required-fields-right/
http://simplyaccessible.com/article/required-form-fields/

And also worth pointing to:
http://www.alistapart.com/articles/aria-and-progressive-enhancement/

bowersox’s picture

@mgifford, My feeling is that we should add both the ARIA and HTML5 markup to indicate required fields. However, I believe that is a separate issue and we should not try to tackle it in this issue. I know the markup for required fields introduces some complexity about the ability to use the "Cancel" or "Previous Step" buttons in multi-step forms, so it should definitely be a separate issue that we address carefully.

mgifford’s picture

Issue tags:+FAPI, +Required Fields, +a11ySprint

Good point. Adding #1623098: Add HTML5 & ARIA for Required Form Elements and also tagging this issue for the sprint.

bleen18’s picture

Assigned:bleen18» Unassigned

I clearly have not had a lot of time to try and plow through some of the roadblocks I've come across already ... I still plan on working on this, but I dont want anyone to wait on me if they want to take a swing so I'm unassigning myself

bleen18’s picture

Ok ... I've been looking at this issue again and I'm determined to figure it out. I need some feedback though:

I think that the only to fix for https://skitch.com/bleen/eyhxf/site-information-d8 is to add another FAPI property called "#hide-errors" (or something) and automatically apply that to radio's that are part of radio groups and checkboxes that are part of checkbox groups. This property might be useful anyway, but I'd love to get another opinion.

I dont want to go too far down this rabbit hole only to find out that there are strong objections to adding a new FAPI property.

Thoughts?

mgifford’s picture

Issue tags:+form validation

Linking to related issue about validation of complex form elements #179932: Required radios/checkboxes are not validated

@bleen18 - are these types of errors all tied to compound forms that probably should be in a fieldset anyways? #504962: Provide a compound form element with accessible labels

I'd just like to know how to repeat that nasty error you got. Not opposed to the #hide-errors. Something we might be able to talk about on the weekend at the a11y code sprint.

bleen18’s picture

Mgifford: currently these errors are only tied to radios elements and checkboxes elements but I havent tested much with more complex forms yet. Basically it would effect any form elements that share a #name the way the radio element (correctly) shares a name with its parent radios element. Makes sense?

bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
new10.58 KB
FAILED: [[SimpleTest]]: [MySQL] 36,874 pass(es), 20 fail(s), and 129 exception(s).
[ View ]
new111.41 KB

This patch:

  • Adds a simple error message (drupal_set_message) with links to the individual errors
  • Adds the detailed error message to each individual element that has an error
  • Introduces a new FAPI property to all elements called #hide_errors. When TRUE, the inline error is not displayed with the element. This might need to be thought out further but it is needed in some form to get radios & checkboxes to work
  • Next/Prev error navigation at the bottom of each element

Still to do:

  • Tests
  • Think through the ramifications of the #hide_errors property
  • Documentation (after patch is committed)

Important Notes:

  • The attached foo module alters the admin/config/system/site-information form for easier testing
  • I'm setting this to needs review to see how badly the tests are failing.
  • It would be great to get some feedback and get some folks to apply this patch and start really testing.

Site information | d8.png

Bojhan’s picture

Status:Needs work» Needs review

@bleen18 Thanks for keeping this issue moving, I really love to get this in soon so we can usability test it.

I don't really like that during this thread, we tend to randomly add functionality. It's taken quite a while to reach concencus on some of the fundamental parts, lets get those in first. A quick review;

1 ) Lets lose the icons, per field - I never liked the icon, and it only adds a whole lot of clutter on the page.
2 ) The error should be next to the label, not above.
3) The next/previous is to me, functionality that hasn't been agreed upon - so lets keep that out of the patch.

I honestly, don't see how the fieldset is adding anything. If I am color blind, I have no idea what a "gray" box around a field communicates. As some who can see this color, it adds a whole lot of unnecessary clutter. I thought we just needed some signaling, like a "!" in front of every error.

andrewmacpherson’s picture

Status:Needs review» Needs work
StatusFileSize
new49.14 KB

There's an interesting problem with the Confirm Password field from user.module

When submitting mismatched passwords, the confirm password field is correctly highlighted, but isn't included in the messages area. Here's a screenshot from /admin/people/create. I deliberately chose a username which I knew already existed, and provided mismatched passwords, so there are two errors. The messages area reads: "2 errors have been found: , Username".

The problem is that the confirm password element doesn't have a #title property. Instead, it has two password elements as children, which have their own #title properties. So we need to find a more reliable way to include the field name in the messages area. The test results from comment 16 shows approx. 100 failures from form_display_errors(), so I guess this problem extends to other fields too.

Also note the position of the comma. form_display_errors() isn't listing the error-fields in the same order that the fields appear in the form, which is somewhat confusing.

andrewmacpherson’s picture

The links in the error-messages area aren't very easy to use if you're a sighted keyboard-only user.

An example scenario: visit /admin/people/create, and trigger an error by providing a username which is aleady in use (e.g. admin). When the form is returned with errors, try to correct them using only the keyboard...

  1. I use the skip-to-main-content link (TAB, ENTER).
  2. Next, I TAB through the error messages until the "username" link has focus, and follow it by pressing ENTER.
  3. The page shifts, and now the Username text input is at the top of the view-port. So far, so good.
  4. However, I don't see any indication of focus. If I start typing some letters, nothing happens. Clearly the text input hasn't been brought into focus.
  5. So I press TAB
  6. What the heck? Now the email address text input has focus. That's not the input I was trying to reach.
  7. So now I have to go backwards through the focus order. I press SHIFT + TAB, and finally arrive at the username input.

(This test was using Firefox 13 on Linux + KDE.)

Currently the error-message links use the id attribute of the elements as their href. Using the link changes your place in the tabindex order, but it doesn't actually focus the input.

Suggestions:

  • Instead of using the id attribute of the target input, we could generate an id attribute for the associated element. Assuming the label precedes the input, TAB should then focus the correct input. However, it's probably unwise to assume the label always precedes the input.
  • Alternatively, we could generate an id for the form-item wrapper div, and target that. The desired input would then come next in the tabbing order. I think this would be more reliable.

Another weird behaviour I sometimes observed: after following the shortcut to the username field, the page shifted in the viewport, and the username input was concealed beneath the toolbar, so even if it did gain focus, you wouldn't be able to see what you were typing.

bleen18’s picture

Re #29:

1 & 2) I'm not married to the icons, but putting the error next to the field labels extremely problematic for long field labels and terribly confusing when the error appears on a non-required field (aka with no "*") offering any kind of separation. Suddenly the field looks like its title & the error message are concatenated into one string. I feel pretty strongly that the error should not appear next to the title for these reasons; I dont feel strongly at all that we need the icon.

3) This issue is about UX. If there is a good usability argument for getting rid of the next/prev links I think we should definitely discuss but just because these links were not in the screenshot in the original is not a good reason to nix them. Think about the UX on a long form:

  • I see many errors linked at the top
  • I click the first and address the error
  • I have to scroll all the way back up to click the next link
  • Rinse and repeat

Thats a terrible UX compared to:

  • I see many errors linked at the top
  • I click the first and address the error
  • I click the "next error" link and address the error
  • Rinse & repeat ...

Can we at please keep this open for discussion?

Re #30:
Thanks for taking a swing with this ... I knew there'd still be some problems to deal with.

So we need to find a more reliable way to include the field name in the messages area.

Anyone have any ideas about this one? Off the top of my head, nothing seems to come to mind.

[edit] I hadnt finished that last sentence before hitting save - DOH!

nod_’s picture

Well if you really want that kind of link inside error messages, why not a link "return to top" instead of next/previous? You don't have to scroll to the top.

That said prev/next feel way to "invasive" to put by default in core.

bleen18’s picture

re #31: hmmm ... this one is going to suck but definitely needs to be addressed. The biggest challenge I see is that some form elements are groups of elements (ex: radios) and some are not.

Maybe we can add an ID to the error message itself a link to that? Then the tabbing *might* work out more better? Maybe?

Meal replacement shake’s picture

I agree with Bohjan. It seems focus gets lost and we are not maintaining focus on the primary issues at hand.

bowersox’s picture

@bleen18: I agree with Bohjan too. Even though I see your argument about the "next" links being helpful to users rather than scrolling, I still feel we should stick to the consensus that was reached. Later improvements can become another issue later if we can build consensus around that.

We have spent since April 2009 on this issue (it started as #447816). Let's please get the part we all agreed on into Drupal 8.

bleen18’s picture

This issue is about improving the accessibility and usability of form errors. I appreciate that there was a lot of work done prior to my getting involved in this issue and I don't mean to minimize that but at the end of the day we all want an improved experience, right? That said, I don't believe there was any consensus around either of these two issues in the wiki for this issue.

1) regarding the position of the error next to the label: Evan Donovan made this comment, http://groups.drupal.org/node/209513#comment-710049, where he made the identical point I am making: the error should be on a separate line. That comment was not discussed or responded to. No consensus was had...

2) the next/prev navigation: there was no consensus around this because it was never discussed. In the wiki the only example of a "very long form" is a form that has only 7 fields. This is minuscule. There was no discussion at all about how this would work with a form that has dozens of fields. I maintain several modules that have forms like this (DFP and Dart for instance) and without the next/prev nav (or some other solution) I would argue that what we have now is more usable (though definitely not as accessible). Disclaimer: both of those modules use v tabs heavily to improve the UX of the large forms but even still there are individual tabs with far more than 7 fields

I understand that there was discussion ahead of time on this issue and I have been involved in several d.o. issues where someone has come along and tried to derail a consensus but I have also been involved in issues like that where someone has come along and derailed the consensus because something important had not been considered. The reason I am being stubborn here (you can look back at other posts of mine and see that I am never stubborn like this) is because it is this exact UX problem that my users suffer and that I want to see fixed (errors on very long forms).

Consensus is good. Consensus is important. But a good consensus should not prevent us from making good adjustments. If you don't think these are good improvements, please make those arguments and I will gladly listen but don't just dismiss them because no one brought them up 4 months ago.

Final note: this is my last plea... If people still think we should just do exactly what is in the original post without any changes at all I will not argue after this. Although I've always though that stepping up and actually bringing the code held more weight in this community...

Bojhan’s picture

@bleen18 You are totally right, and I am sorry. The reason why we might respond somewhat harshly, is because its been a really though issue to reach concencus on - that is however no excuse not to respond to your suggestion. From my point of view we have reached concencus on the fact that we need; error links on top, error messages inline and some visual signaling inline that there are errors.

Let me argument, why I think this particular improvement of placing next/prev links isn't good:

1) The functionality if I understand correctly, it is primarily nice to have when there are many errors and when the user is navigating by keyboard. To me both the first and last argument, do not apply to a majority use cases and our audience.

2) It adds controls that are optimized for keyboard users, visual users are going to be far more adapt to scanning the left side - finding the red labels. I find it highly unlikely, one would scroll to the top - to go to the next error.

3) The links as they are now, require some visual boxing - such as a fieldset. I don't think we need a fieldset, therefor it will either require redesign of these elements or make these "hanging in air".

4) Last is the fact that it takes up a lot of screen estate, for something that is not required. If you look at how this could look on mobile, we are getting into quite messy waters.

Again sorry, for not responding on your suggestion initially.

bleen18’s picture

Ok .. these are some discussion points I can work with :). Lets see if we cant go through them and either come to a general agreement why these links should stay, change or go...

1) Yes, you are correct. these links are most useful for a form with several errors. If there is only one error, no links would exist and if there are only two, the first one would only have the "next" and the last would only have the "prev". I agree that in a majority of cases no one would see any of these links in which case we're both happy :) In the rarer case of multiple form errors however, that is when these links are most useful

2) I've tried writing a response to this one a couple of times and so far I have (internally) vehemently agreed with you and vehemently disagreed ... It just feels like if we can make it easier we should. One thing I will say is that if we ultimately decide to nix these next/prev links perhaps we should keep them for keyboard users the way that the "skip to" links work - I'm not quite ready to concede the point yet, but it's worth thinking about.

3) This one I dont understand. You briefly mentioned fieldsets earlier in #29 (I think) and I forgot to ask about it then, but as it stands there are no fieldsets being added anywhere. Can you elaborate.

4) My argument here is (again) it only takes up that extra real estate when it is useful ... and scrolling on a mobile device (an already crappy experience) is exactly the time I think this functionality is most useful.

Does the fact that these nav elements only appear when there are multiple errors help alleviate your concerns? Also, a disclaimer: I barely gave any thought to the positioning/styling of those next/prev links so if there are suggestions on that front that might help to settle this that could work too.

Thanks for talking this out with me ... I'm sure that we'll come to a good conclusion at the end. In the mean time I'll try and spend some time this weekend on the issues raised in #30 & #31 as this is all moot if those bugs dont get squashed.

bleen18’s picture

One other question unrelated to the next/prev debate...

The FAPI property I added is called #hide_errors:

  • Should this be #show_errors instead (thus reversing the logic)?
  • In either case, should I be more specific: #hide_inline_errors?
  • In general I'm nervous about this property. I fear it may get more complex later. But I could not come out with any other reasonable solution to showing an error for a radio group without also showing it on all the individual radio elements. Any feedback here would be uber helpful...
nod_’s picture

We seem to assume we're going to have a form with lots of errors here, I agree with bojhan that for the typical use-case this is not true.

Showing form validation errors is an UX fail in and of itself. It means there is no client-side validation (HTML5 can do that to some extend) and that the descriptions/labels were confusing, not explicit enough.

While I understand your point bleen18, I think making the HTML5 integration of form validation would be a better way of addressing this issue because if you can reduce the number of errors to begin with, this simplify the problem. When validation error do happen the agreed upon way of dealing with it might not be the best, but it's good enough. What's important is that contrib is still able to tweak it and add prev/next buttons to form errors.

bowersox’s picture

Regarding the question raised in 40, I think we can handle nested elements with logic like this:

foreach ($element in @form) {
  if (isset($element['parent']['error']) {
    // do not give this element the red box and error icon, because the parent will get those
  } else {
    // this element needs the red box and error icon
  }
}

Hopefully with that pseudo-code idea we can do this without introducing a new FAPI #hide_inline_errors property.

bowersox’s picture

Regarding the prev/next links discussed in 38 and 39, my feeling is that they are more harm than help. They add a lot of UI clutter/overwhelm without delivering much help to users. At the top of the page there is a summary of the errors with a link to each erroneous field. Any user who is having trouble finding which fields have errors can return to the top of the page and use those links. Scrolling back to the top of the page is a quick gesture on a trackpad or a 1-finger touch at the top of an iPhone/iPad screen.

@bleen18, I do want to thank you for your passion to make this UI good and your willingness to explain your arguments and work through this decision together.

bleen18’s picture

#42: there is no element[parent][error] ... That would be stupendous though as it would solve this particular issue perfectly.

Re next/prev ... I appreciate that people have started to make some decent arguments against the merits of this functionality (and not just the fact that it is a change) and it seems that most of them make reasonable points against the proposal so I'm going to remove it from my next patch (after some much deserved BBQ).

bleen18’s picture

StatusFileSize
new9.31 KB
FAILED: [[SimpleTest]]: [MySQL] 40,050 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

Ok folks ... I need some guidance:

This patch does 3 things:

  • Adds a function (which is ugly, kludgy and needs to die a fiery death) called form_element_get_title(). This function takes a form element and returns the title, or it searches the element's children until it finds a title. This solves one of the issues brought up in #30 about relying on title. This solution is crappy because it might choose a pretty random title that is not so helpful; we could still end up with no title if the element nor any of its children has a title; to add this ridiculous function makes my brain unhappy. So far the only viable solution I can think of for this issue is to make #title a requirement for all form elements but this would be a HUGE change and probably not a very welcomed one. YOUR FEEDBACK HERE
  • Adds the #hide_errors property to the password/confirmation form. This highlights my concerns (raised in #40) about the fragility of this new FAPI property. We will now need to be on the lookout for form errors that may get duplicated and add #hide_errors to that form element with no good way to find all the occurrences in core where this might be an issue. This will also effect contrib down the road. YOUR FEEDBACK HERE
  • Removes the next/prev links
bowersox’s picture

Feedback on these first 2 items:

  • There is actually a proposal to make #title required: #933004: Test that all form elements have a title for accessibility. I would say a fall-back if an element has no #title is just to say a generic message like "Error in text field", based on the #type. Of course that would still be link down to the field on the page. Maybe if we do those things we don't need to walk the children to try to find a #title.
  • I am still thinking we should try to avoid adding a #hide_errors property, if we can. What about we just have smart logic for password fields to only display an error once. Another option would be to try making something like what's suggested in 42 possible by changing the data structure to be have the property you would need (like element[parent][error]).

Is that feasible?

bleen18’s picture

Requiring title would work great but I like your suggestion of "error in this text field" as a fallback.

As for your point about hide errors...

  • This effects radios as well as checkboxes as well as the passwords as well as any complex custom form elements that contrib has... This is why it makes me nervous, but I can't think of a good alternative
  • Setting parent[errors] does not seem possible as far as I can tell because the children are checked for errors before the parent. If you have thoughts on how that suggestion might work I'd love to hear it ...

More feedback welcome.....

sun’s picture

+++ b/core/includes/form.inc
@@ -902,9 +902,16 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+      // Occassionally a form will set an error in its submit handler so we make
+      // sure to display those errors here before the form is redirected.
+      form_display_errors($form);

We do not babysit broken code, and setting a form validation error after form validation has completed is broken. This can/should be removed.

+++ b/core/includes/form.inc
@@ -902,9 +902,16 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+    else {
+      form_display_errors($form);
+    }

1) It looks like this should be moved to the end of drupal_validate_form() instead.

2) We likely want to limit this additional processing to non-programmed forms.

+++ b/core/includes/form.inc
@@ -3048,6 +3137,8 @@ function form_process_radios($element) {
+        // Errors should only be shown on the parent radios element.
+        '#hide_errors' => TRUE,

Need to think about this idea some more, but FWIW, I think that this should rather read #error_use_parent or similar. #hide_errors is absolutely misleading.

bleen18’s picture

We likely want to limit this additional processing to non-programmed forms.

I'm not sure what you mean by this... can you elaborate?

Need to think about this idea some more, but FWIW, I think that this should rather read #error_use_parent or similar. #hide_errors is absolutely misleading.

Are you mulling over the idea of using a property like this? or just the name of the property (which I agree needs changing)? I dont want to keep going too far down this rabbit hole if this solution is ultimately a dead end.

mgifford’s picture

@bleen18 - With form_display_errors($form), why was it added here? Can it be moved up in the processing so that we're not validating it twice?

@sun - any thoughts on @bleen18's response? Just want to get some clarification here. Might make sense to do this on IRC. What's the best way to get feedback from you on this issue?

sun’s picture

re: #49:

i) I meant that the specific call to form_display_errors() I referenced should be wrapped in a if (!$form_state['programmed']) condition.

btw, it looks like you can skip the else, because if the initial if condition is positive, then the call to drupal_redirect_form() will end the request.

ii) I meant the name of the #property only.

Overall, I need to spare some time to give this patch some more thought though. However, it would still be a good idea to get these known issues out of the way already.

bleen18’s picture

This patch addresses @sun's comments in #48 and in doing so address @mgiffords comment in #50.

The big issues that still remain include:

  • If a form element has no title, the link in the message at the top of the page is not acurate (see #45)
  • The new form element property #error_use_parent (formerly #hide_errors) still seems like a fragile solution to me but necessary as of now to handle errors on radios, checkboxes and any form element that has children
  • Still needs testing with form elements that accept multiple values
bleen18’s picture

Status:Needs work» Needs review
StatusFileSize
new8.71 KB
FAILED: [[SimpleTest]]: [MySQL] 40,048 pass(es), 15 fail(s), and 2 exception(s).
[ View ]

oops ... forgot to attach patch

mgifford’s picture

Looking at @yatil's js code now - https://github.com/yatil/accessible-form-js

Probably not something we can draw on, but interesting model.

bleen18’s picture

@sun ... re: #38

We do not babysit broken code, and setting a form validation error after form validation has completed is broken. This can/should be removed.

So what should we do in the case of the test that is failing on the search block? In that case, of search_box_form_submit? In that form submit function errors are set but there is no way to pass the test that makes sure the user is redirected if the form is empty if that validation functionality is moved from form submit to a validation function. (http://api.drupal.org/api/drupal/modules%21search%21search.test/function...)

Thoughts?

mgifford’s picture

Could an exception be made for search?

@bleen18 Are there other examples where where we might need form validation done there?

bleen18’s picture

#57 that was the only one i saw, but once I found it I stopped looking. The point here though is that even if we just have one exception, then i would need to put back the code that @sun suggested I remove in #38.

That code would handle all exceptions and it couldn't easily be altered to only handle one or two exceptions if that was the desired functionality.

Basically we are talking all or nothing: we either put in code that handles form_set_errors in a form_submit function, or we dont.

sun’s picture

search_box_form_submit() is the exact definition of broken code. ;) I wasn't aware we have that in core. Let's create a separate issue to fix that (but check for a possibly existing first, please).

bleen18’s picture

mgifford’s picture

@sun can this patch proceed on it's own now that there is a separate issue for search_box_form_submit()? Also, I'd like some ideas on how to better tag @bleen18's new patch. Not sure if a WTF tag is the best way to get folks to look at and review this issue.

I'd just like to see that we can't have better inline form error handling in D8 at the moment and want to see that we address these. We could mark it as postponed possibly, but not sure how related it needs to be.

mgifford’s picture

Great to see that there is now a patch to try out #1789768: search_box_form_submit() improperly calls form_set_error() to deal with the search_box_form_submit() issue.

I really think we should be able to keep working on the patch in #53 at the same time though.

franz’s picture

+++ b/core/includes/form.inc
@@ -955,6 +955,52 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+ return $title;
+++ b/core/includes/form.inc
@@ -1641,11 +1689,13 @@ function form_get_errors() {
+ return $form[$key];
+++ b/core/includes/form.inc
@@ -1940,6 +1991,39 @@ function form_builder($form_id, &$element, &$form_state) {
+ $element = form_get_element($element_key, $form[$key]);
+ if (!empty($element)) {
+   break;
+ }

Need to use soft-tabs, your editor is inserting tab characters.

mgifford’s picture

This is important to fix, but the bigger issue is that it seems like the whole diff needs to change and no longer applies due to some recent submits.

$ git apply 1493324_9.patch
1493324_9.patch:71: tab in indent.
return $title;
1493324_9.patch:119: tab in indent.
return $form[$key];
1493324_9.patch:157: tab in indent.
$element = form_get_element($element_key, $form[$key]);
1493324_9.patch:158: tab in indent.
if (!empty($element)) {
1493324_9.patch:159: tab in indent.
  break;
error: patch failed: core/includes/common.inc:6734
error: core/includes/common.inc: patch does not apply
error: patch failed: core/includes/form.inc:865
error: core/includes/form.inc: patch does not apply

I tried to just quickly do an upgrade manually but too much has changed since August 16h when this patch was rolled. Looks like it needs to be rebuilt...

franz’s picture

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new7.72 KB
FAILED: [[SimpleTest]]: [MySQL] 48,008 pass(es), 18 fail(s), and 2,348 exception(s).
[ View ]

Re-rolled against HEAD.

This still needs a lot of work.

mgifford’s picture

Patch still applies nicely, but there are 17 fail(s) in the tests.

Hard to do the manual testing with - #1797438: HTML5 validation is not fully accessible - as the browser validation gets in the way.

Owen Barton’s picture

Status:Needs work» Needs review
Issue tags:-Usability, -Needs tests, -FAPI, -User interface, -accessibility, -form validation, -Required Fields, -a11ySprint

#66: drupal8.form-error-inline.66.patch queued for re-testing.

bleen18’s picture

re #68: ... to test I used firebug/chrome to remove the "required" attribute from the input manually after the page had loaded. Not ideal, but Drupal doesnt know the difference and it stops HTML5 validation from pestering you

attiks’s picture

FYI: I created #1845546: Implement validation for the TypedData API to try to solve this on all levels, since there are only 10 days left, we need to figure out what needs to be done before feature freeze and what can be done later.

mgifford’s picture

Can someone re-roll @sun's patch from October? There's more progress now on #1789768: search_box_form_submit() improperly calls form_set_error()

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new7.73 KB
FAILED: [[SimpleTest]]: [MySQL] 50,315 pass(es), 21 fail(s), and 4,712 exception(s).
[ View ]

Re-roll

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new423 bytes
new7.84 KB
FAILED: [[SimpleTest]]: [MySQL] 55,824 pass(es), 19 fail(s), and 2 exception(s).
[ View ]

This should fix at least all the notices.

mgifford’s picture

Status:Needs work» Needs review

#76: drupal8.form-error-inline.76.patch queued for re-testing.

mausolos’s picture

(Drupalcon PDX code sprint) I'll take a look at this.

mausolos’s picture

Status:Needs work» Needs review
Issue tags:-Usability, -Needs tests, -FAPI, -User interface, -accessibility, -form validation, -Required Fields, -a11ySprint

#76: drupal8.form-error-inline.76.patch queued for re-testing.

mausolos’s picture

Still fails, and I got distracted with other stuff. Out of time, might take another look some other time.

falcon03’s picture

falcon03’s picture

mgifford’s picture

Status:Needs work» Needs review
Issue tags:-User interface, -mobile+user interface bug, +blackberry mobile
StatusFileSize
new7.86 KB
FAILED: [[SimpleTest]]: [MySQL] 59,538 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

system.theme.css moved into css/

mgifford’s picture

Issue tags:+User interface, +mobile

I'm pretty sure I didn't intend to remove these (User interface, mobile) tags.

Looks like a few tests need to be adjusted.

cbiggins’s picture

My only concern with the current solution (which looks great otherwise) is that there is no correlation or relationship between the field error and the field, besides a wrapping div.

I'm wondering if there is something that can be done to establish a connection between the two, an aria describedby perhaps?

Otherwise, it looks good. The jump link in the top errors works and is clear, the wrap and error on each field is clear and obvious.

mgifford’s picture

Status:Needs work» Needs review

#86: drupal8.form-error-inline.86.patch queued for re-testing.

mgifford’s picture

I just lost a much bigger comment and I'm running out of time, so quickly.

I think this is a good idea. Follows the example provided here:
http://www.karlgroves.com/2011/10/10/accessible-form-labeling-instructions/

I think it can be done with some simple changes to function _form_set_attributes() or form_builder(), but it means that we're going to have to ensure that describedby can handle multiple attributes.

Main concern I had was that we should be focusing on the existing errors so that we can get the existing code into D8. If we can add in this additional semantic relationship that would be a big bonus. We just need to see more progress on this issue.

nod_’s picture

Status:Needs work» Needs review
Issue tags:-Required Fields, -user interface bug, -blackberry mobile, -#d8uix+styleguide
StatusFileSize
new7.86 KB
FAILED: [[SimpleTest]]: [MySQL] 58,716 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Reroll, might need some styleguide love too. All that red is really aggressive :p

Bojhan’s picture

What are we doing? That it changes style?

Xano’s picture

Removing some unneeded tags.

+++ b/core/includes/form.inc
@@ -2029,6 +2077,38 @@ function form_builder($form_id, &$element, &$form_state) {
+function form_get_element($element_key, $form) {

Shouldn't we use NestedArray::getValue() for this?

mgifford’s picture

@Xano so it should be something more like this:

+function form_display_errors(array $form) {
+  if ($errors = form_get_errors()) {
+    $error_links = array();
+    foreach ($errors as $key => $error) {
+      $element = NULL;
+      $element = NestedArray::getValue($form, '', $key);
+      if ($element) {
+        $title = form_element_get_title($element);
+        $error_links[] = l($title, '', array('fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE));
+      }
+      else {
+        drupal_set_message($error, 'error');
+        unset($errors[$key]);
+      }
+    }
+
+    if (!empty($error_links)) {
+      drupal_set_message(format_plural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links), 'error');
+    }
+  }
+}

And we can just drop form_get_element().

Let me know if that's right and I'll do up a new patch. The bigger issue though is still the 11 fail(s).

larowlan’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new7.68 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Re-roll after FormBuilder service

mgifford’s picture

I didn't see any problems implementing this patch in STM. I'll try to retest it.

mgifford’s picture

98: inline-errors-1493324..patch queued for re-testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new556 bytes
new7.68 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Whoops

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new7.71 KB
new13.51 KB
FAILED: [[SimpleTest]]: [MySQL] 59,849 pass(es), 26 fail(s), and 15 exception(s).
[ View ]

Layer upon layer

mgifford’s picture

Status:Needs work» Needs review

105: inline-errors-1493324..patch queued for re-testing.

tim.plunkett’s picture

  1. +++ b/core/includes/form.inc
    @@ -2771,8 +2777,11 @@ function theme_form_element($variables) {
    +  $variables['errors'] = \Drupal::formBuilder()->getError($element);

    This is a huge problem. We're actively trying to REMOVE global calls to form_get_errors(): https://drupal.org/node/2131851

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1793,4 +1816,82 @@ public function setRequest(Request $request) {
    +          $error_links[] = $this->linkGenerator->generate($title, $this->request->get(RouteObjectInterface::ROUTE_NAME), $this->request->query->all(), array('fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE));

    This is really bad :( First, you probably want $this->request->attributes->get('_raw_variables')->all() as the second param, but this is really strange.

jessebeach’s picture

It seems that timplunkett and larowlan are working on refactoring form error handling here #2131851: Form errors must be specific to a form and not a global. We should hold off this issue and consult with them on the other one.

bleen18’s picture

@jessebeach .. the link in #110 is to this issue. Is there another issue?

tim.plunkett’s picture

jessebeach’s picture

@bleen18, sorry, I've updated it to #2131851

jessebeach’s picture

The form errors issue is passing tests #2131851: Form errors must be specific to a form and not a global.

Lets go through it and see if we can port the accessibility changes developed here on top of that proposed patch.

mgifford’s picture

StatusFileSize
new13.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,853 pass(es), 299 fail(s), and 236,787 exception(s).
[ View ]

I think it mostly applies. The only error was in core/lib/Drupal/Core/Form/FormBuilder.php

Where we were using:
'#error_use_parent' => FALSE,

But the present code in Core uses NULL:

    // Assign basic defaults common for all form elements.
    $element += array(
      '#required' => FALSE,
      '#attributes' => array(),
      '#title_display' => 'before',
      '#errors' => NULL,
    );

Here's a re-roll of the patch. I'm not actually sure how well it works.

tim.plunkett’s picture

  1. +++ b/core/includes/form.inc
    @@ -2736,8 +2742,11 @@ function theme_form_element($variables) {
    +  $variables['errors'] = \Drupal::formBuilder()->getError($element);

    @@ -2760,8 +2769,17 @@ function theme_form_element($variables) {
    +    $attributes['class'][] = 'form-error';

    You can check $element['#errors'] now. Please see _form_set_attributes and consider merging these checks

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1829,4 +1851,82 @@ public function setRequest(Request $request) {
    +  protected function displayErrors($form) {

    I still don't think this is the responsibility of the FormBuilder.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new14.49 KB
FAILED: [[SimpleTest]]: [MySQL] 57,218 pass(es), 350 fail(s), and 376,465 exception(s).
[ View ]

Here's a re-roll with _form_set_attributes() - I can't comment on the Form Builder. I just want the desired output to work so am open to suggestions.

tim.plunkett’s picture

  1. +++ b/core/includes/form.inc
    @@ -2736,8 +2742,11 @@ function theme_form_element($variables) {
    +  $variables['errors'] = \Drupal::formBuilder()->getError($element);

    This line is still going to cause hundreds of errors and tens of thousands of exceptions.

  2. +++ b/core/includes/form.inc
    @@ -2749,19 +2758,29 @@ function theme_form_element($variables) {
    +  if (!empty($variables['errors'])) {
    +    _form_set_attributes($element, array('form-error'));

    You completely misunderstood me. I meant to look at the internals of _form_set_attributes, it already does a check for errors!

mgifford’s picture

damn.. I definitely misunderstood. Scratch the patch in #119.

The big attributes added are 'form-error' & 'form-error-message' in this patch. I'm not sure how to integrate this in _form_set_attributes()'s:

if (isset($element['#parents']) && isset($element['#errors']) && !empty($element['#validated'])) {
    $element['#attributes']['class'][] = 'error';
    $element['#attributes']['aria-invalid'] = 'true';
  }

I don't think you're suggesting that we integrate with theme_form_error_message().

I was hoping to be able to nudge this ahead, but can't take this any further.

How do we address this in FormBuilder? Who can take this on?

anandps’s picture

Background

While this issue is getting some attention, I just wanted to mention something a matter of concern.

When a form generates an error after submission and the page reloads, I've found when testing with JAWS that there's absolutely no indication to a screen-reader user that something went wrong.

I may have been missing something and my tests were a long while ago but... I feel that if this is still the case and at least one or two other people could verify this, then I'd like to propose one addition to the work performed on this issue:

Proposal

It would be wonderful if we could get the Form system to hook into the theme system to dynamically add an error message in the <title> field or at the start of the body. Doing so would ensure that screen-reader users would be informed immediately on page load that the form they just submitted returned errors.

A different way to handle this would be use Drupal.behaviors() to alert the user after page load is complete. I'm thinking about the fact that the JavaScript alert function (e.g. alert('The submitted form has errors.'); ) is pretty much as noisy as it gets. It will ensure the user has been notified and acknowledges the problem by having to click a button like, "Ok" or something.

The same behaviour will help on mobile devices.

One more thing to do would be to set focus on the offending form. All forms in Drupal have an id attribute. If we use JavaScript to increase the friendliness of the system, then we could very well place focus on the form right after the proposed alert('...'); message is dismissed.

Thoughts?

PS: One of the links on the original design-discussion for this solution made me think of using JavaScript's alert('...');. I'd originally thought that we might place the markup of the message at the very start of the <body> tag.

mgifford’s picture

Take a look at Drupal.announce() that @jessebeach was able to bring into Core -
https://drupal.org/node/1973218

She & Wim presented in Portland - https://portland2013.drupal.org/node/2158

I don't think that putting the error in the title will be useful, although ARIA can certainly be useful for some screen readers.

There are definitely other examples that can be emulated such as https://github.com/wet-boew/wet-boew/wiki/Form-validation

But, a huge part of the problem is establishing the semantic relationship between the error and the form element.

I think that the existing patch goes a long way to addressing known accessibility problems with the error messages. It needs to be tested with JAWS & VoiceOver of course, but it needs to pass the unit tests too.

I would like to see if we can fix the known problems first and extend it with testing. This has been an issue that has been discussed for a long while and I know we don't need to go back to the drawing board.

anandps’s picture

Hey there @mgifford! :o)

mparker17 and I have been in touch and hope to work on this in the next few days. Thanks for your comment above! I do hope to get the alert/notice in at the same time as it's definitely related. I'll let you know how things go if we're the first ones to make further progress on this.

Thanks!

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new12.94 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new2.31 KB

A re-roll, but the in FormBuilderTest.php 3/4 of the fragments aren't applying any longer.

mgifford’s picture

I guess this function got removed from FormTestBase.php:
PHP Fatal error: Call to a member function drupalStaticReset() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Core/Form/FormTestBase.php on line 138

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

I'm working on this.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new46.96 KB
new17.03 KB
new10.38 KB
FAILED: [[SimpleTest]]: [MySQL] 60,012 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This is more what I had in mind. The interdiff is bigger than the patch itself, but I attached it anyway.

This includes unit tests for the new FormElementHelper, but not web tests yet for the actual links and error divs.

The FormBuilder doesn't have to care that this is happening.

Here's an example of trying to make a view with a duplicate name:

tim.plunkett’s picture

StatusFileSize
new11.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,817 pass(es), 24 fail(s), and 1 exception(s).
[ View ]
new2.07 KB
new41.72 KB

Whoops, looking at that screenshot I noticed that I was still displaying the messages at the top.
Fixed!

Updated screenshot:

tim.plunkett’s picture

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new12.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-inline-error-1493324-137.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.92 KB

Using setFormError() and friends from a submit handler is effectively the same as just drupal_set_message($message, 'error').
Because it does not interrupt validation, it is not even really an error, just a message.
For now, I put in a workaround to allow it to still show the error message, but we might want to consider changing all of those to d_s_m directly.

tim.plunkett’s picture

file_save_upload makes this extra tricky because it is sometimes called from validate and sometimes called from submit...

mgifford’s picture

mgifford’s picture

StatusFileSize
new8.09 KB
FAILED: [[SimpleTest]]: [MySQL] 63,099 pass(es), 633 fail(s), and 126 exception(s).
[ View ]

re-roll... because of Hunk #7 FAILED at 2803.
1 out of 8 hunks FAILED -- saving rejects to file core/includes/form.inc.rej

tim.plunkett’s picture

I'm not sure how we went from 12.49K to 8K, did you drop something?

mgifford’s picture

I certainly didn't try to drop anything. There was just that one fail when I applied the patch manually and then worked through the failed chunk.

tim.plunkett’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Oh, this conflicted with #2152213: Convert theme_form_element() to Twig, and the patch keeps using $attributes while HEAD now uses $variables['attributes']. So this needs a real reroll.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new8.1 KB
FAILED: [[SimpleTest]]: [MySQL] 63,078 pass(es), 633 fail(s), and 126 exception(s).
[ View ]

Ok. I think there was only one '$attributes'.

tim.plunkett’s picture

The patch is missing the entire FormElementHelperTest, seen at the bottom of the patch in #137.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new10.51 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Thanks @tim.plunkett - with the all important tests this time!

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new10.51 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Forgot about _theme()

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Required Fields, -#d8uix, -Needs reroll
StatusFileSize
new2.77 KB
new12.11 KB
FAILED: [[SimpleTest]]: [MySQL] 64,548 pass(es), 96 fail(s), and 0 exception(s).
[ View ]

You also left out the FormElementHelper class, which was the major change in this issue...

And the form errors weren't even printing anymore, because $output is no longer used.
I started to update it, but after checking with joelpittet on IRC, he encouraged me to inline this directly in form-element.html.twig

We still need a web test with some xpath to assert the errors are all shown in the right place.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new12.11 KB
FAILED: [[SimpleTest]]: [MySQL] 64,627 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
new523 bytes

Still needs tests, I don't have the time to work on them, but just an xpath showing that there is the top region as well as the inline error would be enough.

The errors are already passed through t(), so we can't double-escape them.

mgifford’s picture

Thanks @tim.plunkett for continuing to push this important issue further. I really don't know where things went so very wrong with my earlier patches. I really don't think I was doing anything all that unusual. Git didn't apply but usually this helps override that and identify where there is a conflict:

patch -p0 < form-inline-error-1493324-137.patch

Anyways, there is progress!

tim.plunkett’s picture

git apply --3way should work, or if not, git apply --reject.
Or the issue was that you did not git add the new files before diffing.

Anyway, the last fails are all for elements using #error_use_parent: radios and checkboxes.
Whoever authored the original patch, what exactly was the mechanism to make them show up in their parent or elsewhere?

mgifford’s picture

I'll have to play with git apply --3way & git apply --reject

I think my main problem though was not git adding the new files. Thanks!

There's probably a good chance that #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes broke this as it deals with radios & checkboxes.

bleen18’s picture

Whoever authored the original patch, what exactly was the mechanism to make them show up in their parent or elsewhere?

At the time, the errors were showing up for both the parent and the individual checkbox/radio. The #error_use_parent property simply prevented the error from displaying on the individual checkbox/radio. Clearly a lot has changed since I worked on this patch though.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new12.5 KB
FAILED: [[SimpleTest]]: [MySQL] 64,602 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
new556 bytes

That indeed broke it, thanks for the pointer!
It incorrectly removed checkboxes and radios from the elements that are considered "form elements", and now they are the only input elements that aren't run through form-element.html.twig.

sun’s picture

Status:Needs review» Needs work

The compound elements of #type radios/checkboxes are no longer wrapped in a form element, but in a fieldset instead. A fieldset is a form element wrapper already; i.e., a fieldset is not wrapped into an outer form element.

Those two #types are not really form elements; they are simply expanded into multiple sub-elements, but technically they do not present an own form element - each sub-element is processed on its own.

Markup-wise, it wouldn't make sense to output a <div class="form-wrapper"><fieldset>...</fieldset></div> - especially because the form element has its own logic for outputting a label and stuff that the fieldset has on its own already.

Due to that, the patch in #164 duplicates the #title of the element into (1) the fieldset legend and (2) the wrapping form element label.

There's quite some duplication of logic going on between theme_fieldset and theme|template_preprocess_form_element in the meantime, and it would probably make sense to move all of that into a shared helper function.

It might be best to convert theme_fieldset into twig first, so that both form wrapper functions are using preprocess functions, and once that is done, we can easily move the shared logic into a new preprocess function that is shared between both.

#2152209: Convert theme_fieldset() to Twig

tim.plunkett’s picture

If I still had this assigned to me, I would unassign it.

I fundamentally disagree with the changes made to remove checkboxes and radios from the form element flow, and I don't have the time or patience to wait on slowly reversing that mistake in another issue.

tim.plunkett’s picture

Status:Needs work» Postponed

Postponed on the revert of the issue that causes the regression in checkboxes and radios.

Shyamala’s picture

Issue tags:+D.o UX

Adding tags

andypost’s picture

Status:Postponed» Needs work

is there a reason to postpone?

YesCT’s picture

@andypost I think @tim.plunkett was referring to comment #20 in #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes. But that has not been reverted.

@sun in #165 says this might be easier after #2152209: Convert theme_fieldset() to Twig and that is fixed and in.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new12.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,332 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Reroll, we'll see if this works. If not, then fieldsets are still wrong.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new12.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,565 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

Just a re-roll. use Drupal\Core\Render\Element; is now already defined.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Working on this again.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new26.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,632 pass(es), 454 fail(s), and 82 exception(s).
[ View ]
new17.14 KB

Okay, going back to the approach of not using form errors during submission, since they *don't do anything* outside of calling d_s_m in HEAD/D7.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new26.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,621 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new505 bytes

Well, that was a lot of fails.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new2.64 KB
new27.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,616 pass(es).
[ View ]
mgifford’s picture

That's great, the bot's like it. Thanks Tim.

Now, some minor CSS issues I noticed. I think this looks better. Screenshots attached.

+++ b/core/modules/system/css/system.theme.css
@@ -48,7 +48,7 @@
   background-color: #fef5f1;
   border: 1px solid #ed541d;
   color: #8c2e0b;
-  padding: 10px;
+  padding: 5px;
}
.form-error-message {
   margin-bottom: 10px;
@@ -602,6 +602,7 @@
   background-image: url(../../../misc/icons/ea2800/error.svg);
   border-color: #f9c9bf #f9c9bf #f9c9bf transparent;  /* LTR */
   box-shadow: -8px 0 0 #e62600; /* LTR */
+  margin-left: 8px;
}
tim.plunkett’s picture

Issue tags:-Needs tests
StatusFileSize
new32.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,388 pass(es), 171 fail(s), and 0 exception(s).
[ View ]
new32.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,649 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
new32.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,626 pass(es).
[ View ]
new4.34 KB

Okay, I've added a test.
Uploading just the test in isolation is obviously going to fail, since the markup is changing.
Instead I've uploaded two failing patches, one with the per-element error handling suppressed, and one with the form count-the-errors handling suppressed.

  protected function finalizeValidation($form_id, &$form, &$form_state) {
    // After validation, loop through and assign each element its errors.
// COMMENTED OUT IN INDIVIDUAL
    $this->setElementErrorsFromFormState($form, $form_state);
    // Store all of the errors for this form at the top level.
// COMMENTED OUT IN JUMPLINKS
    $form['#errors'] = $this->getErrors($form_state);
tim.plunkett’s picture

Issue summary:View changes
Issue tags:+API change

Updated the issue summary with the API change.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

Alright, now just for some reviews.

Bojhan’s picture

I think we don't need the outer border on the error state. Since we already have a red border on the form itself, having two borders is unnecessary signaling not needed for a11y and also more bloat from a visual design perspective.

@mgifford could you comment on if this is going in the right direction, because we dont have errors inline nor something different than color? or am I missing something?

mgifford’s picture

Bojhan, just as an example, some comparison images. First with the patch:
Errors with patch

Next without the external border:
image size without the patch

Taken from:
/admin/structure/types/manage/article/fields/node.article.field_image

Bojhan’s picture

StatusFileSize
new41.49 KB
new76.04 KB
new28.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,213 pass(es), 688 fail(s), and 131 exception(s).
[ View ]

Here you go, I think this will work. Its now more inline with the other Seven styling.

tim.plunkett’s picture

I had a working 32K patch in #183. You removed 4K and now it is broken, but provided no interdiff or even summary of code changes.
Please don't do that.

tim.plunkett’s picture

Status:Needs work» Postponed
Issue tags:-API change
StatusFileSize
new344 bytes
new18.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,677 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Turns out you just left out the new files.
However, because we're now making an API change here, and its being held up on visual changes, I'm splitting the validate/submit changes out to a new issue.

This is now blocked on that.

Attached is the interdiff of changes made by @Bojhan. The patch will not work until #2239299: Form errors should only be set during validation goes in.

Bojhan’s picture

Status:Postponed» Active

@timplunkett My mistake - I tried to get it right. I only changed 3 lines though. Lets keep this on active while we make sure that the visual changes pass a11y review. Then we get put it to postponed when its ready to go in but waiting on #2239299: Form errors should only be set during validation.

mgifford’s picture

The patch is definitely coming along. The errors are inline now, but there should be a short summary at the top like there was. Ideally there would be a summary of the messages with a link to take you down to the error. I had seen those earlier but haven't been able to replicate them with the latest patch.

It showed up here in an earlier patch by adding menus, recipients & terms:
https://drupal.org/files/issues/Error-Add-Menu-Link-With-Less-Padding.png
https://drupal.org/files/issues/Error-AddTerm.png
https://drupal.org/files/issues/Error-Recipients-10px-Padding.png

I can easily create errors here:
admin/structure/types/manage/article/fields/node.article.field_image/field
admin/structure/types/manage/article/fields/node.article.field_image
admin/structure/types/manage/article/fields/node.article.body (uploading an image)
admin/config/system/site-information (spaces)
user/password (nonsense)
admin/structure/contact/manage/feedback
admin/structure/types/manage/article/form-display (edit body)

I think it would be clearer if it was prefixed with:
"Error: Maximum width must be a number."
"Error: No file selected."
"Error: The path ' ' is either invalid or you do not have access to it."
"Error: The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space."
"Error: Sorry, nonsense is not recognized as a username or an e-mail address."
"Error: Recipients field is required."

I also think the formatting of the text is way too similar to the Label.

Maybe the label should be black if it is separate from the error message.

I'm trying to find instances to test template_preprocess_form() but haven't been able to identify them. In #165 @sun talked about "mov[ing] all of that into a shared helper function."

I'm looking forward to seeing:
drupal_set_message(format_plural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links), 'error');

expressed.

tim.plunkett’s picture

#193, this doesn't work yet. I said that in #191 when I marked it postponed.
I don't see how this can be active while it is non-functioning, but that's what @Bojhan wants.

mgifford’s picture

Right. I wasn't sure exactly how it wasn't functioning. However if we apply both patches, it's much easier to evaluate.

admin/config/system/site-information
error with two patches for site information.

admin/structure/types/manage/article/form-display
error with two patches for form display.

admin/structure/types/manage/article/fields/node.article.body
error with two patches for adding an image.

user/password
error with two patches for user password reset.

So we just have to focus a bit more testing on #2239299: Form errors should only be set during validation for now I assume.

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new18.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch inline-errors-1493324-196.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled

mgifford’s picture

StatusFileSize
new121.03 KB

Ok, just about to RTBC this. Two things I'd like some clarification on.

First, do we want to have proper punctuation?

Error message: 3 errors have been found: Default front page, Default 403 (access denied) page, Default 404 (not found) page

vs (with the "and" & "."):

Error message: 3 errors have been found: Default front page, Default 403 (access denied) page, and Default 404 (not found) page.

Finally, is there anything that can be done about the cases where there is no link provided in the header error? For instance there is no link if you enter a value that isn't a number here:
admin/structure/types/manage/article/fields/node.article.field_image/field

"Limit must be a number." is simply repeated in the header & then along with the form element.

Similar issues:
admin/structure/contact/manage/personal/fields
admin/structure/comments/manage/node__comment/fields
admin/appearance/settings/bartik

This is probably bigger than what can or should be addressed in this issue, but would like to address it.

Cottser’s picture

Maybe this is naïve (I'm not fully up to date with this issue but I don't think it's been mentioned) but why not display the multiple errors w/ links as a <ul>/item_list? Then we don't need to worry so much about grammar which has different rules for different languages.

mgifford’s picture

@Cottser - I like that idea, but think we should spin it off on another issue.

I took a brief look and could find the same problem in:
core/modules/views_ui/views_ui.theme.inc:
$variables['displays'] = empty($variables['displays']) ? t('None') : format_plural(count($variables['displays']), 'Display', 'Displays') . ': ' . '<em>' . implode(', ', $variables['displays']) . '</em>';

core/lib/Drupal/Core/Extension/InfoParser.php:
$message = format_plural(count($missing_keys), 'Missing required key (!missing_keys) in !file.', 'Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));

core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php:
$top_message = format_plural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links);

core/modules/system/system.install:
'value' => format_plural(count($modules), 'The %modules module is disabled.', 'The following modules are disabled: %modules', array('%modules' =>implode(', ', $modules))),

core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/filter/TaxonomyIndexTid.php:
form_error($form, $form_state, format_plural(count($missing), 'Unable to find term: @terms', 'Unable to find terms: @terms', array('@terms' => implode(', ', array_keys($missing)))));

core/modules/user/lib/Drupal/user/Plugin/views/filter/Name.php:
form_error($form, $form_state, format_plural(count($missing), 'Unable to find user: @users', 'Unable to find users: @users', array('@users' => implode(', ', array_keys($missing)))));

I think having a consistent pattern is probably more important. I'll happily start a new issue for this. EDIT: Added #2242631: Plural lists should use HTML lists rather than simply implode(', ', $var)

The remaining issue I see is probably outside of scope too. I just wanted to raise it before marking it RTBC.

andypost’s picture

Overall looks great!
This needs follow-up issue for "Limit must be a number." and then rtbc

PS: Is it possible to re-use joy-ride (tour) to navigate between errors? That could be done in contrib a-la beautytips to display full error message...

  1. +++ b/core/includes/form.inc
    @@ -1151,6 +1152,7 @@ function form_process_password_confirm($element) {
    +    '#error_use_parent' => TRUE,

    @@ -1158,6 +1160,7 @@ function form_process_password_confirm($element) {
    +    '#error_use_parent' => TRUE,

    @@ -1261,6 +1264,8 @@ function form_process_radios($element) {
    +        '#error_use_parent' => TRUE,

    @@ -1415,6 +1421,8 @@ function form_process_checkboxes($element) {
    +        // Errors should only be shown on the parent checkboxes element.
    +        '#error_use_parent' => TRUE,

    @@ -2883,6 +2910,14 @@ function template_preprocess_form_element(&$variables) {
    +  if (!empty($element['#errors']) && empty($element['#error_use_parent'])) {

    Suppose this needs change notice

  2. +++ b/core/modules/system/templates/form-element.html.twig
    @@ -37,6 +38,11 @@
    +  {% if errors %}
    +    <div class="form-error-message">
    +      <strong>{{ errors }}</strong>

    maybe better to get rid of strong tag here and use css to style?

mgifford’s picture

196: inline-errors-1493324-196.patch queued for re-testing.

mgifford’s picture

StatusFileSize
new18.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,607 pass(es).
[ View ]

Just another reroll.

mgifford’s picture

@tim.plunkett - Thanks.

@andypost That would be really cool to "re-use joy-ride (tour) to navigate between errors"!

I wrote up a follow-up issue #2250041: No link provided in the header error to the "Limit must be a number." problem.

1) I'm not sure if I'm going to have the chance write up the Change record for a bit, so hoping someone else takes it on.

2) It is more semantic the way it is now and would be more useful for assistive technology than just styled CSS.

So do we need to wait for the Change record before marking this RTBC?

mgifford’s picture

StatusFileSize
new18.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch inline-errors-1493324-207.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Just a re-roll.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new18.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,024 pass(es).
[ View ]
mgifford’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll
YesCT’s picture

I dont think this needs a change record. (I guess a committer will correct us if it does.) @andypost though it might in #200 1. but that doesn't seem like change record kind of change...

I'm going to reroll this now.

YesCT’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.27 KB
new18.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,676 pass(es).
[ View ]

conflicts were from #2209977: Move form validation logic out of FormBuilder into a new class

I did just a straight reroll, redid the same changes where the code had moved to, but had phpunit seg fault.
@tim.plunkett helped me find that issue added tests which used drupalSetMessage(), which this issue removes.

The interdiff is the changes tim found compared to my seg faulting reroll.

[edit]
upgrading my version of php got rid of the seg fault. maybe related to #2246775: Suggest 5.4.11 as minimum PHP version for Windows and MacOS if XCache is enabled

mgifford’s picture

Status:Needs review» Reviewed & tested by the community

As per #200, now setting this to RTBC.

Follow-up issue [##2250041]

in #211 YesCT suggested a change record isn't needed.

I argued to keep STRONG in #205.

So hopefully this is good to go!

Thanks @YesCT & @tim.plunkett for the recent work on the re-roll.

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community

Random fail from when HEAD was broken (Drupal\simpletest\Tests\InstallationProfileModuleTestsTest)

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Retest was green.

mrjmd’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Needs work
StatusFileSize
new36.48 KB
new41.72 KB
new313.35 KB
new335.34 KB

I did a round of testing on this and discussed it with YesCT, marking it back to Needs Work because of duplicate labels showing up:

Duplicate Labels

When I first applied the patch from #212 to a fresh clone of HEAD, I didn't notice any change in behavior at all:

Patched Error Message

I tried a few different things, and finally was able to reproduce the expected behavior:

Patched Working Error Message

In the screenshots above I was using Firefox. In Firefox and Chrome, unless I filled in all required fields before submitting the form, I was getting behavior like the first image above. If I filled in the required fields but caused some other validation error, I would get the expected behavior.

In Safari this seems to work as expected, BUT there seems to be an issue of duplicate labels appearing.

bleen18’s picture

In the screenshots above I was using Firefox. In Firefox and Chrome, unless I filled in all required fields before submitting the form, I was getting behavior like the first image above. If I filled in the required fields but caused some other validation error, I would get the expected behavior.

This is due to browser-based HTML5 validation. You can turn this off for testing purposes: http://novalidate.com/

The double-label issue is still a thing (I'm assuming, I have not confirmed)

mgifford’s picture

I just confirmed the double label (and description actually).

It occurs in every instance where radios & checkboxes are from what I can see. Doesn't require an error message either.

admin/config/development/logging

/admin/structure/block (block configuration modal)

admin/structure/types/manage/article/fields/node.article.comment

/admin/structure/comments/manage/node__comment/fields/comment.node__comment.comment_body

It doesn't happen without the patch.

tim.plunkett’s picture

Can you test it without the patch? Is this a new regression in HEAD itself? I don't know where this patch could be causing that.

YesCT’s picture

re #224
there is a before screenshot of head in the issue summary in the UI changes section, check near "Screenshot before applying #212 patch"

with the patch, there is both:
missing label on fields with the inline errors
and
duplicate labels elsewhere

the missing labels can be seen on earlier screenshots, for example in #197, but.. that was not embedded in the comment, so not as many people probably saw it.

[EDIT: oh! the labels are not missing, it's just that the errors are not after/inline with the label as in the proposal screenshot in the issue summary (which is from comment #1).]

for clarity:
head without patch
Before #212

with patch from #212
Duplicate Labels

mgifford’s picture

StatusFileSize
new66.43 KB
mgifford’s picture

I tried it again with git reset --hard, git pull and rm'ing the two added files in the patch. Still got the same results.

This is another screenshot after clearing the caches several times.

Only local images are allowed.

mrjmd’s picture

Poked around some more, it appears to be this line in the patch that's been added to core/includes/form.inc that is causing the issue:

$element['#theme_wrappers'][] = 'form_element';

Manually removing it got rid of double titles and descriptions for me. Is this code needed for other reasons? Should I reroll a patch without it?

mgifford’s picture

@mrjmd that fixes it for me! I had to look back at that code to see how long it was in this patch.. Looks like it was introduced in #164. Let me go re-roll it without that.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new18.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,766 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
new461 bytes

Here's the reroll & interdiff.

tim.plunkett’s picture

I didn't add that for fun. It was to work around the breakage sun caused in his other issue.

See #165.

I have no idea how to proceed here, he gutted radios/checkboxes too much.

mgifford’s picture

I didn't suggest that you did it for fun or out negligence. All I did is work to quickly identify where it was introduced. Nobody questions that you know Core well, care about Drupal and write anything but solid patches.

I don't know who gutted the radios/checkboxes. I'm assuming it's @sun from your link.

He suggests to "There's quite some duplication of logic going on between theme_fieldset and theme|template_preprocess_form_element in the meantime, and it would probably make sense to move all of that into a shared helper function.", but don't think there's a new issue for that.

In my reading of #165 there's no suggestion of how to deal with errors, just a clear and logical argument for why a wrapper isn't needed. This sadly seems to leave us with no means to provide inline errors for radios & checkboxes.

Maybe we can separate radios/checkboxes from this issue. Commit this issue with the vast majority of input types. It's not ideal, but it will be serious progress.

YesCT’s picture

Well, we cant have the double field Iabels,
and I think we should have inline errors for all elements.
(I do not think we should put radios and checkboxes in in a separate issue.)

Seems like this issue is blocked on fixing something. I'm not sure what that something is, but I took a guess. The problem should be clarified by someone who understands this better... #2275373: Add function for shared logic between fieldset and form element Marking postponed on that.

webchick’s picture

Agreed... while I'm certainly sensitive to all of the work that's gone into this, and the later patches are looking great, we really can't commit a partial fix here, IMO. "Doubling up" checkboxes/radios labels *or* having randomly (to users) inconsistent error handling on forms both leave Drupal 8 in a non-shippable state. We can't horse-trade a major bug (even a 2+ year old one) for a critical one, IMO.

I'm not necessarily averse to revisiting old decisions on radio/checkbox markup to unblock this, but I'm confused by the cryptic comment at #232. I believe it's in reference to #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes but the issue it was requested to postpone that one on is #2152209: Convert theme_fieldset() to Twig which has been in 8.x for months.

tim.plunkett’s picture

In #2192419-18: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes I asked for that issue to be rolled back, as it caused this current problem

After that, it was identified as having caused #2236789: Styling of inline radios broken: inappropriate trailing colons (breaks EditorImageDialog), itself a major bug.

It was never explained how #2152209: Convert theme_fieldset() to Twig would help alleviate either of these problems, and it clearly hasn't.

Basically, the radios/checkbox issue made those elements NOT form elements, and instead manually prepends the label and description. This issue tried to turn them back into form elements, since the error handling is for *form elements*.
But now they are getting the standard label handling from being a form element, as well as manually handling for the labels.

I don't know how to fix this without reverting that other issue.

Reverting that would also immediately fix the double colon bug.

webchick’s picture

That issue doesn't cleanly roll back right now, unfortunately (and not just because of PSR-4). But in reading the patch in #9 in that issue, I don't see where radios/checkboxes are special cased. Tried grepping for "radios" "checkboxes" "options" etc.

tim.plunkett’s picture

The changes are in form_pre_render_conditional_form_element, which is assigned to radios and checkboxes in system_element_info()

+++ b/core/includes/form.inc
@@ -1259,7 +1278,10 @@ function form_pre_render_conditional_form_element($element) {
   if (isset($element['#title']) || isset($element['#description'])) {
-    $element['#theme_wrappers'][] = 'form_element';
+    // @see #type 'fieldgroup'
+    $element['#theme_wrappers'][] = 'fieldset';

Then, in fieldset, handling was added for the title:

+++ b/core/includes/form.inc
@@ -951,30 +951,49 @@ function theme_fieldset($variables) {
-  if (!empty($element['#title'])) {
+
+  if ((isset($element['#title']) && $element['#title'] !== '') || !empty($element['#required'])) {
     // Always wrap fieldset legends in a SPAN for CSS positioning.
-    $output .= '<legend' . new Attribute($legend_attributes) . '><span class="fieldset-legend">' . $element['#title'] . '</span></legend>';
+    $output .= '<legend' . new Attribute($legend_attributes) . '><span class="fieldset-legend">';
+    $output .= t('!title!required', array('!title' => $element['#title'], '!required' => $required));
+    $output .= '</span></legend>';

So radios and checkboxes are no longer form elements, they are just wrapped by a fieldgroup with explicit title/description handling.

This issue tried to make them form elements again, but can't revert the fieldgroup changes.

mgifford’s picture

Is there no way around this? It's really discouraging that we can't seem to find a solution to this.

Seems we're at an impasse here between @tim.plunkett & @sun

It's postponed based on #2275373: Add function for shared logic between fieldset and form element but there's been no progress on this.

@webchick - do you have any insights here? Is there any other way to address this? This is a long issue, and this thread is the summary of a long issue.

This is actually an important accessibility problem that it seemed we were very close to fixing.

tim.plunkett’s picture

It depends, do you consider checkboxes and radios to be form elements?
There are two ways around this.

Either we pretend checkboxes and radios are NOT form elements, and add a workaround to get this in.

Or @sun can undo the API changes he made to the checkboxes and radios elements, and this will Just Work™.

tim.plunkett’s picture

Status:Postponed» Needs review
Issue tags:+Needs tests
StatusFileSize
new20.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Form/ValidationTest.php.
[ View ]

Reroll of #212.

I guess we'd also need tests to catch any double labels?

rooby’s picture

Either we pretend checkboxes and radios are NOT form elements, and add a workaround to get this in.

Although I'm not fully across the related issues in this situation, technically speaking they are not form elements.

Checkbox and radio are but checkboxes and radios are not.

It's only from the end user's point of view that a group of individual checkbox/radio elements is a single field.

tim.plunkett’s picture

So, they should also not receive form errors?

joelpittet’s picture

Checkboxes and radios (the group) need form validation errors.

Pretty Example: http://bootstrapvalidator.com/examples/icheck/

tim.plunkett’s picture

StatusFileSize
new20.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 30,524 pass(es), 11,525 fail(s), and 1,792 exception(s).
[ View ]

That was a rhetorical question. Of course they need form validation errors.

It will be a real shame if this gets bumped to D9.

joelpittet’s picture

Rhetorically answered in support:P

If it's a rollback that's needed lets do that.

mgifford’s picture

Status:Needs review» Needs work
StatusFileSize
new58.11 KB

Radios & Checkboxes look good - http://sc72d4e211e4c3c3.s3.simplytest.me/user/1/edit?destination=admin/p...

No duplication like seen in #225

Putting spaces in the default front page gives me this error - http://sc72d4e211e4c3c3.s3.simplytest.me/admin/config/system/site-inform...
Fatal error: Call to undefined function l() in /home/sc72d4e211e4c3c3/www/core/includes/form.inc on line 481

Same error when requesting a new password:
http://sc72d4e211e4c3c3.s3.simplytest.me/user/password

May be unrelated, but looks like it's tied to $error_links[] = l($title, '', ['fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE]);

Works great here:
http://sc72d4e211e4c3c3.s3.simplytest.me/admin/structure/types/manage/ar...

Image uploads works well:
http://sc72d4e211e4c3c3.s3.simplytest.me/admin/structure/types/manage/ar...

tim.plunkett’s picture

Issue tags:+Needs reroll

Yes, l() has been removed. Should be \Drupal::l($title, Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE]));

BarisW’s picture

Status:Needs work» Needs review
StatusFileSize
new1.95 KB
new20.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,761 pass(es), 7 fail(s), and 1 exception(s).
[ View ]

Changed the l() calls.

mgifford’s picture

StatusFileSize
new20.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1493324-inline-form-errors-255.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Think the second one should be:

+      // Gather the element for checking the jump link section.
+      $error_links[] = \Drupal::l($message['title'], Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $message['key']), 'external' => TRUE]));
BarisW’s picture

Ah, sure. You are totally right. It has been a long day ;) Thanks!

mgifford’s picture

Been the most productive day in the d.o issue queue I can remember for a very, very long time!

rpayanm’s picture

Status:Needs work» Needs review
StatusFileSize
new20.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 49,368 pass(es), 16,749 fail(s), and 908 exception(s).
[ View ]

rerolling...

tim.plunkett’s picture

Issue tags:-Needs reroll
StatusFileSize
new20.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,058 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

This issue is still very upsetting to me. What a shame if this doesn't get resolved.

Forgot the interdiff, but it was just this:

diff --git a/core/modules/system/templates/form-element.html.twig b/core/modules/system/templates/form-element.html.twig
index 697e599..0bf4419 100644
--- a/core/modules/system/templates/form-element.html.twig
+++ b/core/modules/system/templates/form-element.html.twig
@@ -46,7 +46,6 @@
  * @ingroup themeable
  */
#}
-<<<<<<< HEAD
{%
   set classes = [
     'form-item',
rpayanm’s picture

Status:Needs work» Needs review
StatusFileSize
new426 bytes
new20.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,168 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

@tim.plunkett thank you, my bad :)

crasx’s picture

Issue tags:+SprintWeekend2015

When I tested this locally I found format_plural no longer exists. - https://www.drupal.org/node/2173787

I'm going to update the patch

crasx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.66 KB
new20.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,002 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Ran 270 locally and got a ton of fails (like 150) only running Drupal\system\Tests\Form\ValidationTest.
changed format_plural to \Drupal::translation()->formatPlural
With this patch I now get 8 fails only running Drupal\system\Tests\Form\ValidationTest

I will work on the 8 errors today

rootwork’s picture

crasx go ahead and assign it to yourself if you're actively working on it. (Just don't forget to unassign it when you're done so others know they can work on it too.)

crasx’s picture

Assigned:Unassigned» crasx

Thanks, will do

crasx’s picture

Assigned:crasx» Unassigned

I'm done for the day, here's some intermediate work to fix a couple of the fails. Will work more on this tomorrow

diff --git a/core/includes/form.inc b/core/includes/form.inc                                                                                                                                                                                                                  
index 18f1d10..b50c2c2 100644                                                                                                                                                                                                                                                 
--- a/core/includes/form.inc
+++ b/core/includes/form.inc
@@ -348,12 +348,14 @@ function template_preprocess_form(&$variables) {

   if (!empty($element['#errors'])) {
     $error_links = [];
+    $error_titles = [];
     // Loop through all form errors, and display a link for each error that
     // is associated with a visible form element.
     foreach ($element['#errors'] as $key => $error) {
       if (($form_element = FormElementHelper::getElementByName($key, $element)) && Element::isVisibleElement($form_element)) {
         $title = FormElementHelper::getElementTitle($form_element);
-        $error_links[] = \Drupal::l($title, Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE]));
+        $error_links['%'.$key] = \Drupal::l('%'.$key, Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE]));
+        $error_titles['%'.$key] = $title;
       }
       else {
         drupal_set_message($error, 'error');
@@ -361,7 +363,11 @@ function template_preprocess_form(&$variables) {
     }

     if (!empty($error_links)) {
-      drupal_set_message(\Drupal::translation()->formatPlural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links), 'error');
+      // We need to pass this through String::format() so drupal_set_message()
+      // doesn't encode the links.
+      $message =  \Drupal::translation()->formatPlural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links);
+      $message = String::format($message, $error_titles);
+      drupal_set_message($message, 'error');
     }
   }
}

crasx’s picture

StatusFileSize
new1.78 KB

Here is an updated version of the foo module for the latest drupal 8. You can access the test form at admin/foo

crasx’s picture

StatusFileSize
new23.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,113 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-273.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

Made some progress on this, but I don't think I did it right. I ended up wrapping the fieldset in a div. Will this introduce other issues? Is it not done the a11y way?

[edit]
oops, shouldn't have named it .diff

crasx’s picture

StatusFileSize
new24.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,120 pass(es).
[ View ]
new891 bytes

The fail in the last test comes from an issue in the Shortcuts module. For now I updated the failing test to check for the right error message, and created an issue #2409885: Switch shortcut set form has accessibility issues.

crasx’s picture

Issue summary:View changes
StatusFileSize
new50.79 KB

added screenshot

[edit]
to get these screenshots I had to use the inspector in chrome to remove the required field

crasx’s picture

StatusFileSize
new72.05 KB

This is why I put a div around the fieldset, compare radios field to checkboxes field.
The radios label has a line through it because it is a fieldset, and the error should be above the label for consistency

crasx’s picture

Issue summary:View changes
StatusFileSize
new88.71 KB
new89.37 KB

updating screenshots

crasx’s picture

StatusFileSize
new23.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,110 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.38 KB

Fix twig formatting

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new23.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,112 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.32 KB

adjusting the whitespace in the twig template for whitespace standards based (thanks davidhernandez for your help in irc)

tim.plunkett’s picture

+++ b/core/includes/form.inc
@@ -344,6 +345,31 @@ function template_preprocess_form(&$variables) {
+        $error_links['@'.$key] = \Drupal::l('@'.$key, Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE]));
+        $error_titles['@'.$key] = $title;

What's with the '@'.$key thing? I read the comments since my last patch and found no explanation.

If we need to keep this, please put spaces around concatenation.

davidhernandez’s picture

Status:Needs work» Needs review
StatusFileSize
new24.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,111 pass(es).
[ View ]
new1.8 KB

Adding back the fix for the shortcut test that was originally added in #283 and adding the spaces for #290.

It would be nice to find a way to get the message into the fieldset without using the wrapper div, but I didn't try it, so not passing judgement.

davidhernandez’s picture

Sorry, meant #291 not #290.

crasx’s picture

The @ in the first array is not nessecairy, I added it to make it clear that the index in error_titles is paired with the index in error_links. The @ is required in error_titles for String::format.

Thanks for fixing the spacing. I was thinking last night it may be possible to do it without the div. You would need to find a way to get the label within the fieldset to not appear at the top. I tried it this morning but couldn't figure out how.

davidhernandez’s picture

@crasx, what you are seeing is how the browser renders the legend/label of a fieldset by default. (with the line through it). I think to remove the wrapper div you'll have to absolutely position the legend to get it to go below the error text.

mgifford’s picture

Status:Needs review» Needs work
Issue tags:+Needs manual testing
StatusFileSize
new42.54 KB
new55.44 KB

I went through a bunch of the problems in #195. This definitely needs manual testing as it's an important.

No problems with the duplicate labels here admin/people/create

We are still using "Error message 3 errors have been found: Default front page, Default 403 (access denied) page, Default 404 (not found) page" rather than with proper punctuation "Error message 3 errors have been found: Default front page, Default 403 (access denied) page, and Default 404 (not found) page." (we're missing the "and" and "." when there are a series of errors - admin/config/system/site-information)

I'm getting duplicates here though with images:

Both when the filetype isn't approved or if the image doesn't meet the minimum file size.

Seems way closer than we have been in a long time though. Been a really great community effort.

SKAUGHT’s picture

i know managed_file has it's own ajax response for messages (normally triggered after adding a file to the field, and hitting 'upload'..this behaviour will probably need to be changed globally.

davidhernandez’s picture

Status:Needs work» Needs review
StatusFileSize
new23.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,252 pass(es).
[ View ]
new4.9 KB
new30.55 KB

I reworked the fieldset template to avoid the 'if's and wrapper. Also, moved the css class from preprocess to the templates.

Re: #297, I'm not getting the duplicate errors on an image. Maybe that needs specific conditions? I'm not sure how to address creating an English list, with 'and' in it, in a way that is translatable.

When I get multiple errors that are not for fieldsets I see them separated. That's not unusual, but if that is the case shouldn't error text be more specific? Does it look weird to anyone else? See attached image.

Bojhan’s picture

I am really not sure what to review here. If I look at #286, I am pretty worried. Over 10 months ago I already mentioned that the fieldset around everything is pretty erratic. Just the red form should be enough.

@David This does not look weird.

aspilicious’s picture

I agree with Bojhan that the giant red squares are not pretty at all...

aspilicious’s picture

But looking at the screenshots I think we need a wrapper to display the message. Can we improve the styling so that it matches the styleguide a bit more.

joelpittet’s picture

@david would format_plural do the trick if the count is the error list length?

sorry that is a d7 function but I'm on a phone I think it's \String::formatPlural() now if I remember right.

davidhernandez’s picture

@Bojhan, is the issue just the red box? I'm late to this issue, so I don't know consensus for adding it. Just skimming the issue, it seems to have been there for a while.

+++ b/core/includes/form.inc
@@ -344,6 +351,31 @@ function template_preprocess_form(&$variables) {
+      $message =  \Drupal::translation()->formatPlural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links);

@Joel, this is the line. It is already running through formatPlural. I believe it only supports two options, but we might need three.

1 error has been found: foo.
2 errors have been found: foo and bar.
# errors have been found: foo, bar, ..., and baz.

And since the error list is simply attached to the end, I'm not sure how to work in the 'and' so it gets translated. Translate it by itself? That seems like overkill. If anyone knows someplace else in core that prints lists like this I can copy it.

joelpittet’s picture

Hmmm, initial thoughts are:

<?php
$message
=  \Drupal::translation()->formatPlural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links);
?>

becomes

<?php
$error_count
= count($error_links);
$last_error_link = array_pop($error_links);
$message =  \Drupal::translation()->formatPlural($error_count,
 
'1 error has been found: !last_error_link',
 
'@count errors have been found: !starting_error_links',
  [
   
'!starting_error_links' => implode(', ', $error_links),
   
'!last_error_link' => $last_error_link,
  ]
);
?>
davidhernandez’s picture

Thanks, Joel. I think you left out the last bit, but I see where you're going. That seems workable, but might have to forgo the serial comma, which goes against the style guide. I'd have to play with it (unless someone else wants to!)

joelpittet’s picture

There is a style guide for words in drupal? I blame Oxford and U of Chicago for this! Strunk and White FTW

joelpittet’s picture

If you want that serial comma, this is a variation. http://stackoverflow.com/a/18476638/80281

Oh yeah I forgot the 'and' bit:)

<?php
$error_count
= count($error_links);
$last_error_link = array_pop($error_links);
$message =  \Drupal::translation()->formatPlural($error_count,
 
'1 error has been found: !last_error_link',
 
'@count errors have been found: !starting_error_links and !last_error_link',
  [
   
'!starting_error_links' => implode(', ', $error_links),
   
'!last_error_link' => $last_error_link,
  ]
);
?>

Keep in mind with those !vars that we have a PITA in core of recent that expects all passthrough variables to be marked safe. I kinda hope that gets reverted... #2399037-24: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument

tstoeckler’s picture

There's not really a standard for this. Elsewhere we just leave it at all commas. That doesn't mean we shouldn't pursue the current approach further but if we do we should get a sign off from @Gábor Hojtsy in terms of translatability.

davidhernandez’s picture

StatusFileSize
new24.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,598 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.84 KB

Added code for the message text. It is functioning for me under the three conditions I mentioned in #304 and includes the serial comma. I also fixed CSS mistakes I had in the previous patch. Not sure how that happened.

davidhernandez’s picture

Status:Needs work» Needs review
StatusFileSize
new24.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,724 pass(es).
[ View ]
new2.09 KB

The test was not expecting the new sentence structure. Also fixed a errant space.

YesCT’s picture

I think we should just leave it a comma separated list and not worry about the last ", and".

@tstoeckler can we link to an example in core that is just comma separated without the last and?

Seems like that would be a separate issue.

YesCT’s picture

@aspilicious #301 #302
I'm not sure if you are talking about the english ", and" style (https://www.drupal.org/style-guide/content#english), or the red error style.
If it is the red error style... We should add a link to the style guide you are referring to to the issue summary, can someone do that?

Gábor Hojtsy’s picture

Right, we don't do ", , and " magic elsewhere. If we need to do it here for some reason, then we can make something like the following happen: t('@errors and @last_error' ....); where you remove the last item from the list to display separately. Languages that do not have this construct can add a comma in place of ' and ' in this string and get the same. However that would require a high level of attention from the translator to figure out.

Why do we need this here even though we don't use it elsewhere?

Gábor Hojtsy’s picture

YesCT asked for examples of commas. A quick grep on code we use to tell admins when .info files are invalid or dependent modules or themes are missing:

core/lib/Drupal/Core/Extension/InfoParser.php:          $message = String::format('Missing required keys (!missing_keys) in !file.', array(!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
core/lib/Drupal/Core/Extension/ModuleInstaller.php:          '%modules' => implode(', ', $module_list),
core/lib/Drupal/Core/Extension/ModuleInstaller.php:          '%missing' => implode(', ', $missing_modules),
core/lib/Drupal/Core/Extension/ModuleInstaller.php:        $reason_message[] = implode(', ', $reason);
core/lib/Drupal/Core/Extension/ModuleInstaller.php:        '@reasons' => implode(', ', $reason_message),
core/lib/Drupal/Core/Extension/ThemeHandler.php:          '!themes' => implode(', ', $missing),
YesCT’s picture

Thanks! #2419461: Use ", and" before the last item in a list of things programatically created for UI facing text in case we need to further discuss (I dont think so) or document that particular thing.

Here,
@davidhernandez
can you undo the bit you added to make ", and" ?

davidhernandez’s picture

StatusFileSize
new23.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,879 pass(es).
[ View ]
new1.26 KB

Languages that do not have this construct...

I think this is the point to emphasize, which probably explains why we aren't doing this elsewhere in core.

I removed the aforementioned code. A reverted the previous commit from my local, so the test changes is gone, but isn't in the interdiff.

tstoeckler’s picture

StatusFileSize
new1.8 KB
new23.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,869 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new44.23 KB

I wasn't very happy with the additional String::format() for generating the drupal_set_message() so I revised that. I also improves translatability because it does away with the string concatenation.

I also found that the red error background for form elements does not quite match the red background for messages which looks slightly weird:
A screenshot of a form error on the profile edit page with the message red and the form red annotated to prove their difference.

Re @Bojhan in #300: Looking back to your #188 the current style looks fairly similar. Can you be more specific in what needs to be done here?

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new787 bytes
new23.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,969 pass(es).
[ View ]

Yay, for test coverage!

tim.plunkett’s picture

The changes in #319 and #321 resolve my last hesitation about this patch. Thanks @davidhernandez and @tstoeckler so much for working on this.

I think we still need tests for the double label bug from before?

SKAUGHT’s picture

#299 @davidhernandez -- '& lists' issue: https://www.drupal.org/node/2242631

mgifford’s picture

Issue summary:View changes
StatusFileSize
new63.63 KB

Really great progress on this issue over the last two weeks.

Would be good to have more manual testing done on this.

This isn't a big deal, but the password example in #319 the "Password" link is looking for the "edit-pass" anchor and not "edit-pass-pass1". Not sure if there is an easy way to deal with this type of form.

The pages here look good now admin/config/system/site-information

@tim.plunkett - Any chance we could move double label bug testing out to a follow-up issue?

Probably not as I just found another one here - admin/config/people/accounts/fields/user.user.user_picture?bundle=user

duplicate error

RavindraSingh’s picture

StatusFileSize
new151.3 KB

@mgifford , I didn't see this error on accounts/fields/user.user.user_picture?bundle=user page. When I upload the wrong extension file it throws the error on and on trying again incorrect file it just replace the previous error. Can you please pull the 8.0.x first then test it out.

Please see the screenshot.
Form inline errors
If you are still getting the same error. please detailed out the steps to reproduce with specific browser.

tim.plunkett’s picture

@tim.plunkett - Any chance we could move double label bug testing out to a follow-up issue?
I don't see why we should punt test coverage for a bug we've already discovered.

mgifford’s picture

Interesting... I repeated the bug.

  • Went to SimplyTest.me,
  • installed the system,
  • went to the Picture setting for user,
  • uploaded a .patch file
  • Got the duplicate warning "The specified file favicon404-174940-92.patch could not be uploaded. Only files with the following extensions are allowed: png gif jpg jpeg. "

Those links should work if you're viewing it in the next 24 minutes.

davidhernandez’s picture

Status:Needs review» Needs work

Just commenting to say I can confirm what mgifford is seeing. I'm only seeing it happen on the settings for an Image field (the default image part.) I haven't tracked it down yet. Not sure if it's ajax causing it.

davidhernandez’s picture

Status:Needs work» Needs review

I tracked this down to the prefix (the one with id containing ajax-wrapper) being added twice. It is being wrapped around the form item and the input. I also realized this is happening in head and isn't caused by this issue. Can someone confirm? If so, I'm fine with calling it out of scope and fixing it in another issue.

davidhernandez’s picture

StatusFileSize
new25.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,437 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new2.26 KB

Copying the changes to Classy theme now that those System templates have been copied over.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new25.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,452 pass(es).
[ View ]
new789 bytes

Fixing the phpunit coding standards.

Cottser’s picture

Always happy to see this moving :)

I can confirm what @davidhernandez mentioned in #329, the duplicate error reported in #324 happens on HEAD as well, so that should be a separate issue.

Below is the only thing I could spot out of place after a quick code review. The patch seems to make a lot of sense.

+++ b/core/includes/form.inc
@@ -344,6 +351,30 @@ function template_preprocess_form(&$variables) {
+      $message =  \Drupal::translation()->formatPlural(count($error_links), '1 error has been found: !errors', '@count errors have been found: !errors', [

Minor: extra space after =

I can also confirm @mgifford's report from #324 where in this error 2 errors have been found: Username, Password (scenario from the issue summary), "Password" links to #edit-pass, but that element doesn't exist.

davidhernandez’s picture

StatusFileSize
new26.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch inline_form_errors_for-1493324-335.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.64 KB

I removed the extra space. I added a hack to get the #edit-pass link to work. The problem is the password confirm field is a bit of an oddball. It's not a fieldset, or a true field group, but contains multiple inputs. We just need to get the id there. An alternative is to change the link to point to the first child, but that started looking more complicated.

mgifford’s picture

What happened to core/themes/classy/templates/system/fieldset.html.twig?

davidhernandez’s picture

Issue tags:+Needs reroll

Classy's subfolders were just reorganized. We neglected to add a change record to that issue, which I just added. The system folder is gone, and fieldset moved to the form folder.

#2349559: [meta] Discuss the organization of subfolders in Classy

njbarrett’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new26.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,024 pass(es).
[ View ]
new3.96 KB

Rerolled on 8.0.x

mgifford’s picture

I added a follow-up issue about the duplicate here #2448967: Duplicate labels in admin/people/create

Tested these pages:

/admin/config/people/accounts/fields/user.user.field_link
/admin/config/people/accounts/fields/user.user.user_picture
/user/1/edit
/admin/config/system/site-information
/admin/config/system/actions/configure/user_add_role_action.administrator
/admin/config/media/file-system
/admin/config/regional/translate
/admin/appearance/settings/bartik

Errors only where expected (other than the main page at the moment due to another issue).

EDIT: This is looking great!

davidhernandez’s picture

Errors only where expected

I'm misunderstanding that. Are you saying you only got the errors you expected?

mgifford’s picture

It worked properly. I was testing for errors, and they appeared as I expected. No problems thus far. Would still like more manual testing though.

mrjmd’s picture

I've done a pretty thorough review, overall this is working great! As far as the code itself, I have only one minor coding standards gripe as an extra blank line has been inserted here:

+++ b/core/modules/system/src/Tests/Form/ValidationTest.php
@@ -206,17 +207,34 @@ function testCustomRequiredError() {
+

As for a functional review, a couple of things I ran into (screenshots attached):

  • On the login form, if I miss my password, the inline errors look right, but I don't see the general error message at the top of the page until the next page load.
  • On the add user form, if I don't enter anything into the password fields, it shows the errors properly. But if I then click the anchor link in the error message at the top of the page, it does not highlight the fields the way it does with other fields (maybe this is not possible since it's two fields).
  • Also it's worth mentioning that its somewhat annoying how the anchor links on longer form pages actually jump you past the field that threw the error, I assume because of the way the admin bar at the top is overlayed.

The only one of these I'd call a real issue is the login form issue.

Screenshot of login error

Screenshot of login errorScreenshot of login error after logging in

Screenshot of login error after logging inScreenshot of user add, clicking password link doesn't highlight

Screenshot of user add, clicking password link doesn't highlight

mrjmd’s picture

Status:Needs review» Needs work
vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new26.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new3.68 KB

Looks like template_preprocess_form is getting called too far after the status message rendered as the form is inside a block which is inside a render region etc. We might need to set the error message more close to to form_state to get it rendered before message get rendered. Moving the drupal_set_message calculation to formState.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new27.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,039 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new921 bytes

Ignore #346 for now. We need to move the error code bit higher than preprocess, but @tim.plunkett advised to write the test that fails the patch in #340 as explained in #344. Here is a test.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Working on this now.

vijaycs85’s picture

@tim.plunkett: FYI, Spent couple of hours on the failure when moving the setFormErrors() to FormValidator (https://www.drupal.org/node/1970534#comment-9702939). The only problem is l() function not available in unit test (because of no container). If we can fix it, it would work? I don't see any other place to set drupal_set_message before any render start.

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new49.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,140 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new26.15 KB

IMO the answer is to add a new collaborator, not to complicate FormValidator.

Now if you want to completely change how errors are handled, you can override that without interfering with accidentally breaking validation.

This has a couple @todos for needed docs, but I wanted to post the patch first.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new50.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,157 pass(es).
[ View ]
new876 bytes
tim.plunkett’s picture

Issue tags:-form validation
StatusFileSize
new50.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,137 pass(es).
[ View ]
new1.98 KB

Okay, this needs manual testing, but I think this is in a good place.

mrjmd’s picture

I did another round of testing on this... it seems to be working great!

My second and third bullet points from #344 still exist but are likely not in scope of this issue.

I will leave it in "Needs Review" to get more eyes on it, but I could not find any reason not to RTBC.

davidhernandez’s picture

The third item in #344 I agree sounds like the toolbar. The second one though we should probably try to fix. It sounds like what I was trying to fix in #335. @mrjmd can you check what it produces for the anchor link, and see if the password wrapper is getting the corresponding id added to it?

vijaycs85’s picture

Thanks @tim.plunkett. Just grabbed all @FormElement to see how they look with error. Except those never get any error (like, of type value, token, hidden), Most of them looking good. Looks like datelist and datetime need some UX love.
Just created a dummy github issue to attach screenshots instead of hammering this issue with another 10 images. https://github.com/vijaycs85/styleguide/issues/1

vijaycs85’s picture

#357: #344.2 - yeah, it's trying to link #edit-pass which is a wrapper of the password fields #edit-pass-1 and #edit-pass-2.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

How does datelist/datetime look in HEAD? That seems like a valid follow-up.

vijaycs85’s picture

StatusFileSize
new78.64 KB

It looks fine in HEAD as there is no inline error. Added a follow up #2450703: Fix inline form error styling issues for datelist and datetime elements

davidhernandez’s picture

Issue summary:View changes
Status:Needs review» Needs work
StatusFileSize
new112.35 KB

The #edit-pass anchor link is working for me.

One thing I noticed, if I enter a bad 'current password' but a good 'password' and 'confirm password' both report errors. I don/t know if it should work that way. If it should, we should make sure the password grouping is working right. The two fields each get an 'error' css class added, but it does not get an error message and the wrapper does not get styled.

vijaycs85’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new160.59 KB

This is the current behaviour as well. When try to change password with invalid current password, the password & confirm password fields are set as 'error' without any message. I guess, this is to tell the user that they need to re-enter the new password again.

So this is a generic case for inline message. If a field is set to be error without any message, it will get listed in the top message, gets error class, but no inline error.

vijaycs85’s picture

Issue summary:View changes
davidhernandez’s picture

The current functionality though doesn't indicate two errors, as this patch does. There is only the one message. I think it is odd/confusing to show the password fields as erred, but without a message or additional highlighting.

In any case, it would seem to be a consequence of how things currently work, so I'm ok calling it out of scope to move this along.

crasx’s picture

StatusFileSize
new50.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,511 pass(es).
[ View ]
new2.92 KB

reroll, no conflicts

crasx’s picture

BLadwin’s picture

At MidCamp Sprint today (3/22/15) - reading from #344 down to #367. Will add any remaining follow up issues as they arise.

rteijeiro’s picture

StatusFileSize
new50.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,152 pass(es).
[ View ]
new1.64 KB

The code looks fine to me. Just fixed a couple of nitpicks in comments.

BLadwin’s picture

The functionality looks good - this issue has come a long way over many many many (almost too many) comments so instead of polluting this queue with another request, I created #2457373: Inline form error links should bring form element title, form element and the error in view.. Otherwise, marking this RTBC!

BLadwin’s picture

Status:Needs review» Reviewed & tested by the community

... forgot the status update ...