Problem/Motivation

When #1493324: Inline form errors for accessibility and UX was committed, it was known to be unfinished and buggy.

#2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE) was a plan to resolve the outstanding issues, but very little progress was made on those.
Issues like #2346773: Details form element should open when there are errors on child elements and #2509268: Inline errors repeated on child elements in module uninstall form are major regressions.

Proposed resolution

Retain the changes to template files
Move the new functionality to an Experimental module called "Inline Form Errors"
Default behavior will use D7-style error messages.

Remaining tasks

N/A

User interface changes

Reverting to D7 style errors unless the "Inline Form Errors" module is enabled.

API changes

N/A

Data model changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
68.29 KB
aspilicious’s picture

whut? :(

Bojhan’s picture

Fascinating issue, it is not too surprising that basically all activity dropped of on important followups. However for a important UI issues, I would almost consider this rule :)

The UX is truly abysmal for the parts, were we didn't fix it. The same could be said for other parts of core, were we degraded the UX significantly - but in this case it affects the very core of Drupal - its forms.

Hopefully with our new release method, we can do a lot more put it in - hope that we fix the followup's, if not pull it back out. This particular change, will be quite hard to put in post RC though I assume.

xjm’s picture

Oof.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
73.46 KB
5.67 KB

Just some test fixes

legolasbo’s picture

Issue tags: +rc target

I guess this is an rc target unless the mentioned follow-ups are fixed?

tim.plunkett’s picture

Now with no API changes.
Interdiff is only the change I made after reverting files to their original state.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
44.96 KB
5.69 KB

Cleaning up the FormErrorHandler now that we don't render anything, only set messages.

dawehner’s picture

Issue tags: +Needs manual testing

.

DamienMcKenna’s picture

:(

Incidentally, would the work that's going into reverting the functionality and cleaning it up again be better spent finishing the issues mgifford referenced?

DamienMcKenna’s picture

Alternatively, should the referenced issues have been marked as "critical" as they were blocking the RC? The workflow just seems a little confusing.

tim.plunkett’s picture

Status: Needs work » Needs review

The work to fix it is hard and unknown if it's even possible. The work to revert this is done.

It should never have been committed in such a broken state.

I was too desperate to get it in after four years of work, I didn't step back and realize that a pity-commit wasn't best for Drupal.

Bojhan’s picture

@tim.plunkett Let's have Dries/Angie weight in and not strong arm with more patches that's not very nice.

I am on the fence, the a11y improvement is important - but the current state is also rather disruptive/difficult to keep up. Its mostly disheartening, that none of the followups received any activity for 3 months.

bojanz’s picture

I am in favor of proceeding with the revert.
Inline errors landed very late in the D8 cycle and resulted in unprecedented breakage.
I seriously don't remember a single core patch in the past 2 years that caused so many major bugs without being reverted right away.

Furthermore, we have no clue how to solve #2509268: Inline errors repeated on child elements in module uninstall form without introducing additional BC breaks, since we'd need to rethink all of our assumptions about how
errors are inherited across form elements and field widgets. I discussed this issue with yched and timplunkett in Barcelona and we got nowhere.

tim.plunkett’s picture

I don't think getting a patch green is "strong arming", and I resent the accusation.

Without the recent revision, I was unsure of what API changes were necessary.

Now we know what we're facing, and the product manager can make a decision.

xjm’s picture

Issue tags: -rc target

"rc target" is for issues that can committed during RC, a very select subset only at committer discretion. Please do not add that tag except in consultation with a core committer. :)

"rc deadline" are issues that must be finished before RC1 or otherwise must be postponed.

More info: https://groups.drupal.org/node/424518

So retagging. :) Thanks!

xjm’s picture

Issue tags: +rc deadline
nod_’s picture

I'm sad as well but strongarm is a bit brutal. @Bojhan we used the same process with overlay. Patch to remove it was green before any formal decision was made. But in this case it's one of the form subsystem maintainer making the patch.

Bojhan’s picture

My wording was a bit strong. Your right. Sorry :)

pwolanin’s picture

Given the amount of work tim.plunkett put into the original patch, I would give a lot of weight to his call to revert it before release.

I think this also makes since in terms of the new philosophy that Dries suggested in Barcelona that we shouldn't be including in core features that are not complete and have a lot of unresolved follow-ups.

tim.plunkett’s picture

Fabianx’s picture

Very sad to see this reverted, but yes from a product perspective an unfinished feature should be reverted.

alexpott’s picture

I too am sad to see this reverted but I think it is correct choice. On commit it was stressed that the work was unfinished and that it was important to get it finished. However, we have to accept that it has not been done and looking at want needs needs to be done versus reverting - it is clear which path will lead to a quicker and less buggy release. +1 to making this change and reverting inline form errors.

xjm’s picture

@Bojhan, this patch will not be committed without the signoff of webchick, Dries, or possibly both, per the tag. Rolling patches isn't forcing anything.

Bojhan’s picture

@xjm I know, see #26. This will revert a very big WCAG win, so we need to be quite aware how this will impact our marketing around this.

Sad to see the hundreds of hours that went into this, sucked up by lack of follow up. But Fabian points here holds true, and this is arguably something that can still be introduced in 8.1

pwolanin’s picture

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -328,7 +326,7 @@ public function testFieldAdminHandler() {
-  public function testAvailableFormatters() {
+  public function _testAvailableFormatters() {
     // Create a new vocabulary.
     Vocabulary::create(array('vid' => 'tags', 'name' => 'tags'))->save();

This looks like it's disabling a test case - is that really needed for the revert?

tim.plunkett’s picture

#2557367: Fix inline list CSS changed some assertions that we're removing. Rerolled, and fixed #35.

David_Rothstein’s picture

Is it necessary to revert the whole thing, or would it be possible to leave behind a small part that still contains most of the accessibility improvement?

For example, remove the inline form errors (and go back to error messages at the top of the page) but instead of the messages being like "@name field is required.", they could be like "@name field is required. <a href="#edit-field-name">Go to the field</a>." So you could still easily get from the error message to the corresponding form element, which (as I understand it) was the biggest part of the accessibility improvement.

The link could be hidden from everyone but screen reader users also (and at this stage probably should be, since I'm sure even with that there would still be some bugs left behind).

I had thought of the above while wondering if the accessibility improvements from that issue could be backported to Drupal 7 somehow, but never really tracked down if it was feasible or not.

tim.plunkett’s picture

@David_Rothstein unfortunately that's half of the part that's broken.
If we had those links, they would not function for errors inside details element, or anything hidden via #states.

I'm not reverting the entire patch, as I leave the new FormErrorHandler, I just rewrite it to call drupal_set_message() always.

Fabianx’s picture

Hm, maybe an even less invasive approach is to keep the inline form errors, but only for screen readers (and tests) for now.

Then for screen readers there could be an extra text saying:

"See below for details."

or

"See below for the same message near the failed input field."

That would duplicate things, but as far as I have understood screen-readers don't take into account hiding via JS of details element, etc. anyway ...

That would keep the accessibility part, but restore visible functionality back to the old version.

Would that not potentially keep the best for both worlds?

That would be #37 with the additional restriction that that link would only be visible for screen readers.

David_Rothstein’s picture

If the element being linked to is hidden with display:none, then I think most screen readers would experience the non-functioning links also.

But the question is whether the accessibility benefit to screen reader users (for the majority of form elements) would outweigh the bugs experienced by screen reader users for a few form elements? That is what I was getting at in #37.

It's also worth pointing out that it might be pretty easy to deal with the non-functioning links in JavaScript: On page load, check if the element being linked to is visible. If not, don't show the link to screen reader users either. (Of course making the link actually work would be the better solution, but this one is the simpler fix.)

webchick’s picture

The issue summary here sounded eerily familiar, so quoting myself from the original issue:

The thing that is unfortunate about this patch is it leaves us in a situation where there are three possible ways for form validation errors to present themselves to end users [...]

So while I understand there are valid technical reasons for this visual disconnect on the various types of validation, the bottom line is that overall this does ends up making Drupal look sloppier as a product, and even worse, does so for our least capable users (end users of Drupal sites). :( It also opens up a new open "front" in D8, with buckets of follow-up work to do in order to get us back down to the previous state which was only 2 possible error "looks and feels" (browser-side + server-side), while we are at the same time trying very hard to remove things to do to ship Drupal 8.

I am therefore inclined to hold this until it's "really ready," myself, which means eliminating the "old school" Form API error styling altogether, and switching wholesale to the new Form API style. I realize though that there was ample pushback on that idea, though... not the least of which reason is this issue is pushing 600 comments. (OTOH, we shouldn't exactly be setting precedent that the way to get something controversial in is to make generate enough comments that core committers feel bad for you. ;) We're still cleaning up stuff from a few patches that went in during feature freeze where that was the case. :\)

...and here we are. :\

So despite the fact that I really, truly recognize that this represents a big blow to out of the box WCAG efforts, I do land in favour of rolling this back until it's actually complete. This is consistent with the "release train is leaving the station; it's either done or not done" direction @Dries laid out in his Barcelona keynote for all kinds of sensible reasons. This is also why I favour a mostly-clean rollback vs. trying to work out some interim solution with < 48 hours to go before RC1. (We hope.)

I talked to Tim and he assured me that he's leaving enough of the old patch in that putting this onto 8.1.x+ will still be possible,. The other good news is that this patch has already made it through the core gauntlet once; that work doesn't need to be re-done, just the remainder of the work to make this issue complete. We've also learned from UX testing in Minnesota that it didn't present any cognitive problems to users, so there's nothing in my view stopping this from re-entering core once all the follow-up details are taken care of (in the same patch). In fact, I'd love to see this go in as a prioritized change to 8.1.x once it's ready.

Code freeze for RC1 will be in a few hours, and I'd like to commit it before then so there's still (a small sliver of) time to catch any fallout. However, I noticed that the original issue #1493324: Inline form errors for accessibility and UX wasn't pinged about the existence of this issue, and that seemed very uncool to me. So just did that and will come back here in 4-5 hours so at least there's a chance of others who worked on this patch providing their input before it gets the axe.

Berdir’s picture

Just a quick question based on discussion in the office.

Can we make it somehow so that we make it opt-in (and don't opt-in any core forms for now)? Then we can enable it in 8.1 for certain forms where it does make sense, e.g. node forms or so.

If we're talking about a deadline of a few hours then it's probably too late to implement that?

webchick’s picture

The problem I think is there's no forms that you can necessarily make that "opt-in" call about, since it only works for a finite list of specific elements, and all forms can be altered to add other elements. Especially node forms.

cilefen’s picture

aspilicious’s picture

Well, I would love to have an opt-in for developers, that way we can enable it in our projects.
90% of the time, we *have* to put the errors inline due to design/UX restrictions.

if core doesn't do this by default I have to hack it in anyway.
In Drupal 7 we had the IFE module: https://www.drupal.org/project/ife

Sadly enough nobody worked on it for D8 as it "wasn't" needed anymore.
I know the maintainer (stijndm) and he isn't going to work on a drupal 8 version.
So at *this* point we are in a worse position than drupal 7 (if we revert this).

BUT I do understand why we would revert this. I truly hope we will figure something out for 8.1.x
I have a feeling this will *never* be fixed if we try to inline error each form.

David_Rothstein’s picture

I talked to Tim and he assured me that he's leaving enough of the old patch in that putting this onto 8.1.x+ will still be possible.

I don't see how, unless it's an opt-in feature. This is the kind of change that could really break people's themes.

A more limited version that's only for screen reader users (like #37 or #39) could probably be done for all sites in 8.1.x+ though (without having to make it opt-in).

Bojhan’s picture

I stated in #589 that we should resolve the outstanding issues. We didn't not even close, so I am OK with removing this for now.

However I think its fair to the hundreds of hours that went in (from myself, tim, mgifford etc.), to have some guidance on what needs to be done for this to go back in (8.1). Especially considering David his concerns, since I also imagine it will break a hell of a lot - if its put in after 8.0.0, since you haven't accounted for this pattern in your design work.

For this to go in, I expect that for this to go back in, we need to resolve at least the following 3 issues:

I am very cautious of the various cans of worms opened in this issue about an (opt-in) and #2579779: Regression: Login form error message is redundant and does not make sense which basically open the door to rehashing a 600 comment issue. The direction we have chosen is a compromise of many many factors, this reason this is going out is not on design direction (e.g. like the overlay) - but the bugs we could not resolve appropriately within time.

Not to start a discussion about 8.1 already, but the point of adding features to core after release - is not to make it all an opt-in :) Then we end up with a permissions like page for everything we add, every checkbox, option, small design alteration.

cilefen’s picture

cosmicdreams’s picture

Similar to Berdir's question in #42: Is it possible to have the current work rolled into a module. Perhaps rolling it as a module will be a good test for the deep level of integration modules will have in Drupal 8. It may also give us a way to field test the changes before making it into core.

I've listened to Dries talk about the release train, and read up on the articles that followed. The idea is fantastic. I would love to see that we could do things in contrib / a feature branch of D8 and get things to a point where we can rapidly iterate and test the changes in the field.

tim.plunkett’s picture

The changes necessary in FormErrorHandler could absolutely live in a module: https://gist.github.com/timplunkett/c242147ec390e700754d

The changes to core templates, not so easily.

joelpittet’s picture

How about that idea of a toggle switch that essentially "reverts" this, but you have to turn on to use? Then we can look at making it default on in 8.1.x possibly.

Fabianx’s picture

Why not leave the changes to the templates?

They don't really hurt and allow opt-in for that.

Fabianx’s picture

There is definitely the possibility to leave a lot more of this in, all what would be needed is to use something like:

  // Display any error messages.
  $variables['errors'] = NULL;
  if (!empty($element['#errors_show_inline']) && !empty($element['#errors']) && empty($element['#error_no_message'])) {
    $variables['errors'] = $element['#errors'];
  }
 }

for all those if statements.

This keeps at least a framework to add this back, also can be added back on a per element basis so makes it way easier for ife module.

Also new theme templates for contrib form elements can be build in a forward-compatible way.

The main change remaining for this patch would be:

- a) Tests
- b) The main messages to show again all errors, which is easily overridable.

tim.plunkett’s picture

The template changes *are* important for doing this later in 8.y.x
However, I don't agree with the code proposed in #53, the #errors_show_inline is confusing and contradictory with #error_no_message.

As for a toggle switch, @webchick proposed an experimental module to renable this.

So, here is inline_form_errors.module.

hass’s picture

tim.plunkett’s picture

Title: Revert "Inline Form Errors" » Move "Inline Form Errors" functionality to optional module and restore D7-style form errors by default
Issue summary: View changes
Fabianx’s picture

RTBC - except for the test fails! Looks great!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
36.28 KB
1.36 KB

Silly mistake from copy/pasting the unit test.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
251.8 KB
290.96 KB
290.77 KB

Reviewed the code and everything seems to be in order then tested it out locally with the form style error module https://github.com/dmsmidt/errorstyle

Before Patch:

After patch:

After patch with experimental enabled:

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 2578561-ife-58.patch, failed testing.

tim.plunkett’s picture

#2488032: Integrate help test into module uninstall test was committed in between the DrupalCI run and the PIFR run. Working on a hook_help now.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
36.79 KB
942 bytes

Manually tested in #59.
hook_help added.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

I think that the experimental core module is an awesome idea - we get the best of worlds - we are nearer RC and after RC and 8.0.0 is released we can continue to work on inline form errors.

timplunkett++

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: 2578561-ife-63.patch, failed testing.

The last submitted patch, 63: 2578561-ife-63.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
37 KB
1.08 KB
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent. Really glad with how this turned out, all things considered.

Ideally, this would wait on a11y maintainer review, but we need to finish everything we were going to commit for RC1 like 6 hours ago (this issue has been in play all day, and directly impacts shippability, hence the exception). We still have a little bit of wiggle room tomorrow if there are serious reservations with this approach, but agreed with alexpott that it seems the best of both worlds: the functionality stays in core, but we're honest about its current state, and have a place to continue building off it in 8.1 and beyond.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed e55a7d8 on 8.0.x
    Issue #2578561 by tim.plunkett, joelpittet, Bojhan, Fabianx, xjm,...
mgifford’s picture

Wow this issue moved fast! Sorry I haven't been more involved in the last 5 days. Bad timing with moving offices (and renovating at the same time).

I do understand why this went in without an accessibility review. We do have to get D8 out and it will be interesting to see how the community responds to a experimental core module. It's certainly worth trying.

It would be important to work to iron out the remaining issues and get the accessible inline form errors functionality enabled by default in 8.1.

It would be good to follow issues that are related to this.

Status: Fixed » Closed (fixed)

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

Strutsagget’s picture

Just wondering what the status is right now and how to get inline form errors in Drupal 8?

Sorry just found it. You activate it under experimental modules like a normal module.

cilefen’s picture

@Strutsagget Enable the inline form errors experimental module.