Inline form errors for accessibility and UX

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.

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.

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...

oops .. uploaded the patch twice instead of the screenshot

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

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

Oooops .. wrong patch. Lets try this

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

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.

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.

A couple fixes for stuff I broke in #15

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.

@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.

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/

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

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

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?

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.

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?

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.

@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.

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.

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.

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!

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.

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?

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

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...

@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.

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.

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...

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.

#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).

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

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.....

+++ 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.

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.

@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?

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.

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

oops ... forgot to attach patch

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

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

@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?

Could an exception be made for search?

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

#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.

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).

created #1789768: search_box_form_submit() improperly calls form_set_error()

@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.

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.

+++ 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.

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...

#1789768: search_box_form_submit() improperly calls form_set_error() Kindly needs a careful review, please. =)

Re-rolled against HEAD.

This still needs a lot of work.

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

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

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

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

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.

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()

Re-roll

This should fix at least all the notices.

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

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

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

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

tagging

system.theme.css moved into css/

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

Looks like a few tests need to be adjusted.

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.

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

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.

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

What are we doing? That it changes style?

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?

@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).

Re-roll after FormBuilder service

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

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

Whoops

Layer upon layer

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

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.

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.

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

Added related issue.

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

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.

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.

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.

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.

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!

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?

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.

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.

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!

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

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

I'm working on this.

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:

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

Updated screenshot:

131: form-inline-errors-1493324-131.patch queued for re-testing.

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.

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

137: form-inline-error-1493324-137.patch queued for re-testing.

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

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

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.

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.

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

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

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

Forgot about _theme()

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.

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.

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!

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?

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.

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.

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.

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

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.

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

Adding tags

is there a reason to postpone?

@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.

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