Catching form submit and showing validation errors about form-participation HOT 27 CLOSED

not sure what @calebdwilliams thinks but the PR LGTM

from form-participation.

ok, so, digging into this issue a little bit I just think you're maybe implementing things incorrectly in Lit.

My custom inputs do the same thing you're talking about here:

I want to show error message once you press submit without doing any change on input.
If there is an invalid-text attribute (as in example) this text should be visible instead of required validator's default error message.

In the code sandbox you provided, the one thing I see wrong with your custom error implementation is that you havent told Lit to re-render and change the display because you haven't set any property that Lit is watching reactively.

This validityCallback implementation, the way I read it, just sets what the error message should be, but its not actually setting any reactive property so that Lit will re-render the component and make the error show

In my implementation I have a Lit updated() function that calls the FormControlMixin's setValue() if the value has changed. That's another thing I'm not seeing with this implementation. In your original code sandbox demo, you're not setting a value anywhere that I can see? So the validity state is correct because the component has a required attribute and no value, but the validators aren't running because setValue() isn't being called?

I'm sure there's more that I'm missing, but I have inputs in my implementation that respond form submission without being touched, so I know using the mixin allows it. My implementation is private and fairly complex, but I'll see if I can put together some kind of demo that shows it

Another thing that complicates this a little bit is that Lit is smart enough to not re-render if a reactive property didnt actually change. So if you have a reactive value prop that starts as '' and gets set to '' somewhere in the first lop cycles (firstUpdated or something), Lit won't actually count that as a re-render.

from form-participation.

@muratcorlu @hrmcdonald @michaelwarren1106 I just opened #44. I think this might address some of the issues y'all have been having. If I understand the issue correctly (and I'm still not entirely sure I do), we were missing some steps in the mixin's #onInvalid listener (that also helped remove that setInterval ugliness).

Let me know if you think this will do what you need or if you're able to test it. If I need to put this into a prerelease state, I'm happy to do so (but probably will have to set that up for a different branch, so it would take more time to mess with the Actions).

from form-participation.

Just published @open-wc/[email protected]

from form-participation.

Without a reproduction it's difficult to diagnose, but based on this Pen things seem to be working as expected. This is for numbers one and two.

For number three, you need to set a default value for your form control, something like

class MyInput extends FormControl(HTMLElement) {
  constructor() {
    super();
    this.setValue('');
  }
}

This will set an initial value for your component when the element is created.

from form-participation.

Hi @calebdwilliams Thanks for your answer.

I created a demo: https://codesandbox.io/s/inspiring-stallman-08lg9x?file=/src/my-input.ts

In this demo there is simple form field with required validator and a custom error message property. And I put 2 submit buttons, one is a native, and another is a custom one that uses submit helper.

Now, my target is, regardless of which type of submit button is used:

1. I want to show error message once you press submit without doing any change on input.
2. If there is an invalid-text attribute (as in example) this text should be visible instead of required validator's default error message.

When I use setValue in constructor, validityCallback runs but in that phase my component is not connected yet, so it can not read it's attributes. Also, I feel like validators should run at the moment that reportValidity of the form called.

Another point is, if you use native submit button, my input component can catch submit event and then call it's own reportValidity method. But that's not possible with custom submit button, since it just calls form.reportValidity() and because of it returns false, it doesn't trigger submit event. But form's reportValidity doesn't call my input's reportValidity method. So I can not understand how to show error message at that moment.

from form-participation.

For the first issue, if you want to set an initial value you can choose to do that in either the constructor, the connectedCallback or formAssociatedCallback.

The submit issue is a bit more complicated as the submit method just calls the HTMLFormElement.prototype.reportValidity which I believe should call the individual element's ElementInternals.prototype.reportValidity. It seems like the submit button calls the elements reportValidity instead of the internal reportValidity. I can't rely on reportValidity being present (or returning the appropriate value) otherwise I could do this:

export const submit = (form: HTMLFormElement): void => {
  const { elements } = form;
  const validity = Array.from(elements).map(element => {
    // @ts-ignore
    return element.reportValidity?.();
  });
  if (!form.reportValidity() || validity.includes(false)) {
    return;
  } else {
    const submitEvent = new SubmitEvent('submit', {
      bubbles: true,
      cancelable: true
    });
    form.dispatchEvent(submitEvent);
    if (!submitEvent.defaultPrevented) {
      form.submit();
    }
  }
};

from form-participation.

Native submit button doesn't call reportValidity() of the elements as well. But if there is no novalidate attribute on the form, it doesn't stop submit event, even if there are some invalid elements in the form. And then inside my custom element I'm catching this submit event like below:

 connectedCallback(): void {
    super.connectedCallback();

    this.form?.addEventListener('submit', (e: SubmitEvent) => {
      if (!this.reportValidity()) {
        e.preventDefault();
      }
    });
}

Maybe it's better that submit helper also respect novalidate attribute and don't block submit event if novalidate presents even though form is not valid.

If there is no novalidate, current behaviour looks same with native submit button (just showing bubble of the first invalid input).

from form-participation.

@muratcorlu got it, the function should absolutely respect novalidate. I'm happy to add that or you can open a PR if you'd like to commit that back .

from form-participation.

@calebdwilliams I'll try to open a PR in next days

from form-participation.

I've got a similar situation to some of what is discussed here, and I'm not sure how to approach it.

Take the example in this stackblitz: https://stackblitz.com/edit/vitejs-vite-jdgtpg?file=src/my-element.ts

It's an element attempting to act as a native input. I've marked it required and set an ok-enough-for-this-demo email regex pattern on it. Validation and everything appears to work as expected. I only want to show the error when the user has changed the value or on submit.

The only case that I can't get working is when submit is clicked before the input has been changed. I have no way of marking the control "dirty" on submit. How can I tell when a submit was attempted even if it fails validation? Hooking into the validityCallback() won't do because it gets called on firstUpdated so the control is registered with the form properly.

If I wanted to track "touched" next I'd run into the same situation. I'd want to marked touched true on blur or an attempted submit, but not always during a validation check.

from form-participation.

@hrmcdonald your issues are completely same with mine. The issue about not being able to set your input dirty with submit, will be fixed with my PR (#40) and adding novalidate to your form element. Then your submit event listener in your connectedCallback will start working and you will able to set your input state as dirty.

But even in that case validityCallback is not being called in my tests. Even if I call setValue('') in firstUpdated to trigger an initial validation, in submit phase, somehow validation message returns back to the default value of the requiredValidator without calling validityCallback. I'm currently debugging this but apparently internal runValidator method only called when we set a value or blur the element.

When I debug the issue, I noticed a weird thing. There is a private method that sets the validation message like below:

Main decision point here is validationTarget. In our examples we define validationTarget like this:

@query('.some-element')
validationTarget: HTMLElement;

And the first time that our mixin runs the validator is in the constructor of the mixin, by setting value to null (setValue calls runValidator):

In this phase, our validationTarget is undefined, since it'll be filled once our component finishes its first render. Also in this phase our component attributes are not red by Lit yet. So in my case (I get custom error message from component user with invalid-text attribute) even by calling my validityCallback I can not return my custom message.

Then, the condition inside setValidityWithOptionalTarget goes to second one and library does something that I don't understand and try to set validity on validationTarget element up to 100 attempts. Since this happens inside a setInterval, it goes out of sync code and in my case this operation ends after than my component finishes to render. And because of I set the value in my firstUpdated again to trigger validation in correct time, that validation reads correct value, and sets it, but just after that, that validity set overrides my value with the default one.

woah... I'm tired while writing, I hope you could follow 😊

So, now my questions:

1. Do we really need that setInterval? There is a to-do comment there, about documenting an edge case. I think I'm already on that edge case. 😊 I'm trying to write a custom select component, that doesn't have any native element in it but also want to make it focusable in case of an error. Do I have an option other than using validationTarget for that? Or is that code somehow help for it? Can we avoid that race condition?
2. Do we need to call setValue in constructor inside the mixin?

My PR currently doesn't include anything about this part but if @calebdwilliams can guide me about a direction, I can try to fix this issue as well.

from form-participation.

@hrmcdonald your issues are completely same with mine. The issue about not being able to set your input dirty with submit, will be fixed with my PR (#40) and adding novalidate to your form element. Then your submit event listener in your connectedCallback will start working and you will able to set your input state as dirty.

True yes, but my example does lean on native validation technically, so I guess you are suggesting we would need to replace the native validation for inputs with custom validators used by the mixin like those defined in this library instead right? Then lean completely on those instead of the native validation correct? While not ideal, I guess yeah, once that is worked out it could work fine.

It might not be possible, but instead, is there not just a way from the form element itself perhaps we can just listen to submit attempts without disabling native validation? Or was the conclusion y'all have already arrived at that because the submit event only fires after native validation passes this novalidate route is our only path forward?

from form-participation.

If you want to use with native bubbles, I think it already works. I slightly changed your example here: https://stackblitz.com/edit/vitejs-vite-geybkw?file=src%2Fmy-element.ts,index.html

Listening invalid event and focusing to the input was breaking the flow, I think.

from form-participation.

If you want to use with native bubbles, I think it already works. I slightly changed your example here: https://stackblitz.com/edit/vitejs-vite-geybkw?file=src%2Fmy-element.ts,index.html

Listening invalid event and focusing to the input was breaking the flow, I think.

Ah I see. Got it thanks. I would prefer the custom behavior you are also after, but this might have to do until this issue is resolved.

from form-participation.

Good to hear that it fixed your issue.

My PR needs review from maintainers. But the issue that I mentioned above about custom error messages is a bit more trickier. Maybe we can solve it in a separate PR and track it in a separate issue.

from form-participation.

@michaelwarren1106 Thank you. What do you think about other issue that I explained in comment above? #39 (comment) I can convert it to a separate issue if it makes more sense.

from form-participation.

i’ll dig into it more later tonight

from form-participation.

OK, I'm thinking there's a bigger issue at play here and I'm not quite sure I understand it, so I'm reopening this issue and would love to dive in a bit this week on this one to shore up this part of the developer experience.

It seems like there are two or three things going a bit wrong or not as expected. Could you provide reproduction steps with the actual outcome vs the expected outcome?

Also, sorry for the delay in getting back to y'all, I have had a couple quick trips in a row and haven't had a ton of time to work on this stuff.

from form-participation.

@muratcorlu @hrmcdonald this is a very naive example of what I think you're describing and it is working the say I would expect. Am I missing something: https://codepen.io/calebdwilliams/pen/NWLgoEr?editors=1010

from form-participation.

I'll have a look for the PR and try to clarify if it covers the issue that I mentioned or not as soon as today or tomorrow. Thanks for your effort in advance!

from form-participation.

Confirmed closed by #44

from form-participation.

This is published as version 0.7.0.

from form-participation.

What is your plan to release @open-wc/form-helpers? (submit helper work #40 )

from form-participation.

That should also be live if a changeset was created. If not all we need to do is create the changeset and merge.

from form-participation.

I still see the v0.2.1 that is published 1 month ago, as the last version. https://www.npmjs.com/package/@open-wc/form-helpers

Should I create a changeset in my PR?

from form-participation.

Yeah, I’d you create a changeset for helpers I’ll get it merged and released. Sorry, the only drawback to changesets is it sometimes gets forgotten (even, or maybe especially, by reviewers).

from form-participation.