Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a form validation handler | handle form messages #34043

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented May 20, 2021

A Javascript handler for form|fields messages.

As a developer, I have been several times, in the awkward position, needing to implement custom code, that will co-operate with bs-form fields. So, many times I had the same thought. "What if bootstrap provided the base 'JS adapter' for its fields"

This PR tries to provide a Bootstrap component, that automatically handle browser validation messages, and in addition will give the opportunity to use custom, given feedback messages.

In advance, it contains JavaScript code, providing

  • on form component:
    • method to clear these messages,
    • get FormData` (for ajax usage)
    • execute validation
    • access each formField instance
    • JS hook, through component configuration, where developers can use for Ajax submission (not provided by default) and/or attach any custom validation messages
  • per field methods:
    • clear appended feedbacks
    • append success | error | info feedback messages

Needs:

  • Documentation
  • Tests
  • Feedback

Cons:
Adds a significant size as it introduces two new classes 🤔

Related #28414
close #28995

Preview : https://deploy-preview-34043--twbs-bootstrap.netlify.app/docs/5.2/forms/validation/#custom-styles

js/src/forms/field.js Outdated Show resolved Hide resolved
js/src/forms/field.js Outdated Show resolved Hide resolved
js/src/forms/field.js Outdated Show resolved Hide resolved
@patrickhlauke patrickhlauke self-requested a review May 24, 2021 22:47
@patrickhlauke
Copy link
Member

patrickhlauke commented May 24, 2021

almost perfect. one annoying issue though: if the form field already has an aria-describedby to begin with (see for instance the "Username" control, which initially already references the element containing the "@" sign), then this is overwritten by the success or error message. instead, it should be concatenated (so that the aria-describedby shows what was there to begin with, a space, and the id of the error message) - see the server side static example later on. this may require storing the value of aria-describedby (if present) before the validation happened, using this to generate the aria-describedby when the validation message is shown, etc.

also, not sure if this PR also touches on the tooltip case, but here the generated markup seems incorrect ... the actual valid/invalid tooltip sits outside of the <div> that is referenced by the aria-describedby - so this references just an empty container, with the actual tooltip being a sibling of this.

@patrickhlauke
Copy link
Member

patrickhlauke commented May 25, 2021

After submitting with empty values, the username results in this structure

<div class="col-md-4">
    <label for="validationCustomUsername" class="form-label">Username</label>
    <div class="input-group has-validation">
      <span class="input-group-text" id="inputGroupPrepend">@</span>
      <input type="text" class="form-control" id="validationCustomUsername" aria-describedby="inputGroupPrepend" required="" data-bs-invalid="Please choose a username.">
      <div class="field-feedback invalid-feedback" id="inputGroupPrepend">Please choose a username.</div>
    </div>
  </div>

note that this now gives both the <span> with the "@" and the invalid feedback <div> the same id, which is invalid. and the end result is that the AT still only announces the latter.

what i meant above, which may not have been clear: the end result should be that the aria-describedby contains a reference to both the previous/existing value it had (pointing to the <span>) and to the validation <div>, as a space-separated value. so the end result should be something like

<div class="col-md-4">
    <label for="validationCustomUsername" class="form-label">Username</label>
    <div class="input-group has-validation">
      <span class="input-group-text" id="inputGroupPrepend">@</span>
      <input type="text" class="form-control" id="validationCustomUsername" aria-describedby="inputGroupPrepend validationCustomUsername-formTip" required="" data-bs-invalid="Please choose a username.">
      <div class="field-feedback invalid-feedback" id="validationCustomUsername-formTip">Please choose a username.</div>
    </div>
  </div>

@patrickhlauke
Copy link
Member

also, the latest changes seem to have broken the tooltips case https://deploy-preview-34043--twbs-bootstrap.netlify.app/docs/5.0/forms/validation/#tooltips which now just show the browser default error handling rather than the custom tooltips

@patrickhlauke
Copy link
Member

btw probably worth saying: it might look like i just come in here to point out what's wrong... but this is already a great way forward! if we can tweak these last few bits, it'll be wonderful to have validation properly addressed in BS.

@GeoSot
Copy link
Member Author

GeoSot commented May 25, 2021

btw probably worth saying: it might look like i just come in here to point out what's wrong... but this is already a great way forward! if we can tweak these last few bits, it'll be wonderful to have validation properly addressed in BS.

You are right on this. give me some minutes

note that this now gives both the with the "@" and the invalid feedback

the same id, which is invalid. and the end result is that the AT still only announces the latter.

got it. Thanks

@patrickhlauke
Copy link
Member

it's almost perfect now, thanks for indulging me there @GeoSot. the last aspect that would be good to address is potentially tricky.

if you try to submit an invalid/incomplete form, and you get the error messages...if you then go back and correct the errors, visually the error message disappears and the green success styling is shown. however, behind the scenes, the aria-describedby still points to the now hidden error message. and even if an error message is completely hidden using display:none, screen readers will still read/announce it when it's referenced by aria-describedby. this can then lead to confusing situation where the user corrects the form entry, it visually shows it's correct, but regardless it still announces the error.

i think the only way around this would be to watch for a change event on the form (or individual form controls themselves?), and then dynamically resetting aria-describedby to its initial value (e.g. in the case of "Username" only pointing to the @ container) / changing it to the success message id if present (and in the case of "Username", this would then need to point to BOTH the @ and the success message if it had one ... for this reason, will be good to keep a reference right at the start to whatever the aria-describedby value was before validation as well.)

@patrickhlauke
Copy link
Member

i.e. after validation once the user does go back and edit, currently it visually then shows this

image

and the markup for that username input is

<input type="text" class="form-control" id="validationCustomUsername" aria-describedby="inputGroupPrepend validationCustomUsername-formTip" required="" data-bs-invalid="Please choose a username.">

i.e. note that aria-describedby now still points to inputGroupPrepend validationCustomUsername-formTip when it should just point to inputGroupPrepend

@patrickhlauke
Copy link
Member

patrickhlauke commented Jun 6, 2021

last outstanding aspect, if possible, is removing the reference to the error message in aria-describedby once a previously invalid field becomes valid (either right away when the user fulfills the validation requirement, or at least when they try to resubmit and the validation run happens again).

i.e. currently, even after filling in a previously empty field, and the field now not being valid, the error message is still associated/announced ... and with aria-describedby, even if the target referenced is display:none, it's still announced. see this animation (with NVDA speech viewer), and note that after filling in the "City" and then moving away and returning, it still announces as "City, edit, required, please provide a valid city".

Animation showing field validation and NVDA speech viewer

@patrickhlauke
Copy link
Member

getting there :)

i see the aria-describedby is correctly updated if at first something was invalid, and you then go in to make it valid. great stuff.

however, doing multiple erroneous submissions results in error messages being multiplied...

form showing multiple same error messages all under the fields as a result of multiple submissions

in the case of the tooltip variant, visually there's only one tooltip, but the aria-describedby balloons out with multiple references...as a result, AT will announce the referenced tooltip multiple times too

tooltip case, showing single tooltips, but from the DOM view it's clear there are multiple references to the tooltip in the aria-describedby

lastly, in the "supported elements" case, the initial errors aren't associated with the form fields. and editing the form to then be valid, some of the errors remain visible (but still not announced) https://deploy-preview-34043--twbs-bootstrap.netlify.app/docs/5.0/forms/validation/#supported-elements

textarea showing the valid green tick but still with the red error message below

@GeoSot
Copy link
Member Author

GeoSot commented Jun 14, 2021

thank you @patrickhlauke :)

I was aware of duplicated feedback elements, but I wasn't sure how to solve it. Although, I think I have found a valid solution.

lastly, in the "supported elements" case

I didn't change something there (at least yet), as it is a fixed example that shows the classes usage. (it works the same way on our published docs)


In order to do our checks, the only changed sections are

  • Forms:
    • Custom styles
    • Tooltips
  • Examples:
    • Checkout
    • Checkout RTL

When we finish the basic tests, we can see more on documentation area, and js-code needs to be discussed|optimized with the help of the js team

@patrickhlauke
Copy link
Member

I was aware of duplicated feedback elements, but I wasn't sure how to solve it. Although, I think I have found a valid solution.

conceptually, it likely needs to do a check of "if the relevant error is already shown/the aria-describedby already contains the reference, do nothing, otherwise show the error/add the reference to the aria-describedby".

I didn't change something there (at least yet), as it is a fixed example that shows the classes usage. (it works the same way on our published docs)

we probably need to make those dynamic as well (starting off with an already present error, but then following the same JS logic that removes any reference/hides the error message/etc once user interacts with it)

@patrickhlauke
Copy link
Member

added a commit that normalises all ids used in the various form examples, and adds the relevant aria-describedby etc to the static examples, as a first step

@patrickhlauke
Copy link
Member

patrickhlauke commented Jun 14, 2021

i see the issue of multiple errors appearing when hitting submit multiple times has now been fixed. great stuff.

out of interest, how easy/hard would it be to hook up the validation script to https://deploy-preview-34043--twbs-bootstrap.netlify.app/docs/5.0/forms/validation/#supported-elements as well? as currently the CSS changes kick in (partially) and it leaves this in weird semi-states...

textarea field showing the green tick, but the error tip still showing underneath it

@patrickhlauke
Copy link
Member

one other oddity i spotted (but likely no easy way around it) is that for the checkbox example, the generated error comes straight after the input, so ends up before the label

invalid checkbox with error message - error is directly in line with the checkbox, with the label one line down

@GeoSot
Copy link
Member Author

GeoSot commented Jun 14, 2021

conceptually, it likely needs to do a check of "if the relevant error is already shown/the aria-describedby already contains the reference, do nothing, otherwise show the error/add the reference to the aria-describedby".

done

we probably need to make those dynamic as well (starting off with an already present error, but then following the same JS logic that removes any reference/hides the error message/etc once user interacts with it)

I think it will be a bit more interventional, than it should

out of interest, how easy/hard would it be to hook up the validation script to https://deploy-preview-34043--twbs-bootstrap.netlify.app/docs/5.0/forms/validation/#supported-elements as well? as currently the CSS changes kick in (partially) and it leaves this in weird semi-states...

It can be done, but it is considerable better to make .is-valid & is-invalid to have more weight than pseudo-classes, as they are used as overrides

one other oddity i spotted (but likely no easy way around it) is that for the checkbox example, the generated error comes straight after the input, so ends up before the label

done

@GeoSot GeoSot mentioned this pull request Dec 23, 2021
2 tasks
@GeoSot GeoSot force-pushed the gs-forms branch 2 times, most recently from 7316a4d to 2f3ad96 Compare February 3, 2022 16:45
@GeoSot GeoSot force-pushed the gs-forms branch 2 times, most recently from 9c8311e to 1bd6c94 Compare February 17, 2022 22:38
@GeoSot GeoSot force-pushed the gs-forms branch 2 times, most recently from 5275b43 to aa5f413 Compare May 10, 2022 23:22
@GeoSot GeoSot added the v5 label May 10, 2022
@GeoSot GeoSot added this to In progress in v5.3.0 via automation May 11, 2022
@patrickhlauke
Copy link
Member

Just for my own sanity (as I had lost track of where we were at with this), made a quick recording using Chrome/NVDA of how this all currently shakes out

bootstrap-form-validation-pr34043.mp4
  • 0:00 - 0:42 Custom styles: work great/as I'd expect/want them
  • 0:42 - 1:27 Browser styles: work as expected by browser (with Chrome popping up the validation bubbles one at a time as you submit)
  • 1:27 - 1:59 Server side: notice that only the errors are tied to the form fields. for consistency, probably makes sense to also tie the "Looks good" text to the form control, the same way that error messages are
  • 1:59 - 2:18 Supported elements: no association between validation messages and their form fields - wonder if we can either automate this/run the script over it, or if we just manually add the aria-describedby here?
  • 2:18 - 3:00 Tooltips: work as expected

@GeoSot
Copy link
Member Author

GeoSot commented Jun 11, 2022

@patrickhlauke for start, I want to really thank you for your patience and your help.

I saw the review above and :

1:27 - 1:59 Server side: notice that only the errors are tied to the form fields. for consistency, probably makes sense to also tie the "Looks good" text to the form control, the same way that error messages are

seems to be an issue of docs, because we are just indicating the proper markup in case of server validation, fixed in: 07240fe

1:59 - 2:18 Supported elements: no association between validation messages and their form fields - wonder if we can either automate this/run the script over it, or if we just manually add the aria-describedby here?

seems to be an issue of docs, which I handled, adding the aria-describedby attribute manually (we cannot use the js script as the submit btn is disabled ), fixed in: f9041cc

@GeoSot GeoSot force-pushed the gs-forms branch 2 times, most recently from 6b4858d to f9041cc Compare June 30, 2022 22:22
.bundlewatch.config.json Outdated Show resolved Hide resolved
.bundlewatch.config.json Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member

snuck in a tweak to the documentation text itself...

@patrickhlauke
Copy link
Member

@GeoSot you'll probably also want to add a bullet point to the "how it works" section, to mention the extra scripting that ties the form control to its feedback/error message when it appears. just draft it up, happy to give it a read/tweak once there https://deploy-preview-34043--twbs-bootstrap.netlify.app/docs/5.2/forms/validation/#how-it-works

@GeoSot
Copy link
Member Author

GeoSot commented Jul 4, 2022

Thank you @patrickhlauke 🙂

I have it in my mind too, but I am holding myself, till I get some feedback from other @twbs/team members (or contributors), concerning the usefulness of this idea, and opinions over the JavaScript code.
(Just to minimize the process steps and avoid going back and forth on file changing)

GeoSot and others added 2 commits October 8, 2022 00:06
add "aria-describedby" attribute on "supported elements" section

add "aria-describedby" attribute on server side succeed validation messages
* remove the warning about custom errors and tooltips not being accessible ... they now mostly are
* change the phrasing for server-side validation, so that *all* feedback (whether valid or invalid) should have `aria-describedby`
@GeoSot GeoSot force-pushed the gs-forms branch 2 times, most recently from 498df8a to e1047e1 Compare October 7, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.3.0
In progress
Status: Needs review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants