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

Support @validateOn="blur/change" #24

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Support @validateOn="blur/change" #24

merged 2 commits into from
Feb 2, 2023

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Jan 27, 2023

This is a draft PR to just start a discussion, will close it afterwards as it is not ready for review yet...

Let's say we have @validateOn="blur". Now on an input (or other field control) the user causes the blur/focusout events to trigger. The action that must follow is that we validate, all fields or just this one, but most importantly we only show the field's errors that triggered the event, not any other.

So the question is this: how do we know, which field caused the event? There are several ways to do this, all with their pro and cons:

  1. we can let the event bubble up to <form> (so our main <HeadlessForm> component), inspect the event target's name attribute (which is what we passed as form.field @name="...") and use that to look-up the field. This. is what this PR currently does, and it works so far. However it requires the event to be triggered from a real native form control, and it needs to have name properly set up. That's the case here, but this might not work everywhere, e.g. might not work for rich custom controls like a <PowerSelect>, or combined controls like a date picker that (internally) is split up into 3 <select> with name="date_{day,month,year}", so the name won't match what the field's name is (just date in this example). I think it would be possible to provide a yielded action like triggerValidationFor(name), but this requires actively wiring this up for the user.
  2. we could only do the latter, i.e. provide a triggerValidationFor(name) and all controls need to wire this up explicitly, so our own here (field.input, field.checkbox etc.) same as any custom ones., and not rely on any name matching logic at all
  3. or we could make our form.field component create a wrapper div, and attach the event listeners to that. Currently it does not render any markup (truly headless), so when the event bubbles up from the <input> it would immediately reach the <form>, at which point we wouldn't know which field this belongs to (unless we do 1.). But with a wrapping div (which would have ...attributes, so users could use it for their own styling etc.) we would be able to catch the event within the boundaries of the field, so we know which field this event belongs to, and show validation only for this field. This has the downside of an additional wrapping div, that does not have any other good reason to exist like a11y or so, so not really headless in a pure way. But the upside is that this would work out of the box, both for our own controls, but also any custom ones, as long as they are able to trigger focusout/change events properly.

Any thoughts?

const { name } = target as HTMLInputElement;

if (name) {
const field = this.fields.get(name as FormKey<FormData<DATA>>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the name matching logic of variant 1 happening!

field.validationEnabled = true;
}
} else {
// @todo how to handle custom controls that don't emit focusout/change events from native form controls?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yeah, that's the downside of that approach 😬

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Jan 27, 2023

Choose a reason for hiding this comment

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

imo,
custom controls can / should emit these events tho.

"To be a custom control, it must behave like a control, including having <these events>"

Copy link
Contributor

@ynotdraw ynotdraw Jan 27, 2023

Choose a reason for hiding this comment

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

imo,
custom controls can / should emit these events tho.

Yeah, I agree. We definitely will want validation to trigger for controls like <PowerSelect, <PowerCalendar, etc. used inside of forms. Getting these elements to act more like their native versions would be 💯 (e.g., <PowerSelect -> <select>, <PowerCalendar -> <input type="date", etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but the question is still whether to do 1. or 3. then.

For 1. to work, the event must have event.target.name === @name passed to field. A <PowerSelect> could have <input type="hidden" name={{@name}}> (does it, idk?), and if that triggers focusout then we are fine.

But what about the example with the 3-select date picker. It will receive @name="date", but will split this up internally to 3 selects of names date_day, date_month, date_year. When one of those selects triggers a focusout, our form will not know what field this belongs to, because event.target.name === 'date'.

For 3., event.target.name would not matter, and everything would "just work"(TM). But the downside (wrapper div) remains...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the caveat for 1 is fine.
For 3, can you write out the details of this approach by extending this demo -- i just want to be super extra sure I understand how this wrapping div approach would work

Copy link
Contributor Author

@simonihmig simonihmig Jan 27, 2023

Choose a reason for hiding this comment

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

Not sure tbh how the demo would relate to this addon, but I'll try it this way:

The current form.field template would have a wrapping div like so:

<div {{on "focusout (fn @triggerValidationFor @name)}} ...attributes>
  {{! here is what is currently in the template }}
</div>

@triggerValidationFor would be curried into the component when yielding it as a contextual component, so that's a private implementation detail.

From the caller's site:

<HeadlessForm as |form|>
  <form.field @name="date" as |field|>  <-- this would now be a div that captures the event, and *knows* what @name it belongs to!
    <field.label>Date:</field.label>
    <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
      @value={{field.value}}
      @onUpdate={{field.setValue}}
    />
    <field.errors/>
  </form.field>
</HeadlessForm>

So here, as long as the custom component triggers a focusout event, validation would work ootb, no matter if there is no native form element, or what name it/they have or not have...

Btw, triggerValidationFor would not call e.stopPropagation(), so the event would not stop to bubble at the form.field div level, if that was a concern. You could still attach event listeners to the <form> or even parent elements, and see the event...

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Jan 27, 2023

Choose a reason for hiding this comment

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

maybe this div is too much to assume. I imagine this may be something that a custom-control author would want to opt in to.

as in:

<HeadlessForm as |form|>
  <form.field @name="date" as |field|> 
    <field.captureEvents @events={{"focusout"}}> or @events={{array "focusout" "change"}} ? idk
     ^-- this would now be a div that captures the event, and *knows* what @name it belongs to!
    <field.label>Date:</field.label>
      <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
        @value={{field.value}}
        @onUpdate={{field.setValue}}
      />
    </field.captureEvents>
    <field.errors/>
  </form
</HeadlessForm>

But maybe that begins to defeat the purpose of the headless form?

would be curried into the component when yielding it as a contextual component, so that's a private implementation detail.

would there be a way to not add those event bindings if no validator is passed? (conditionally apply the on modifier?)

Btw, triggerValidationFor would not call e.stopPropagation(), so the event would not stop to bubble at the form.field div level, if that was a concern

it was a concern! this is good news.

You could still attach event listeners to the

or even parent elements, and see the event...

most excellent.


I think I've now changed my opinion from being all in on option 1, to at least being a bit 50/50 on options 1 and 3 now.

I do have a concern though, so, what would happen if a field has multiple focusables? (this may sway me back to option 1? idk!)

<HeadlessForm as |form|>
  <form.field @name="date" as |field|>
    <field.label>
       Date:
       <button {{on 'click' (fn this.helpAbout "date")}}>help</button>
        ^ - would we end up validating on events from here?
    </field.label>
    <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
      @value={{field.value}}
      @onUpdate={{field.setValue}}
    />
    <field.errors/>
  </form
</HeadlessForm>

@NullVoxPopuli
Copy link
Contributor

I think allowing bubbling is one of our requirements, because users may want to do this approach to get easy / no-fuss two-way binding™

@ynotdraw
Copy link
Contributor

For number 3 above - I could be misunderstanding this, but thought I'd ask. One concern I'd have here is if the markup is:

<div ...attributes>
  <input />
</div>

rather than

<div>
  <input ...attributes />
</div>

It's my understanding that you have no way to access the input component directly to add modifiers or attributes without exposing component arguments, right? So if I wanted to do <input type="number", there'd have to be a component argument to pass that into the input element directly, right?

<div ...attributes>
  <input type={{@type}} />
</div>

As a user, I'd much rather have access directly to the input element with the attributes spread so that I can provide attributes and modifiers that just work.

<SimonsInput type="number" {{on "focus" this.someAction}} />

So I think for me, I'm leaning more towards option 1 knowing that, but also am curious what @NullVoxPopuli and @nicolechung think.

@NullVoxPopuli
Copy link
Contributor

I, and our current users, also want direct access to the control elements 🙃

@simonihmig
Copy link
Contributor Author

simonihmig commented Jan 27, 2023

It's my understanding that you have no way to access the input component directly to add modifiers or attributes

No, that's not correct. Nothing in that regard would change with 3. here! See this example adding (imaginary) classes to all elements:

<HeadlessForm as |form|>
  <form.field @name="date" class="add-padding-to-wrapper" as |field|>  <-- this would now be a div that captures the event, and *knows* what @name it belongs to!
    <field.label class="align-right">Date:</field.label>
    <field.input class="border-gray"/>
    <field.errors class="text-red" />
  </form.field>
</HeadlessForm>

In other words, there would still be no implicit element, that doesn't have any ...attributes you can use to add whatever you want!

What you see above is already the current API. The only thing that changes is that you can actually do <form.field class="whatever" as |field|>. As it is now, it has no backing element at all, so no ...attributes, see here

@simonihmig
Copy link
Contributor Author

Btw, I think in most cases you will want a wrapping div around the label and input anyway, like a flex container for positioning the children horizontally. So you could use that to do something like <form.field class="flex" @name="foo">....

In our case, when we use <Field> (was that the name of the new reusable component? also didn't check the actual API, just a example here) as in

<HeadlessForm as |form|>
  <form.field @name="date" as |field|>  <-- this would now be a div that captures the event, and *knows* what @name it belongs to!
    <Field>
      <:label>
        <field.label class="align-right">Date:</field.label>
      </:label>
      <:input>    
        <field.input class="border-gray"/>
      </:input>
      <:hint>
        <field.errors class="text-red" />
      </:hint>
    </Field>
  </form.field>
</HeadlessForm>

we would probably end up with one additional wrapper div here. But that's also not the end of the world!? 🤷‍♂️

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jan 27, 2023

having a built-in opportunity for controlling layout (form.field) is a good call out.

I think you sold me 🎉

option 3 ✨
though, I still have a confusion about this scenario: #24 (comment)

@ynotdraw
Copy link
Contributor

ynotdraw commented Jan 27, 2023

Thanks for the great write up! Yeah if that's not an issue at all then I think I'm good with option 3 as well! As you mention, we will want the label and input in a wrapping element anyway.

(Doh, I see now you explicitly mention putting this in field, apologies. Friday brain)

@NullVoxPopuli
Copy link
Contributor

If we all have Friday brain on Friday, we should all take the day off, right? 🙃

@nicolechung
Copy link
Contributor

nicolechung commented Jan 27, 2023

Had to re-read all of this a few times, but option #3 sounds good to me now too 🎉

Btw, I think in most cases you will want a wrapping div around the label and input anyway, like a flex container for positioning the children horizontally. So you could use that to do something like <form.field class="flex" @name="foo">....

This is good to know! Because sometimes we might want the label on the left or right (not just above)

For 3., event.target.name would not matter, and everything would "just work"(TM)

I can't quite visualize how this would work until I see it in a demo or test / maybe I'm not following that whole thread here (quite likely, Friday brain for me too)

@simonihmig
Copy link
Contributor Author

simonihmig commented Jan 28, 2023

I can't quite visualize how this would work until I see it in a demo or test / maybe I'm not following that whole thread here (quite likely, Friday brain for me too)

I can certainly provide a working example, like I could amend this PR to switch to 3., shouldn't be much work. But the outline of the implementation is what I wrote in #24 (comment). See the first snippet there: when you have a div, the field can put on {{on "focusout"}} on it. Though it is not the form control that actually triggers the event, it will bubble up and the event listener attached to the div will see the event. And this event listener knows the name of the field, as @name is in scope for the form.field template.

I imagine this may be something that a custom-control author would want to opt in to.

Oh, that's an interesting approach as well! 🤔 Option 4. I guess 😅

I think it could instead also be a yielded modifier? By currying @name, it would also just know it:

<HeadlessForm as |form|>
  <form.field @name="date" as |field|> 
    <div {{field.captureEvents}}>
     ^-- this would now be a div that captures the event, and *knows* what @name it belongs to!
      <field.label>Date:</field.label>
      <CustomDatePickerComponentConsistingOfThreeSelectsWithUnknownNames
        @value={{field.value}}
        @onUpdate={{field.setValue}}
      />
    </div>
    <field.errors/>
  </form
</HeadlessForm>

would there be a way to not add those event bindings if no validator is passed? (conditionally apply the on modifier?)

Probably yes. Should be the case when you explicitly pass @validate functions. Not sure how exactly #10 will turn out, but maybe that will be difficult to support (conditional event handlers w/ native validation), as you apply things as plain HTML attributes like required, so we don't really know what is in use (unless we inspect DOM to change JS behaviour, which no, I don't think we want to do...)

I do have a concern though, so, what would happen if a field has multiple focusables?

Oh, another great point, I haven't thought about. So yes, without further measures, this would trigger validations as it is now. But there are ways to handle this. Off the top of my head:

  • we could apply some heuristics on what elements we see as valid form controls. Like we would could check for ['button', 'a'].includes(event.target.tagName) and ignore them if they match. However there might be a risk that some custom control is indeed a button (like a custom select), and focusing out is equivalent to focusing out of an input? 🤷‍♂️
  • there is always the escape hatch that a custom control author could intercept the event and call stopPropagation(), if they think this shouldn't be handled by headless form. We could even provide that form the addon, like a {{field.ignoreValidationEvents}} modifier
  • Saad mentioned this in a Zoom session recently: instead of showing validations on every focusout, we could only show them when the value has actually changed. I first thought of this as another mode for @validateOn, but it could also be the default behaviour for @validateOn="blur" maybe? Anyway, in this case just focussing out of the button in your example wouldn't do a thing.

Btw nitpick: we use focusout internally, but the API calls for @validateOn="blur". Do want to keep this? The rationale would be that "blur" is more commonly used and understood, but I am not really sure that is the case!? 🤔

@simonihmig
Copy link
Contributor Author

I created a real PR (#25), but keeping this open here for further discussions.

I am leaning towards option 4 here, which is basically option 1 plus additional primitives for custom controls in case they are needed, like a yielded modifier (see above comment) as a variation of what @NullVoxPopuli proposed with field.captureEvents. WHat do you think?

@simonihmig simonihmig merged commit f7a640c into main Feb 2, 2023
@simonihmig simonihmig mentioned this pull request Feb 2, 2023
@simonihmig simonihmig deleted the validation-timing branch February 3, 2023 08:58
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.

4 participants