- Issue created by @lauriii
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Reproduced.
Proposal:
- use the HTML
required
attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/required - that allows trivial client-side validation
- 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);
- use the HTML
- 🇧🇪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:
- 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 norequired
. Am I missing that this is not missing? 😊 - 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. - 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 aref
attached to the form, we can useHTMLFormElement::reportValidity()
to check for that. - Now that model updates are prevented, we also need to visually highlight the problematic form items.
- 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.
- 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?
- 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
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- The requiredness is already present in the API response for
/xb-components
:Aha, point 2 covers that.
- Aha, ok! WFM 👍
ComponentPropsForm::buildForm()
can figure that out. 👍 - That's exactly what I was referring to in #2.2 😄
- Indeed. Can we use https://www.drupal.org/docs/8/core/modules/inline-form-errors/inline-for... → ? Do you want to use something else?
- WFM 👍
- 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 thetest_REQUIRED_string
prop on theall-props
test component? 🙏 - The requiredness is already present in the API response for
- 🇳🇱Netherlands balintbrews Amsterdam, NL
- 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. 😊 - Great! 👍🏻
- 👍🏻
- 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.
- Right, I meant
- 🇳🇱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:
- Open page
- Click first header
- Validate that it has a required attribute
- Remove the value from the field
- Ensure the preview does NOT change
- With the value still blank (invalid) ensure further interaction is "locked" e.g...
- - ensure you can't select a different component
- - ensure you can't drag a new component on
- - ensure you can't rearrange components
- - ensure you can't delete a component
- Put a new value in
- Ensure the new value shows in the preview
- 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.
- Merge request !295#3469855: Emptying a required value through the UI crashes the app (empty <input>, selecting "None" option in <select> …) → (Merged) created by soaratul
- Issue was unassigned.
- Status changed to Needs work
2 months ago 1:12pm 11 September 2024 - 🇮🇳India soaratul
I wrote all test cases suggested by @jessebaker and team based on assumption only - like we will not allow to...
- Select other component if one is being edited and it's required field is empty
- Drag other component
- Rearrange components
- Delete a component
- 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 🇧🇪🇪🇺
- 🇮🇳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 #15Also: 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)
- 🇺🇸United States traviscarden
Update: I'm on track with a solution, but there's an ugly merge conflict with
0.x
inxb-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.
- 🇧🇪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 crashI'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
2 months ago 1:18pm 18 September 2024 - 🇧🇪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 forenum: […]
prop shapes (write-up at #3471511-13: `enum` data shapes: error when choosing "- None -" in ` → ). I'll repeat the conclusion here for clarity:Conclusion
Select.tsx
, introduced in 🐛 Prop select lists don't affect the component Fixed , needs to be updated to be aware of this special value. If this special value is selected, then:- the UI should interpret this as "no input specified by user"
- the UI should then delete this prop from the model, rather than setting
"_none"
as the value — because that's the whole point of ✨ Contextual form values need to be integrated with Redux Active : to allow real-time updates of the preview based on information on the client side only - 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 ` → 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
2 months ago 2:17pm 18 September 2024 - 🇺🇸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.
- 🇧🇪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 🙈
- 🇧🇪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
2 months ago 2:06pm 19 September 2024 - Assigned to bnjmnm
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@bnjmnm or @hooroomoo, needs approval from you :)
- Issue was unassigned.
- Status changed to Needs work
2 months ago 2:14pm 19 September 2024 - 🇺🇸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
- Assigned to soaratul
- Issue was unassigned.
- Status changed to Needs review
2 months ago 4:38pm 19 September 2024 - Status changed to Needs work
2 months ago 7:37am 20 September 2024 - 🇧🇪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
- Issue was unassigned.
- Status changed to Needs review
2 months ago 11:22am 20 September 2024 - 🇬🇧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
- 🇺🇸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 -
balintbrews →
committed c216262a on 0.x authored by
soaratul →
Issue #3469855 by soaratul, longwave, wim leers, traviscarden,...
-
balintbrews →
committed c216262a on 0.x authored by
soaratul →
- Issue was unassigned.
- Status changed to Fixed
2 months ago 1:29pm 20 September 2024 Automatically closed - issue fixed for 2 weeks with no activity.