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
Comment | File | Size | Author |
---|---|---|---|
#68 | interdiff.txt | 1.08 KB | tim.plunkett |
#68 | 2578561-ife-67.patch | 37 KB | tim.plunkett |
#63 | interdiff.txt | 942 bytes | tim.plunkett |
#63 | 2578561-ife-63.patch | 36.79 KB | tim.plunkett |
#60 | Screen Shot 2015-10-05 at 16.58.13.png | 290.77 KB | joelpittet |
Comments
Comment #2
tim.plunkettComment #3
aspilicious CreditAttribution: aspilicious commentedwhut? :(
Comment #5
Bojhan CreditAttribution: Bojhan as a volunteer commentedFascinating 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.
Comment #6
xjmOof.
Comment #7
tim.plunkettJust some test fixes
Comment #9
legolasboI guess this is an rc target unless the mentioned follow-ups are fixed?
Comment #10
tim.plunkettNow with no API changes.
Interdiff is only the change I made after reverting files to their original state.
Comment #12
tim.plunkettCleaning up the FormErrorHandler now that we don't render anything, only set messages.
Comment #13
dawehner.
Comment #15
mgiffordI'm opposed to this patch as it will take out an important accessibility improvement in Core.
I've added some related issues from #1493324: Inline form errors for accessibility and UX - I'm pretty sure that some of those issues are not going to be resolved by the patch in this issue. They should be tested though to see if they are related or not.
Comment #16
DamienMcKenna:(
Incidentally, would the work that's going into reverting the functionality and cleaning it up again be better spent finishing the issues mgifford referenced?
Comment #17
DamienMcKennaAlternatively, should the referenced issues have been marked as "critical" as they were blocking the RC? The workflow just seems a little confusing.
Comment #19
tim.plunkettThe 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.
Comment #20
Bojhan CreditAttribution: Bojhan as a volunteer commented@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.
Comment #21
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #22
tim.plunkettI 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.
Comment #23
xjm"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!
Comment #24
xjmComment #25
nod_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.
Comment #26
Bojhan CreditAttribution: Bojhan as a volunteer commentedMy wording was a bit strong. Your right. Sorry :)
Comment #27
tim.plunkettThanks @mgifford.
I've removed the issues that are unrelated to this and inline form errors in general.
Comment #28
cilefen CreditAttribution: cilefen commented#2579779: Regression: Login form error message is redundant and does not make sense
Comment #29
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedGiven 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.
Comment #30
tim.plunkettEasy reroll, #2576037: "Sorry" in user-facing errors violates the User Interface Standards changed some text we are deleting.
Comment #31
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedVery sad to see this reverted, but yes from a product perspective an unfinished feature should be reverted.
Comment #32
alexpottI 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.
Comment #33
xjm@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.
Comment #34
Bojhan CreditAttribution: Bojhan as a volunteer commented@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
Comment #35
pwolanin CreditAttribution: pwolanin at Acquia commentedThis looks like it's disabling a test case - is that really needed for the revert?
Comment #36
tim.plunkett#2557367: Fix inline list CSS changed some assertions that we're removing. Rerolled, and fixed #35.
Comment #37
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIs 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.
Comment #38
tim.plunkett@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.
Comment #39
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedHm, 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.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIf 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.)
Comment #41
webchickThe issue summary here sounded eerily familiar, so quoting myself from the original issue:
...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.
Comment #42
BerdirJust 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?
Comment #43
webchickThe 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.
Comment #44
cilefen CreditAttribution: cilefen commented#2579779: Regression: Login form error message is redundant and does not make sense is a demo of an "opt-out", but of the dsm.
Comment #45
aspilicious CreditAttribution: aspilicious commentedWell, 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.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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).
Comment #47
Bojhan CreditAttribution: Bojhan as a volunteer commentedI 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.
Comment #48
cilefen CreditAttribution: cilefen commented#2579779: Regression: Login form error message is redundant and does not make sense is an effort to move ahead. No?
Comment #49
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedSimilar 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.
Comment #50
tim.plunkettThe changes necessary in FormErrorHandler could absolutely live in a module: https://gist.github.com/timplunkett/c242147ec390e700754d
The changes to core templates, not so easily.
Comment #51
joelpittetHow 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.
Comment #52
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWhy not leave the changes to the templates?
They don't really hurt and allow opt-in for that.
Comment #53
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThere is definitely the possibility to leave a lot more of this in, all what would be needed is to use something like:
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.
Comment #54
tim.plunkettThe 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.
Comment #55
hass CreditAttribution: hass commentedI guess this revert also solves #2506827: Fix missing field/form element/datetime errors on form submission. Great.
Comment #56
tim.plunkettComment #58
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - except for the test fails! Looks great!
Comment #59
tim.plunkettSilly mistake from copy/pasting the unit test.
Comment #60
joelpittetReviewed 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:
Comment #62
tim.plunkett#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.
Comment #63
tim.plunkettManually tested in #59.
hook_help added.
Comment #64
joelpittetBack to RTBC
Comment #65
alexpottI 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++
Comment #68
tim.plunkettOh my, #2488032: Integrate help test into module uninstall test is stupidly opinionated.
Comment #69
webchickExcellent. 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!
Comment #71
mgiffordWow 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.
Comment #73
Strutsagget CreditAttribution: Strutsagget commentedJust 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.
Comment #74
cilefen CreditAttribution: cilefen commented@Strutsagget Enable the inline form errors experimental module.