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

Created on 23 August 2024, 3 months ago
Updated 20 September 2024, about 2 months ago

Overview

We would like to ensure a few things

  • 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 it already works without crashing, but tests should be put into place to confirm it.
  • enum/select fields do not have a "None" option since it needs to be set to something
  • Tests for this already existin in 0.x see prop-type.cy.js 'Enum (select)'
  • Required props should have the required attribute in their corresponding props form input.
  • Form input of required props should be validated in the client — to ensure a value is provided —, updating preview should be prevented if validation fails.

e2e tests should be added for the above and code changes as needed to make things work as described.

Note that the suggestion in #12 regarding a "locked" UI is not in the scope of this issue.

Proposed resolution

See #4 + #5 + #6.

User interface changes

No more crash as seen in the above GIF :) (And no more "None" option in a <select> for a enum: […] prop shape).

🐛 Bug report
Status

Fixed

Component

Page builder

Created by

🇫🇮Finland lauriii Finland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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);
    
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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.)

  • 🇳🇱Netherlands balintbrews Amsterdam, NL

    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?

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
    1. The requiredness is already present in the API response for /xb-components:

      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? 🙏

  • 🇳🇱Netherlands balintbrews Amsterdam, NL
    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 📌 Component props form: map textarea, bool, and select elements to React components Fixed , so I think this should happen in React.
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    #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? 🙏

    ?

  • 🇳🇱Netherlands balintbrews Amsterdam, NL

    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! 😊

  • Assigned to soaratul
  • 🇳🇱Netherlands balintbrews Amsterdam, NL

    @soaratul, would you mind getting started with this?

  • 🇺🇸United States tedbow Ithaca, NY, USA

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

  • First commit to issue fork.
  • 🇬🇧United Kingdom jessebaker

    @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.
  • 🇺🇸United States traviscarden

    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.

  • Issue was unassigned.
  • Status changed to Needs work 2 months ago
  • 🇮🇳India soaratul

    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.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

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

  • Assigned to soaratul
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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.

  • 🇮🇳India soaratul

    @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).

  • Assigned to traviscarden
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Reported again: 🐛 Selecting "none" option for one of Heading SDC's s crashes the app Closed: duplicate . Crediting @bhuvaneshwar, the reporter.

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

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇺🇸United States traviscarden

    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.

  • 🇺🇸United States traviscarden

    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. 🙂

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

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

  • Assigned to soaratul
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    @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 🤞

  • First commit to issue fork.
  • 🇬🇧United Kingdom longwave UK

    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.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

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

  • First commit to issue fork.
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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.

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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.

    ______________________________

    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.

  • Assigned to bnjmnm
  • Status changed to Needs review about 2 months ago
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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 `` you said you had solved that already. But … then earlier yesterday, we merged 🐛 XBEndpointRenderer & processResponseAssets() do not support `ajaxPageState` ⇒ duplicate CSS/JS loading Fixed , 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? 🙏

  • Issue was unassigned.
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    What was implemented for enum + _none is as-discussed and continues to work that way. Selecting _none in a select does removes field from the model. The recent refactoring still effectively does this, but instead of looking for _none to determine model-exclusion, it will respond to any value not defined by the enum This is the e2e that confirms selecting _none works.

    That approach does not work for a required enum prop, because it is fully removing something required from the model . I think the best solution here is to disallow _none entirely on required select fields - if it's required then _none isn't a valid option and shouldn't be presented to them anyway.

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

    There is logic in the current MR that adds the required attribute to required fields, that's good! In the case of enum/select + required I think it's best to prevent invalid options from being available so we don't have to rely on FE catching it or have users wondering why a select offers an invalid option.

  • Assigned to soaratul
  • Status changed to Needs work about 2 months ago
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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

  • Issue was unassigned.
  • 🇺🇸United States bnjmnm Ann Arbor, MI
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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 🙈

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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

  • 🇺🇸United States bnjmnm Ann Arbor, MI
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Thanks, that helps!

    I think #37 says the only thing missing here is test coverage for an 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?

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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.

  • Assigned to soaratul
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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? 🙏😊

  • Issue was unassigned.
  • Status changed to RTBC about 2 months ago
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Yay! :D

  • Pipeline finished with Skipped
    about 2 months ago
    #287310
  • Assigned to bnjmnm
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

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

  • Issue was unassigned.
  • Status changed to Needs work about 2 months ago
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    The second requirement

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

    does not have a test

  • 🇬🇧United Kingdom longwave UK

    Fixing attribution.

  • Assigned to soaratul
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    For #44 and other feedback.

  • Issue was unassigned.
  • Status changed to Needs review about 2 months ago
  • Status changed to Needs work about 2 months ago
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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:

  • Assigned to longwave
  • 🇬🇧United Kingdom longwave UK

    Will dig into this again today.

  • Issue was unassigned.
  • Status changed to Needs review about 2 months ago
  • 🇬🇧United Kingdom longwave UK

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

  • Assigned to balintbrews
  • 🇳🇱Netherlands balintbrews Amsterdam, NL
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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 🐛 Premature prop validation can break the UI Active I am working on getting the additional constraints such as minLength enforced.

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    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 in did not require clientside validation

    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, before additional constraints like format and length were rejected server side

  • 🇳🇱Netherlands balintbrews Amsterdam, NL
  • Pipeline finished with Skipped
    about 2 months ago
    #288261
  • Issue was unassigned.
  • Status changed to Fixed about 2 months ago
  • 🇳🇱Netherlands balintbrews Amsterdam, NL
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024