Hero component failed to render after removing text

See https://git.drupalcode.org/project/experience_builder/-/blob/0.x/compone... — heading is required. So then the "failed to render" message is normal.

This seems like another case where deleting the required value should restore the original example.

But … we discussed this 2 days ago, and didn't we agree that we'd go for exactly this behavior everywhere, as a way to convey: "dear user, you removed a required input, so of course it cannot render" — where that is shown to the user in:

1. client-side validation in the component instance form
2. the preview (which you're reporting here)
3. the "Publish all changes" button will report this same validation error

So … what's the bug? 🤔

Would like Ben's assessment on this — do you agree, Ben?

After removing the heading from the hero component, a frontend validation error is correctly displayed. However, if the user switches tabs and attempts to edit another field, the component fails to render. This could be improved from a user experience perspective.
Ideally, the component should not fail to render when a required field is left blank. Instead, the error should be shown when the user tries to publish changes with an empty heading field.
In my view, adding validation for required fields during publishing is one issue, while the component failing when text is removed from a required field is a separate concern and which is not a good user experience.

Ideally, the component should not fail to render when a required field is left blank.

I disagree, if a component has an invalid prop, it shouldn't be rendering. That said, the. component should do a better job informing the user why it isn't rendering, which we will address here:

📌 Invalid prop errors should be detailed in Component preview Active

I don't think the current UX is acceptable; invalid form value should not result in the component rendering an error.

I'm wondering why are we throwing an error instead of rendering the component with the empty string? JSON Schema required allows empty string so the component itself should be able to render with that.

Even though JSON Schema allows that, we could enforce a stronger validation and show it as a validation error on the form / review changes panel, but that could be separate from the rendering part.

@lauriii we discussed this at length with a big group while you were on PTO. 📌 Invalid prop errors should be detailed in Component preview Active is what @effulgentsia, @bnjmnm, myself and others arrived at.

Please merge in 0.x before doing more work, because ✨ Add a Video prop type to the Code Component editor Active landed a few hours ago, and made significant changes in this area — enough to also fix 🐛 Default image is not loaded when adding a pattern Active .

This issue is about something else though: it's not about DefaultRelativeUrlPropSources at all, but it's still touching the same code, so merging in upstream now will prevent a very painful conflict later on. Plus, the clean-up that #3534601 did just might end up helping you here! 😊

Switching to needs review, but specifically a back end person should look at my MR + Code comments to make sure the things I'm removing are OK to remove. All the BE changes needed to get this working required removing stuff that I assume was added for a good reason, but since the tests pass maybe it's fine? LMK!

HEAD

When following the steps to reproduce, the client sends this to the server:

{
  "source": {
    "subheading": {
      "expression": "ℹ︎string␟value",
      "sourceType": "static:field_item:string",
      "value": ""
    },
    "cta1": {
      "expression": "ℹ︎string␟value",
      "sourceType": "static:field_item:string",
      "value": "View"
    },
    "cta1href": {
      "expression": "ℹ︎link␟url",
      "sourceType": "static:field_item:link",
      "value": "https://example.com",
      "sourceTypeSettings": {
        "instance": {
          "title": 0
        }
      }
    },
    "cta2": {
      "expression": "ℹ︎string␟value",
      "sourceType": "static:field_item:string",
      "value": "Click"
    },
    "heading": {
      "required": true,
      "jsonSchema": {
        "type": "string"
      },
      "sourceType": "static:field_item:string",
      "expression": "ℹ︎string␟value",
      "default_values": {
        "source": [
          {
            "value": "There+goes+my+hero"
          }
        ],
        "resolved": "There+goes+my+hero"
      }
    }
  },
  "resolved": {
    "subheading": "",
    "cta1": "View",
    "cta1href": "https://example.com",
    "cta2": "Click"
  }
}

(note no key-value pair for heading under resolved!)

MR

This changes the above by ensuring that heading is present, and with a fallback value of "" 👍

I reviewed the server-side changes, and responded on the MR with 3 concerns, and matched those with 3 commits to address those concerns.

Here's hoping @bnjmnm finds my concerns, commits and rationales convincing! 🤞😊

Improve title accuracy.

Major usability impact, so marking .

This has been pretty thoroughly FE / BE reviewed but still needs MR approval for

Redux-integrated field widgets /ui/src/components/form/inputBehaviors.tsx 

I haven't reviewed the whole MR, since others have, but the inputBehaviors.tsx change looks correct, so I approved it since that was the only file not covered by other code owner approvals.

Just catching up on this via 🐛 UI change in 3529788 allows publishing components with empty required fields Active

Late to the party but

Per @lauriii
It should be possible to enter an empty string in a required text field, such as the heading prop in the hero component then

when that prop is emptied the Preview should still update -- it's fine that it is showing an impossible-ish state due to an empty required prop

I think the approach we took here might not have gone far enough - in this case ajv validation is marking the field as invalid and we should not update the client-side model or send the PATCH request back to the backend/store autosave/update the preview. But that doesn't happen on its own

I _think_ the root issue here was with these steps in the OP that allow a user to bypass clientside validation:

Remove heading text, and sub heading text

In so far as this is the order of operations:

1. Remove heading text 👈️ this puts the form into an invalid state
2. and subheading text 👈️ the input behavior triggers the PATCH/ autosave etc because a prop was updated But it does not consider the validity of the form

i.e. a user can edit any other field to bypass our clientside validation

And then in terms of the root cause of 🐛 UI change in 3529788 allows publishing components with empty required fields Active I think it was these two commits added late to ✨ Add a Video prop type to the Code Component editor Active

1. The changes to StaticPropSource here
2. The test changes here to make the above change pass

i.e. I think we should consider revisiting the approach in this issue, and reverting those two commits from ✨ Add a Video prop type to the Code Component editor Active .

In terms of revisiting here, I think we should prevent PATCHing/autosave/preview from happening if the form is invalid.

Without that, we're basically rendering our use of ajv/clientside validation redundant - if a user can edit another field to dismiss validation - there's no point to it and it doesn't give us any protection at all.

I'll put the comments specific to 🐛 UI change in 3529788 allows publishing components with empty required fields Active over there too.

Sorry for reopening a can of worms.

I've opened I added an MR for a different approach against 🐛 UI change in 3529788 allows publishing components with empty required fields Active in which is basically #21

Here's a screenshot showing the form preventing submission until any errors are resolved.

We already had code for setting formErrors in the formState slice, but weren't using it for HTML5 validation, the MR adds the e.target.validationMessage to the errors in formState slice so they persist and we can query for errors in other fields in the commitFormState callback and return early.

This also lets us show a nice user facing error to 'resolve the errors' as we can also query the slice

But doesn't #21/#22 make the whole bug fix moot? The whole point of this MR is to fix the problem that invalid form fields didn't get autosaved and displayed in the preview which is confusing UX. This was intended to tackle the use case of empty fields and we will have a set of other issues following this up.

I agree with @lauriii on this one — the point is indeed that while the Content Author is doing their thing, it's possible that both:

1. neither the prop sources themselves would be considered invalid (i.e. the field types do not contain valid non-empty field item values)
2. nor would the evaluated results (i.e. the values returned by a static prop source getting evaluated wouldn't pass SDC's JSON Schema validation)

Yet we'd still want:

1. the preview to be updated using the invalid data
2. the invalid data to be auto-saved

Because without the first, it'd result in a (real-time) jarring user experience: the preview wouldn't match what the Content Author sees in the component instance form.

Because without the second, it'd result in a (upon return) jarring user experience: the component instance form wouldn't be in the exact state that the Content Author left it — which is a perhaps typical kind of data loss, but it's data loss nonetheless.

In terms of revisiting here, I think we should prevent PATCHing/autosave/preview from happening if the form is invalid.

That is literally what @effulgentsia, @bnjmnm and I proposed, but @lauriii was very strongly against this, for the UX reasons cited above.

Without that, we're basically rendering our use of ajv/clientside validation redundant - if a user can edit another field to dismiss validation - there's no point to it and it doesn't give us any protection at all.

I agree, partially: I don't think it's entirely moot. I think this is about strategically allowing certain technically invalid values when they degrade the UX too much. For example:

• foobar is never a string, so client-side validation informing the Content Author of that and not even trying to update the preview is good
• https:/ (author mid-typing and then pausing to find the URL copy/paste) is not a valid URL, so not even trying to update the preview is once again good
• '' (the empty string) is semantically inadequate for meeting the "required string" JSON schema prop shape, but it is (usually — some code components or SDCs might very well have logic that crashes if they are given the empty string, but that is likely a minority) 👈 this issue!
• etc.

That's why I handled it so narrowly in this MR and included the rationale in long comments. But … I do realize now that on the client side it's handled much more (too) broadly: that needs to be narrowed to only allow it for type: string without a minLength.

1. The test changes here to make the above change pass

I sure was somewhat nervous about this change. My rationale (which I should've posted 😬🙈) is that it actually never made sense for empty strings to get stored for those in the first place! The reason this test worked at all until that commit you cited is because we never actually prevented storing useless data (a field item value that would've been rejected if we'd been calling ::filterEmptyItems()). That made the stored JSON blobs unnecessarily big.

wim leers → changed the visibility of the branch 1.x to hidden.

WDYT about #24 + the draft MR? 😊

This is IMHO the bit we originally missed.

In response to #23 and #24 I think as a minimum we should be displaying and error banner and retaining HTML5 validation errors on the form using the existing styling. So most of my MR but without the early return in the commitFormStateToStore method.

The thing I'm confused by is that we don't PATCH to the backend if the field you're editing is invalid - inputBehaviors doesn't call the commitFormStateToStore method if validation fails.

So I'm not sure why we would want to do that if you edited a different field.

i.e. I don't see the point of having validation if you can bypass it.

I think it's simple to reason about what we do to show a preview in the case of an empty required string like the original MR did (FYI this wasn't an issue until the changes in ✨ Add a Video prop type to the Code Component editor Active that I've reverted in my MR).

But what about this scenario?

1. Add the hero
2. Edit the href to 'not a link'
3. see the validation error 'data must match format "uri"
4. edit the heading which bypasses the validation error
5. Receive the component failed to render error
   

In that scenario what would we do to render the preview? We don't do any validation on the preview generation so we don't know things are invalid until we hit the catch block in the RenderSafeContainer element. I'm not sure at that point what we can do to recover, so that means we likely need to start doing input validation before preview generation and in case of failure fall back to the example value. Up until this point we've not wanted to do validation on preview for performance etc.

I'm not sure at that point what we can do to recover, so that means we likely need to start doing input validation before preview generation

It may be that these errors are only coming from \Drupal\Core\Template\ComponentsTwigExtension::validateProps in which case that shouldn't happen in production when `assert` should be disabled.

We might need to swap in our own validator on preview routes to prevent it during development and tests where assert is likely enabled

Per @larowlan at #3537154-24: Regression in #3529788: can publish component instances with empty required props → , the data integrity issue this issue originally caused is fixed 👍

@larowlan pointed out in chat that actually minLength: 0 (or the absence of minLength) as I did in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... is inadequate — we should still require strings using format or pattern to be valid, and not attempt to update the preview. IOW: only "prose"-esque strings. Just did that: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

In response to #23 and #24 I think as a minimum we should be displaying an error banner and retaining HTML5 validation errors on the form using the existing styling.

+1 — but can we do that in a follow-up issue instead? 🙏

And can we let the follow-up MR in this issue be just about the tightening of what the original MR of this issue (!1305) did, to truly only handle "required prose strings should be able to be empty and still update the preview to match"? 🙏

You're right that the scenario you described in #29 currently fails. That's exactly what was happening in HEAD for empty required strings, which @lauriii wanted to improve for usability, which is what this issue did. What your STR+screenshot shows is precisely what @bnjmnm, @effulgentsia and proposed a direction for in 📌 Invalid prop errors should be detailed in Component preview Active , and it relates to the known missing designs → outlined in 🌱 [META] Robust component instance error handling Active .

Re #30 - I've confirmed that the error only occurs if you have zend.assertions set to 1

With it set to -1 (even with the deprecated ini_set('assert.active', 1); in experience_builder.module) there is no error.

That's because \Drupal\Core\Template\ComponentsTwigExtension::validateProps only runs inside an assert function.

So that might also be something we should consider in 📌 Invalid prop errors should be detailed in Component preview Active

With that in mind and because 🐛 UI change in 3529788 allows publishing components with empty required fields Active is in - I am going to mark this back as fixed and open follow-up issues for the two follow-up MRs here as well as comment w.r.t zend.assertions on 📌 Invalid prop errors should be detailed in Component preview Active

Screenshot for #32 showing preview still displays with invalid data in link field

Added #3530795-6: [needs design] Invalid prop errors should be detailed in Component preview; otherwise the preview LIES 😱 →
Added 📌 Narrow the conditions under which we allow a prop to be an empty string Active and moved the MR over there
Added 📌 Lift HTML5 validation errors into the formState slice Active and moved the MR from 🐛 UI change in 3529788 allows publishing components with empty required fields Active there

larowlan → closed merge request !1345

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

Followed through on this because this caused some chaos in the days prior to my PTO last week:

• landed 📌 Narrow the conditions under which we allow a prop to be an empty string Active
• responded at #3530795-8: [needs design] Invalid prop errors should be detailed in Component preview; otherwise the preview LIES 😱 → , asking @larowlan to clarify what he proposed
• RTBC'd 📌 Lift HTML5 validation errors into the formState slice Active

All good now 👍