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

Added some missing form validation standard features (implemented for #1181) #1167

Merged
merged 21 commits into from Feb 14, 2023

Conversation

xdev1
Copy link
Contributor

@xdev1 xdev1 commented Feb 1, 2023

The following form handling standard features have been added:

  • Shoelace form controls now have read-only props validationMessage and validity
  • FormControlController now handles cancelable form invalid events

Please let me know what you think of this draft. Thanks.

PS: This PR was previously meant for #1163, but it's now dedicated to #1181 - sorry for the chaos

@vercel
Copy link

vercel bot commented Feb 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
shoelace ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 10, 2023 at 10:11PM (UTC)

@xdev1 xdev1 changed the title #1163 - allows to show validation messages below form controls as alternative to the normal HTML5 form validaton behavior #1163 - allows to show validation messages below form controls as alternative to the normal HTML5 form validation behavior Feb 1, 2023
@xdev1 xdev1 marked this pull request as draft February 1, 2023 23:57
@xdev1 xdev1 changed the title #1163 - allows to show validation messages below form controls as alternative to the normal HTML5 form validation behavior #1181 - Added some missing form validation standard features Feb 5, 2023
@xdev1 xdev1 changed the title #1181 - Added some missing form validation standard features Added some missing form validation standard features (implemented for #1181) Feb 5, 2023
@claviska
Copy link
Member

claviska commented Feb 6, 2023

I didn't have time to test this today, but I reviewed the code and don't see anything that concerns me. If it works, I'm OK proceeding.

I would suggest moving src/internal/validity-states.ts into the form controller so all the form/validation stuff is in one place. (This will also prevent an extra chunk from loading when used via CDN.)

@claviska
Copy link
Member

claviska commented Feb 6, 2023

Note to myself: make sure to add an example of this (i.e. custom inline validation) to the docs.

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 7, 2023

I didn't have time to test this today

@claviska Just to make sure, I will inform you as soon as there's something to see/test/review/whatever here - currently there's nothing really finished to be tested (as things have changed a bit). Would otherwise just be a waste of time for you.

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 9, 2023

We can probably add a checkbox to the original example to add a bit more variety, then copy that and demonstrate the minimal code required to move the errors inline.

@claviska Okay, thanks. Done and available as preview. Please be aware that I've not yet removed my previous inline validation demo (marked as Inline Form Validation (old version - to be deleted after testing) // TODO!!!!) as this might be useful for testing => has to be removed soon, of course.

Current status of this task: Everything finished and commited except for the tests.

@claviska
Copy link
Member

claviska commented Feb 9, 2023

Thanks for the update. We'll need to revisit that example to make sure it's accessible. Since the error message is in an ::after pseudo element, it doesn't get announced by screen readers.

I'm happy to help with this. If all we're waiting on is tests, let's finish that up and then we can tackle a11y.

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 9, 2023

I'm happy to help with this. If all we're waiting on is tests, let's finish that up and then we can tackle a11y.

I would prefer to clarify this asap as long as you are available today (the tests can wait).
This "100% userland inline validity" thingy only works with manipulating attributes and using ::after on the Shoelace form control. I do not have any idea how this can be done without ::after in "100% userland".
Would it be an option to also add some aria-... attributes on the form control from userland? I'm not really an expert on that aria stuff, so maybe I ask silly questions.

If this is not an option, then another option would still be this "1% not-userland" solution where we would have to add this snippet to each SL form control (+ some a11y stuff):

To be placed below the form-control-help-text part.

  <div part="form-control-validation-message"  hidden>
    ${this.validationMessage}
  </div>

Would make things in userland much simpler (literally a handful lines of JS + some more handsful of CSS).
But as I understand you've never been a fan of this solution.

I'm open for any other suggestions of course 😉

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 9, 2023

[Edit] Removed silly proposal with aria-errormessage

@claviska
Copy link
Member

claviska commented Feb 9, 2023

It has to map to an element as shown here.

<p>
  <label for="email">Email address:</label>
  <input
    type="email"
    name="email"
    id="email"
    aria-invalid="true"
    aria-errormessage="err1" />
  <span id="err1" class="errormessage">Error: Enter a valid email address</span>
</p>

So it would need to be applied to the internal input and point to a container that holds the error message. This is exactly how aria-describedby works, for reference.

Without modifying any internals, you can probably get away with aria-live except I'm not sure how users will know the error is linked to any specific field.

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 9, 2023

LOL, at least I should read the first sentence of web pages before posting links to them 😄 . Thanks for the clarification.
The easiest solution is always: #1181 has actual nothing to do with inline form validation. So we could easily remove that "Inline form validation" demo from "form_control.md".

@claviska
Copy link
Member

claviska commented Feb 9, 2023

The easiest solution is always: #1181 has actual nothing to do with inline form validation. So we could easily remove that "Inline form validation" demo from "form_control.md".

Or use the browser's built-in validation and get it all for free! 🙃

I joke, and I'd like to support this use case. The more long-winded response is that this would be easier to do in userland if we had a way to poke through the shadow DOM and assign aria-errormessage from the outside.

In the meantime, we can still make this work. However, it will entail injecting a container into the shadow DOM. This is a bit dirty, but we can rely on the consistent structure of form control parts.

Essentially, Shoelace will use the browser's constraint validation mechanism by default and, if you want inline errors, you'll need to cancel the sl-invalid event, inject the error container with message, add the aria-errormessage attribute. We can wrap this neatly into an example in the docs and possibly even export a helper function to make it more "official."

Again, I'm more than happy to help out with this. Let me know what you think.

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 9, 2023

Again, I'm more than happy to help out with this.

Frankly, I would be really thankful if you could implement that inline validation stuff the way that you have described above ❤️ ... my automatic tests for that validity stuff turned out to be toooooo good 😉, reported some additional bugs - grrrrrr... so I'm happy if I can concentrate on these bugs first.

Do whatever you like to do with this activateInlineFormValidation function 😂

export a helper function to make it more "official."

😄👍

@xdev1
Copy link
Contributor Author

xdev1 commented Feb 9, 2023

inject the error container with message

Then you could easily give it a part attribute, couldn't you? ... so we would not need that data-error attribute any longer...

Copy link
Member

@claviska claviska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. I'm pulling it in so I can polish a few things up and work on an updated inline validation example.

Thank you!

@claviska claviska merged commit 4a28825 into shoelace-style:next Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants