revisit interactive form validation error reporting

[brprice]

See https://github.com/hackworthltd/primer-app/pull/733#pullrequestreview-1245709485 and https://github.com/hackworthltd/primer-app/pull/730#issuecomment-1380467867.

Currently there are some strange UX around when and what errors are reported in forms -- sometimes they seem to update on each keystroke, and sometimes not. There are also some odd order-dependencies. However, all errors seem to be reported correctly when "submit" is pressed (and if any errors then exist, the modals are not closed).

If we decide that errors should be reported/updated on each keystroke, it may also be nice to disable the "submit" button when the form is invalid.

[dhess]

See https://github.com/hackworthltd/primer-app/pull/733#issuecomment-1380737173 for why I don't agree that disabling the submit button is good UX. To recapitulate, client-side validation in our frontend app is just an optimization, and always will be due to our goal of making the frontend as ignorant of Primer-specific semantics as possible. Therefore, it would be worse UX to disable the submit button until frontend validation succeeds, and then still have the possibility that the submission will be rejected because the backend says so. Better, IMO, just to adopt the convention that submit is always active, but may raise additional validation issues once pressed. (This is a pretty common convention in other applications, so should not be surprising to the student.)

[dhess]

Having now read the react-hook-form documentation more carefully, what we're seeing in the simple forms in #733 is expected behavior. From the docs:

[The default onSubmit validation mode] will trigger on the submit event and inputs will attach onChange event listeners to re-validate them.

And then, after the initial submit:

[The reValidateMode property] allows you to configure validation strategy when inputs with errors get re-validated after a user submits the form (onSubmit event and handleSubmit function executed). By default, re-validation is actioned during the input change event.

Personally, I think this is a reasonable default, which is why I didn't bother to investigate the validation behavior when I was testing #733 prior to review. I would describe the philosophy as something like this: trust that the user knows the rules and will get it right without any nagging, but if they do make a mistake, show them the error that explains the mistake, and persist it until the condition that is causing the mistake is rectified.

I don't feel super strongly about it, and I'm OK being overruled on this if I'm in the minority. However, I do think the fact that it's the default behavior for a very popular React forms library is worth considering.

Having said that, in the case of the typedef form UI in #730, I think these defaults aren't working as described in the docs. For example, you immediately get an "empty constructor" warning upon creating the first constructor in the type. It's possible that I need to keep reading about how validation interacts with dynamic forms, or maybe there's a bug.

[dhess]

I haven't heard back yet from upstream on the pre-submit validation behavior in useFieldArray, but I have done some debugging to get to the bottom of another issue with react-hook-form, namely that once an error appears on a field, that error doesn't go away until that specific field is changed.

This is unfortunate for us, because we annotate fields in the typedef UI when a name is identical to another field's name. These errors can be rectified by changing either name, but depending on which field the student changes, the error may persist even after the conflict is resolved. For example, in this video, the first constructor name overlaps with the type name, and the error is displayed on the first constructor field; but after fixing the issue by changing the type name, the error message persists:

https://user-images.githubusercontent.com/2438/212468238-4c1af447-4bee-408e-bb88-5df60777be08.mov

Only after an onChange on the field where the error was originally displayed is the error message finally removed.

What's particularly frustrating about this situation is that, in the code, the validation is running on the entire form contents, and by adding debugging info that logs to the browser console, I can see that, after changing the type name to resolve the conflict, the error object returned by the validation is empty:

So the validation checks are working, but react-flow-hook is apparently not considering the entire error object after validation runs, but only any properties of the error object that correspond to the specific field that was just changed.

I think this behavior is the same as described in this upstream issue: https://github.com/react-hook-form/react-hook-form/issues/3642. It seems like this is a conscious choice by upstream to reduce the number of re-renders. There are a few workarounds suggested in that issue, so I'll look into those next.

[dhess]

in the case of the typedef form UI in #730, I think these defaults aren't working as described in the docs. For example, you immediately get an "empty constructor" warning upon creating the first constructor in the type. It's possible that I need to keep reading about how validation interacts with dynamic forms, or maybe there's a bug.

This behavior is due to the "add" and "remove" buttons not having the property type="button", per https://github.com/react-hook-form/react-hook-form/discussions/9766#discussioncomment-4694254. Who knew that you needed to tell the DOM that a button is a button?

Anyway, I have a fix coming for this particular issue for #730.

[brprice]

This behavior is due to the "add" and "remove" buttons not having the property type="button", per react-hook-form/react-hook-form#9766 (comment). Who knew that you needed to tell the DOM that a button is a button?

Huh, it would have taken me a long time to work that out. Now I know what to look for, I see that https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-type says "submit" is default, and https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#notes says "If your buttons are not for submitting form data to a server, be sure to set their type attribute to button"

[dhess]

Yes, upstream to the rescue.

As part of #730, I've now fixed our UIButton component to address this, as well.