Emptying a required value through the UI crashes the app (empty <input>, selecting "None" option in <select> …)

Reproduced.

Proposal:

1. use the HTML required attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/required
2. that allows trivial client-side validation
3. if that very basic validation does not pass, do not update the preview.

That requires first ensuring that the form rendered for a StaticPropSource is told whether it is required or not when generating the form. Starting point (to just make every field widget required):

diff --git a/src/PropSource/StaticPropSource.php b/src/PropSource/StaticPropSource.php
index e7359427..db6dd5c2 100644
--- a/src/PropSource/StaticPropSource.php
+++ b/src/PropSource/StaticPropSource.php
@@ -293,6 +293,7 @@ final class StaticPropSource extends PropSourceBase {
     // @see \Drupal\Core\Field\FieldTypePluginManager::createFieldItem()
     // @see \Drupal\Core\TypedData\TypedDataManagerInterface::getPropertyInstance()
     $list_class = $this->fieldItem->getFieldDefinition()->getClass();
+    $this->fieldItem->getFieldDefinition()->setRequired(TRUE);
     $field = (new $list_class($this->fieldItem->getFieldDefinition(), $sdc_prop_name, NULL));
     assert($field instanceof FieldItemListInterface);
     $field->set(0, $this->fieldItem);

Disregard #2 — we could do that, but it actually would likely lead to an inferior experience: the required boolean attribute could then appear in multiple places in the widget. It is likely more robust, and simpler, to implement this purely on the client side.

(Although accessibility considerations may end up pushing us in a different direction still, eventually.)

We discussed this today with @jessebaker, this is what we think could be a good approach here:

1. First we need to make sure we can determine which form items are required. Is it fair to assume this meta info should be included in the form markup our backend route renders? It is currently not included. There are all kinds of Drupal Form API attributes, like placeholder, maxlength, size etc., but no required. Am I missing that this is not missing? 😊
2. Assuming we know which form items are required, it's probably a good idea to have that represented as an HTML required attribute as suggested in #2.
3. Once we have required attributes at the right places, we need to extend the code where we update the model on form element changes —which then triggers the preview update—, and prevent the model update when there are missing required values. Assuming we have a ref attached to the form, we can use HTMLFormElement::reportValidity() to check for that.
4. Now that model updates are prevented, we also need to visually highlight the problematic form items.
5. At the same time, we need to disallow selecting another component on the preview. In addition, for practical reasons, I would suggest we also prevent any action that would update the model or the layout. This includes re-arranging components and adding new components/sections.
6. Visual highlights and blocking certain actions on the UI should be cleared when errors are resolved in the component props form.

@wim leers — what are your thoughts?

1. The requiredness is already present in the API response for /xb-components:

   Having the required attribute be generated on the server side is exactly what I proposed in #2, but which @lauriii told me you said you wouldn't need. I'm confused now 🤪😅 Aha, point 2 covers that.

2. Aha, ok! WFM 👍 ComponentPropsForm::buildForm() can figure that out. 👍
3. That's exactly what I was referring to in #2.2 😄
4. Indeed. Can we use https://www.drupal.org/docs/8/core/modules/inline-form-errors/inline-for...? Do you want to use something else?
5. WFM 👍
6. WFM 👍

IOW: I'm on board. 😄

Can we get started with a E2E test that fails on failing to find the required HTML attribute of the test_REQUIRED_string prop on the all-props test component? 🙏

1. Right, I meant /xb-field-form/{entity_type}/{entity} so we can access it at the one place we reach to render the form. I may have previously said that this is mostly a frontend issue, but I had to realize we still need to know what is supposed to be required. 😊
2. Great! 👍🏻
3. 👍🏻
4. We're mapping the generated form markup to our own React components in #3462310: Component props form: map textarea, bool, and select elements to React components, so I think this should happen in React.

#6.4 WFM!

Do you agree with

Can we get started with a E2E test that fails on failing to find the required HTML attribute of the test_REQUIRED_string prop on the all-props test component? 🙏

?

Sure, that sounds like a great idea! Someone can get started on it, and once we have that, we can even move on to testing #4.5. TDD FTW! 😊

@soaratul, would you mind getting started with this?

Won't assign to myself but I can start working on the setting the #required from the server side.

@soaratul Test in pseudo code as discussed:

1. Open page
2. Click first header
3. Validate that it has a required attribute
4. Remove the value from the field
5. Ensure the preview does NOT change
6. With the value still blank (invalid) ensure further interaction is "locked" e.g...
   1. - ensure you can't select a different component
   2. - ensure you can't drag a new component on
   3. - ensure you can't rearrange components
   4. - ensure you can't delete a component
7. Put a new value in
8. Ensure the new value shows in the preview
9. Ensure you now CAN select a different component, drag, rearrange, delete etc.

Leaving this assigned to @soaratul, but I'm going to try to pick up where @tedbow left off with the backend part of the work.

I wrote all test cases suggested by @jessebaker and team based on assumption only - like we will not allow to...

1. Select other component if one is being edited and it's required field is empty
2. Drag other component
3. Rearrange components
4. Delete a component
5. etc..

I hope we would be doing all these checks and implementation from FE side once after fixing the BE issue to pass all the test cases we wrote with assumption.

I have written all test cases based on assumption only so might be possible that it may require some changes based on actual implementation.

#3473155: Redux Sync all single-value types in the SDC test all props form is in, which means AFAICT you can continue here, @soaratul! 😄

What's the status here, @soaratul? AFAICT you finished the test yesterday, does that mean you think that #7 is now done and we can move on to the subsequent steps outlined in #5?

Also: the MR conflicts with HEAD, so it needs a reroll.

@wim-leers Yes exactly
- #7 has been done with assumption
- We can now move to the subsequent in #5, along with what I've mentioned in #15

Also: I will be resolving the conflicts while verifying the test cases - and test cases can be verified only after completing all the task mentioned in #15 e.g. stop dragging, selecting, rearranging, deleting etc.. while editing component(empty required field value).

Reported again: #3473782: Selecting "none" option for a required <select>s crashes the app. Crediting @bhuvaneshwar, the reporter.

#18: 👍 — then assigning to @traviscarden, who started working on this yesterday (see #13)

Update: I'm on track with a solution, but there's an ugly merge conflict with 0.x in xb-general.cy.js that I'm trying to rebase away.

I pushed some code that seems to work locally. I can't run Cypress tests right now for some reason; we'll see what happens on CI. The code probably isn't very elegant, but red => green => refactor, you know. 🙂

The back-end changes LGTM — now we just need to get the E2E test to pass again 😇

@traviscarden won't be working for another several hours, while @soaratul is in the middle of his day! Plus, he wrote the E2E test, so is better suited to update it 🤞

Also, the next step here is #4.3's HTMLFormElement::reportValidity(), which is purely front-end, so out of @traviscarden's wheelhouse.

I implemented #4.3 - the app no longer gets stuck in a loop if you empty a required field - but have no idea where to start on the rest of #4 so leaving this again for now.

Ben posted detailed feedback on the test coverage. The test coverage needs work to improve its reliability. Could you address that feedback, @soaratul? 🙏

The e2e tests (and I assume most/all manual ones) were being performed on the header field in the hero component, which has a minimum length requirement in addition to being required.

A quick check of the logs makes this evident

  Drupal\Core\Render\Component\Exception\InvalidComponentException: [heading] Must be at least 2 characters long in  
                                        Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of                                       
                                        /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php). 

That min length requirement is not important, so it was removed by me (then again by Wim when it was overwritten) and the tests are now testing an input where required is the only thing being checked against.

The issue as reported isn't a problem.
The My Hero header prop is required and has this property set: minLength: 2
If I remove the minLength configuration, it handles the empty required field without a crash

I'm sure required fields could offer a better experience, but there's no issue with crashing - it's an entirely different set of prop requirements causing the crash. Min length will also need to be supported but it's unsure if this issue is the home for it.

______________________________

The IS also mentions enum. Required enum fields will crash if _none is chosen so it may be best to make that option unavailable before it reaches the front end.

__________________

Based on the new info I'm not sure how much existing work can be re-used. I'll leave that to the folks who actively worked on the branch. The issue summary should be updated to this more accurate info + the intended approach to fixing it.

so it may be best to make that option unavailable before it reaches the front end.

That's what @traviscarden worked on. He did that only for required props, which is what this issue was prescribing.

I previously investigated the _none case for enum: […] prop shapes (write-up at #3471511-13: `enum` data shapes: error when choosing "- None -" in `<select>`). I'll repeat the conclusion here for clarity:

Conclusion

Select.tsx, introduced in #3471083: Prop select lists don't affect the component, needs to be updated to be aware of this special value. If this special value is selected, then:

1. the UI should interpret this as "no input specified by user"
2. the UI should then delete this prop from the model, rather than setting "_none" as the value — because that's the whole point of #3462441: Contextual form values need to be integrated with Redux: start with single-property field types: to allow real-time updates of the preview based on information on the client side only
3. that will then prevent this invalid model to be sent from the client to the server:

@bnjmnm: over at #3471511-15: `enum` data shapes: error when choosing "- None -" in `<select>` you said you had solved that already. But … then earlier yesterday, we merged #3472900: XBEndpointRenderer & processResponseAssets() do not support `ajaxPageState` ⇒ duplicate CSS/JS loading, which changed the _none handling 😅 I suspect that's where there probably is a mismatch? 🤓 What is your updated assessment/recommendation compared to your comment #15 at #3471511? 🙏

What was implemented for enum + _none in #3471511 is as-discussed and continues to work that way. Selecting _none in a select does remove the field from the model. The recent refactoring still effectively does this, but instead of looking for _none to determine model-exclusion, it will exclude any value not defined by the enum, not just _none. There's e2e testing that confirms selecting _none works without issue.

It's a different story with REQUIRED selects, but fortunately....

That's what @traviscarden worked on. He did that only for required props, which is what this issue was prescribing.

Ah! the logic in the MR of making it $required server side eliminates the _none - I did not see anything in the code specifically removing _none and forgot about all the excellent stuff form api does behind the scenes. The assessment in my earlier comment was based on HEAD and I should have switched branches instead of just looking at the MR before issuing my findings. 💀Apologies for the derail💀.

+++++++++++++++++++++

So there is in fact a working fix for required enums that needs e2e tests. Probably all that is needed is confirming there's no _none option in a required select.

In both 0.x and the MR, a required field with no other schema restraints will not crash when emptied - all the crashes we'd been running into are because there was a minLength requirement not being met.

In addition to it not crashing #31 demonstrates that a required field can be emptied and the preview will update with that empty value. Perhaps we should be doing something in real time about this, or perhaps it's OK to just prevent saving, which the already-added required attributes would satisfy.

Item 6 in comment #12 - "locking" the UI already has tests written for it in anticipation of its implementation but this specific functionality seems like it would be best to do in a separate issue as there's no (AFAIK) existing UI-lock functionality and there are probably some design discussions needed to determine how this locked UI would be conveyed to the user - or maybe it's simpler than I'm hunching. If it seems worth adding to this issue, lets get it in the IS

So there is in fact a working fix for required enums that needs e2e tests. Probably all that is needed is confirming there's no _none option in a required select.

This suggests test coverage is missing?

But then #34 refers to item 6 in #12, suggesting that the "locking" test coverage should be removed?

(I agree that the "locking" bits should be left for a later issue, because they require design, whereas this is a purely functional fix.)

I'm sorry, but it's not clear to me yet what the next step is that you recommend 🙈

My walls of text made it tough to navigate

#33 ends with me saying we need a test here that confirms the MR successfully filters _none out of required enums - sounds like you agree

Thanks, that helps!

I think #37 says the only thing missing here is test coverage for an optional enum/select field.

My confusion was: "but E2E tests are failing, so how can that then be the only thing still needed?

Turns out @soaratul introduced a subtle whitespace-only change that caused an even lower-level failure!

The error was:
Error: Webpack Compilation Error
Module build failed (from ./node_modules/babel-loader/lib/index.js):
SyntaxError: /builds/project/experience_builder/tests/src/Cypress/cypress/e2e/xb-general.cy.js: Unexpected character '️'. (635:4)
  633 |     // Remove the value from field
  634 |     cy.get('input[data-drupal-selector="edit-xb-component-props-static-static-card1ab-heading-0-value"]').clear();
> 635 |     ️

I just removed that, but it's still failing tests. So these are the genuine test bugs that @soaratul should still fix, right, @bnjmnm?

There are already tests in 0.x for optional enum/select that include selecting none. They are in prop-type.cy.js and labeled 'Enum (select)'

Beyond the whitespace failure, e2e tests are also failing because the new tests include checking for the "locked" feature suggested in #12, a feature that is not happening in this issue.

Riiiiiiiiiight! Thanks for clearing up all the confusion on my part! 🙏🙈

@soaratul, can you reduce what the tests test to the scope outlined in the issue summary that @bnjmnm updated? 🙏😊

Yay! :D

@bnjmnm or @hooroomoo, needs approval from you :)

The second requirement

required enum/select fields do not have a "None" option since it needs to be set to something

does not have an e2e test

Fixing attribution.

For #44 and other feedback.

I was surprised to see @longwave remove the reportValidity() check, but then I found @bnjmnm's comment at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... recommending that. I don't understand why though.

Because I now see zero client-side changes that make the XB app respect the required attribute that now is present for required SDC props' field widgets.

I can still trigger a failure by emptying a required prop:

Will dig into this again today.

The fix is trivial but the test took me a bit longer - fixed after some rubber duck debugging with @balintbrews

The fix only applies to HTMLInputElement because as far as I know, and for the purpose of this issue, we only care about <input required> here.

As far as I can see this fix will *not* work for the case where e.g. minLength is set on a text field and the minimum length is not met, because that requirement is not set up in HTML validation and so we still send invalid data to the server, but to me this case is out of scope to fix here and can be deferred until later.

Re #50

As far as I can see this fix will *not* work for the case where e.g. minLength is set on a text field and the minimum length is not met, because that requirement is not set up in HTML validation and so we still send invalid data to the server, but to me this case is out of scope to fix here and can be deferred until later.

+1, that is why the issue summary specifies :

• emptying a required string field does not crash the app. Testing this must be on a field that has no additional validation constraints, just required. It is possible it already works without crashing, but tests should be put into place to confirm it.

(and easy to miss in a lengthy issue like this)

Enforcing required is more straightforward to implement than the other constraints, and last I checked the MR in this issue took care of that quite nicely.

In #3474732: Premature prop validation can break the UI I am working on getting the additional constraints such as minLength enforced.

I was surprised to see @longwave remove the reportValidity() check, but then I found @bnjmnm's comment at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... recommending that. I don't understand why though.

The validation would never be triggered, the condition would only be met if the target was HTMLFormElement but the target is never <form> so it wasn't doing anything. In an earlier version of this MR there was actual clientside validation happening - but adding theHTMLFormElement condition effectively shut it off.
I would have recommended a refactor to bring back the clientside validation, but the tests were passing and I could manually confirm required-as-only-constraint inputs were not crashing the app when empty. The specific requirements of the issue were met without having to add that.

Because I now see zero client-side changes that make the XB app respect the required attribute that now is present for required SDC props' field widgets.

I think clientside validation needs to happen and I have exactly this happening in #3474732, but the specific problems reported here did not require clientside validation to work.

I can still trigger a failure by emptying a required prop:

A required-only-constraint prop should not fail - the CTA1 field has additional format requirements causing the fail. This is being addressed in #3474732.

I also think it's fine to expand this issue's scope and include clientside validation, it might make it easier to review since the required enforcement would happen earlier, so it never reaches the server to deal with additional constraints like length and format.