Lift HTML5 validation errors into the formState slice

Having the form display / enforce HTML5 Errors was implemented very early on - in a panic - to prevent preview updates that include preview-crashing values. Since then, we've added JSON schema validation that serves this purpose more robustly. This has me wondering if the html5 validation is necessary at all.
On a surface level, it can be confusing to have a mixture of schema and HTML5 validation on the same form.

We've also already had to create a workaround due to HTML5 and JSON schema disagreeing about what constitutes a required field violation, opting to go with JSON schema's definition.

If there's something that HTML5 validation enforces that the AJV schema checking is not, then perhaps this issue is correct to promote it's role within the form. If the HTML5 validation is at best replicating what AJV already does, then we're probably better off removing the HTML5 validation logic from onChange.

// 👀 DO WE REALLY NEED THIS 👇
// Check if the input is valid based on HTML5 attributes before continuing.
      if (e.target instanceof HTMLInputElement && !e.target.reportValidity()) {
        const inputElement = e.target;
        const requiredAndOnlyProblemIsEmpty =
          inputElement.required &&
          Object.keys(inputElement.validity).every(
            (validityProperty: string) =>
              ['valid', 'valueMissing'].includes(validityProperty)
                ? inputElement.validity[validityProperty as keyof ValidityState]
                : !inputElement.validity[
                    validityProperty as keyof ValidityState
                  ],
          );
        // We will return early unless the only problem caught by native
        // validation is a required field that is empty.
        if (!requiredAndOnlyProblemIsEmpty) {
          return;
        }
      }
// // 👀 IF THIS PART THAT WAS ADDED LATER VALIDATES AGAINST THE WHOLE SCHEMA? 👇
// Check if the input is valid based on JSON Schema before continuing.

      if (
        fieldName &&
        newValue &&
        e.target instanceof HTMLInputElement &&
        e.target.form instanceof HTMLFormElement
      ) {
       // RUN JSON SCHEMA VALIDATION
        if (!validateNewValue(e, newValue).valid) {
          return;
        }
      }

There is one bit we'd need to change in this condition, though:

if (
        fieldName &&
        newValue &&
        e.target instanceof HTMLInputElement &&
        e.target.form instanceof HTMLFormElement
      ) 

Here, newValue should be (newValue || newValue === '') so empty strings get the schema validation, too.

That works for me, the main issue here was we weren't writing the errors to the store

bnjmnm → changed the visibility of the branch 3537946-lift-html5-validation to hidden.

With the 3537946-page-data-only MR:

1. HTML5 validation is only used by the page data form - the component instance form is already using JSON schema. However, as you can see here, when the HTML5 validation is triggered, the messages are rendered by our application, not the browser native. This makes it visually consistient with the component instance form.
2. We have maintained the requested functionality of letting empty strings through, even when a validation warning is present, as seen by the request here. (note that when that was implemented, the focus was on component instances.
3. Notice that despite the empty field, the title in the preview is the most recent non-empty value. This may require some discussion regarding how to address (if at all) as it's different from components, but it shouldn't happen here as This behavior was not caused by this MR. I mention this as it's seems like the kind of thing that might surface during manual testing and be mis-attributed to these changes. Also note the Front End request includes the empty string, so if we did want the preview to allow an empty title, it probably requires a back end change.

Looks great - thanks!

do we want to take the opportunity here to make the validation messages nicer and match the Figma designs?

I'm going to vote no on doing this, although I think it was completely reasonable to suggest. The areas being changed in the MR are happening at an earlier stage than anything that would impact the appearance of those messages. Keeping it in its own issue avoids this being delayed by surprise test failures due to the changes, and gives us an issue where there's the opportunity for community members to discuss concerns about the designs (however unlikely those are to occur - the designs seem pretty good)

@bnjmnm's comment in #8 is 🤩👏 — thanks so much for that!

→ the server-side changes 🐛 Hero component failed to render after removing text Active made to make this possible are specifically only for the component instance form. But the client-side changes in #3529788 were generic.

#10++ too.

Needs reroll because 📌 Narrow the conditions under which we allow a prop to be an empty string Active landed.

larowlan → closed merge request !1362