Overview

Follow up to #3529788: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases

In that issue it was identified that HTML5 errors aren't displayed in the UI once the user blurs the input

Proposed resolution

Push HTML5 errors into the formState slice

User interface changes

CommentFileSizeAuthor
#8 html5-validation-page-data.png658.29 KBbnjmnm
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Status: Active » Needs review
bnjmnm’s picture

Status: Needs review » Needs work

Having the form display / enforce HTML5 Errors was implemented very early on - in a panic - to prevent preview updates that include preview-crashing values on the props form 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 in props
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 both forms. If the HTML5 validation is at best replicating what AJV already does, then we're probably better off removing the HTML5 validation logic from the props form, so it's only present on the page data form, which doesn't do schema validation.

// 👀 DO WE REALLY NEED THIS FOR PROPS FORM 👇
// 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 THAT 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.

larowlan’s picture

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.

bnjmnm’s picture

Status: Needs work » Needs review
StatusFileSize
new658.29 KB

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.
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Looks great - thanks!

bnjmnm’s picture

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)

wim leers’s picture

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

(note that when that was implemented, the focus was on component instances.) → the server-side changes #3529788: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases 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 #3537945: Narrow the conditions under which we allow a prop to be an empty string landed.

bnjmnm’s picture

Assigned: bnjmnm » Unassigned

  • wim leers committed 37aba129 on 1.x authored by bnjmnm
    Issue #3537946 by bnjmnm, larowlan: Lift HTML5 validation errors into...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.