backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

[D8][UX] Add inline form errors #1040

Open jenlampton opened 9 years ago

jenlampton commented 9 years ago

This is part of #378 as it is now a feature included in D8 core (d.org issue):

screen shot 2018-10-28 at 10 16 15 am

This was something that tested really well on the D8 UX study, and we should consider adding to Backdrop as well.

User interface changes

Contrib modules


~~ Advocate for this issue: @klonos ~~

klonos commented 9 years ago

Definitely yes :+1:

Dinornis commented 7 years ago

👍 Yes please.

klonos commented 7 years ago

I say lets add this to the list of 1.8 targets. I'd absolutely love it if we could get this + #2430 by then.

jenlampton commented 7 years ago

I don't think this is possible in 1.x? Have we reviewed that yet?

On May 6, 2017 3:38 AM, "Gregory Netsas" notifications@github.com wrote:

I say lets add this to the list of 1.8 targets. I'd absolutely love it if we could get this + #2430 https://github.com/backdrop/backdrop-issues/issues/2430 by then.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/1040#issuecomment-299631033, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSR9s-Y1UnPkfEhg9BPKfRjKCN9OqKks5r3E2XgaJpZM4FNXQa .

klonos commented 7 years ago

😞 ...really sad face if we'll have to wait until 2.x for these two UX issues (especially since we have not even set a date for wen we'll start working on Backdrop v2).

I know that they are both marked as feature requests and that they would introduce some considerable UI changes, but these changes are to solve some long-standing UX issues; not only in Backdrop, but since D7. Both address a similar issue: the message that we are trying to convey to the user on actions they need to take ("validation error in certain fields"/"changes not saved yet") is hidden from the user's view and they either have to scroll up/down to see what's wrong, or they simply leave the form without saving it and then wonder why their changes were not applied (in the case of #2430).

jenlampton commented 7 years ago

It's not about the feature, it's about API change. There's no argument it's a good/necessary feature, but we can't go changing the way forms work mid cycle if it would break most of contrib!

klonos commented 7 years ago

Fair enough. I wish that we had the experimental features thing in core then. That way we could get reference and the inline messages modules in. People could enable and use them if they want, but they'd be disabled by default. Isn't there a way to do it in a non API-breaking fashion?

jenlampton commented 7 years ago

I wish that we had the experimental features thing in core then. That way we could get reference and the inline messages modules in.

I must admit, I only see disadvantages to having experimental features in core. As contrib projects, they would be updated and improved much faster. Plus, we wouldn't have the pressure of needing to get everything right on the first try (or maintain a legacy of bad choices). But I digress...

For something like inline form errors, that shouldn't even be a module (or something you can turn on or off). Building in extra code to make a UX improvement enable-able seems like a non-ideal solution for core.

As we've been adding modules to core, not all of them remain modules. Token became part of system (i think), pathauto part of path, and entity_view_modes part of Field UI. The same would likely need to be done for ife (or whatever solution we choose).

Isn't there a way to do it in a non API-breaking fashion?

Maybe? But someone would need to look into it to decide if "a non API-breaking fashion" is a good enough solution for core, or if that would be dangerous in any way, or too much of a hack.

bd0bd commented 7 years ago

My dear developers, could you create a small module for webform inline errors? Or port any module from Drupal (Inline Form Errors, Clientside Validation). Currently I am forced sometimes to use Drupal ONLY because of this option.

jenlampton commented 7 years ago

@bd0bd Would you please create an issue in https://github.com/backdrop-ops/contrib/issues requesting that one of these modules be ported? Once there, please add the tag Port request and that way developers will know that you need it :)

daggerhart commented 7 years ago

I have an approach to this problem. It's not perfect, but provides a direction that could be taken to provide inline form errors without modifying core APIs. I wasn't sure how to best organize it throughout core as a PR, so I wrote it as a module for now. I agree with @jenlampton that idealy it is not a module.

https://github.com/daggerhart/inline_form_errors/tree/form-validate

It does 3 main things

  1. It adds a #validate callback to all forms. That callback uses form_get_errors() to collect the existing errors.
  2. It then recurses through the form to find the appropriate element that relates to each error, and stores some additional information (#id and #title) about that element in a static array for later use.
  3. On hook_preprocess_layout, it intercepts form error messages from the $_SESSION['messages'] array, converts them into a single error message with anchor links to each field.

This is a little tricky because I wanted the validate callback to run very late, and the preprocess_layout function to run very early. I ended up using a high module weight and theme_registry_alter so that it could execute its hooks at their appropriate time, but I think these tricks could be avoided if it were to become a part of core.

A screenshot of an error-filled form is on the project readme. If this is something we want to explore further as a core feature, I'm happy to rewrite as a PR to core.

klonos commented 7 years ago

@daggerhart care to file an application to become a member of backdrop-contrib? That way, you could get that module in contrib-land, and once there is a release, people will be able to install it via the project browser 😉

daggerhart commented 7 years ago

Just offering one approach that could get inline form errors into core, but I have other ideas as well. I probably should have placed this functionality into core and submitted as a PR. I don't mean for this above repo to be considered a contributed module.

Happy to become a contrib maintainer once I have an idea for something I want to maintain indefinitely, but this isn't that ;). I'll withhold further random-idea-posting on this issue for now. Most of my ideas are probably better hashed out in gitter/other chat before throwing them online.

jenlampton commented 7 years ago

@daggerhart please don't hold back on random-idea-posting, this is a fantastic start! :)

I like your idea of adding a #validate handler. That's a nice clean way we could add something new that may not require adding a module to do so... I wonder if there's a way we could add the #validate to all form elements (instead of all forms) and handle each with ajax, so that we wouldn't need to mess around with layout preprocessing to get and set the inline errors...

klonos commented 7 years ago

@daggerhart please don't hold back on random-idea-posting, this is a fantastic start! :)

I second that. It might be long till we get something in core, but if in contrib, people can start using it now. Besides, brainstorming is one of the best ways to solve problems.

I like your idea of adding a #validate handler.

...not an expert dev, but what I like with this idea is that it is an API addition and these are easier to sneak into core 😄 ...we recently added a #help element in #2546

jenlampton commented 7 years ago

@klonos we already have #validate in core, we're just brainstorming ways we might start using it for this purpose.

daggerhart commented 7 years ago

Back with another approach as a branch named "preprocess-only" in my repo --- https://github.com/daggerhart/inline_form_errors/tree/preprocess-only

This approach accomplishes very similar results, but is better in a number of ways.

  1. It works on the field-level, so theoretically it is more ajax-friendly
  2. It uses the core backdrop_get/set_messages() mechanisms for error message storage, so theoretically these messages are just as reliable as any other.
  3. It only uses 2 functions to make everything happen. theme_preprocess_form_element() and theme_preprocess_status_messages()

It works like this:

  1. In theme_preprocess_form_element() it detects if the current form element has an error within the form_get_errors() array. If it does, it sets a message using a custom $type. backdrop_set_message( $error_data, 'inline_form_errors');
  2. In theme_preprocess_status_messages() we get those messages, and convert them into a single message of the $type 'error', with anchor/jump-links to individual fields. backdrop_get_messages('inline_form_errors');

I'm sure it has issues, just throwing out more ideas.

jenlampton commented 7 years ago

We're two weeks away from code freeze for 1.8, and with no code here yet to review or revise it's not likely this feature will get done in time. Bumping to 1.9.

klonos commented 5 years ago

I have closed #1715 as a duplicate. I'll be copy-pasting comments from there and/or updating the issue summary accordingly...

Also made this part of #378 since the inline messages are now in D8 core for some time.

klonos commented 5 years ago

@daggerhart I was looking at the screenshot of your experimental contrib module and it reminded me of #3329 which I filed recently (merely because the validation errors are rendered/styled same as 'error' messages).

I did not look at the code implementation, but I was thinking that perhaps for core we should implement it as a #validation_error form element property, which we default to something like "This field is required" for all fields (or provide defaults per field type). That way, we would allow per-field custom validation errors.

klonos commented 5 years ago

...also @daggerhart, could you please move that module to contrib? That way, we could perhaps all contribute. Once we have a relatively solid solution, we could create a release and tag this as a "promoted" module (#3193).

domaingood commented 4 years ago

+1 Yes please.

klonos commented 2 years ago

I liked this design (from https://drupalcontributions.opensocial.site):

image

image

klonos commented 2 years ago

I find the validation error summary message at the top of the form really useful, as it provides an overview of what needs to be addressed. The anchor links that auto-scroll to the specific fields is also a UX+, but once you have scrolled to the first validation error, the summary is now outside your viewport. I would like us to also try to solve that UX issue; perhaps by adding "next error" links?

klonos commented 2 years ago

This is such a small code change for such a big UX gain. I think that we should aim to get it in for 1.21.0, and I'm advocating for it.

quicksketch commented 2 years ago

I moved this into 1.21.0 just so we can discuss it in weekly meetings.

jenlampton commented 2 years ago

@bugfolder mentioned that Civi uses https://pear.php.net/package/HTML_QuickForm/docs/latest/ for their inline form errors. Might be worth looking into to see how they did it.

klonos commented 2 years ago

I've temporarily removed my advocacy for this issue, and moved it to #5348 instead. We need that (and #5346) in first.

herbdool commented 1 year ago

I've got a client that did an accessibility audit of their site and got a recommendation to make errors inline with the fields. It did not say they needed be server or client-side generated so I started working on an IFE module port (it basically works). But I ended up testing out @daggerhart experimental module instead: https://github.com/daggerhart/inline_form_errors/tree/preprocess-only (the preprocess-only branch) because I think it might be more robust and didn't need all the settings (yet?)

A solution for core, I think, would probably be better to use something sooner during the element lifecycle rather than in the preprocess step. Not sure where though. And may require also adding aria-invalid, aria-errormessage as mentioned in https://github.com/backdrop/backdrop-issues/issues/5672

klonos commented 1 year ago

Could it be as simple as this? (doesn't work with all forms, so needs further tweaking): https://github.com/backdrop/backdrop/pull/4334

image

herbdool commented 1 year ago

@klonos I left a comment on your PR.

I've also cloned daggart's https://github.com/herbdool/inline_form_errors (using preprocess of element) since I think it's the best approach we've got right now. And I need it for a client. Despite the warning on the experimental module it works well enough (with a small bug fix).

I would even put it on backdrop-contrib as a first attempt, but there's no license info on the module. @daggerhart would you be willing to GPL your experimental module?

klonos commented 1 year ago

Thanks @herbdool 🙏🏼 ...would you expect things to live in a backdrop_preprocess_form_element() function then? (which would in turn live in core/includes/form.inc)

klonos commented 10 months ago

Noting that recent implementations in Drupal (at least in the Claro admin theme) also highlight the field labels when there's validation errors. A couple of examples:

image image