Hidden Flexbox layouts with required sub-elements throw errors when they shouldn't (because the required elements are hidden). The following YAML form code reproduces the issue.

checkbox:
  '#type': checkbox
  '#title': 'Checkbox (leave unchecked and submit the form)'
flexbox:
  '#type': flexbox
  '#states':
    visible:
      ':input[name="checkbox"]':
        checked: true
  required_text:
    '#type': textfield
    '#title': 'Required Text'
    '#required': true
  text:
    '#type': textfield
    '#title': Text
CommentFileSizeAuthor
#34 2946681-33.patch14.09 KBjrockowitz
#34 interdiff-2946681-22-33.txt7.35 KBjrockowitz
#22 2946681-22.patch6.65 KBjrockowitz
#18 2946681-18.patch5.67 KBjrockowitz
#16 2946681-16.patch0 bytesjrockowitz
#6 2946681-6.patch8.44 KBjrockowitz
#4 2946681-4.patch6.34 KBjrockowitz
#3 2946681-3.patch1.12 KBjrockowitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nodecode created an issue. See original summary.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Priority: Normal » Major

The issue is that the flex container does not fully support #states and is using a custom #states wrapper.

@see \Drupal\webform\Plugin\WebformElementBase::prepareWrapper
@see \Drupal\webform\Utility\WebformElementHelper::fixStatesWrapper

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Active » Needs review
FileSize
2946681-3.patch1.12 KB

The attached patch is adding the missing .js-form-wrapper to the flexbox container and remove the states wrapper workaround.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
2946681-4.patch6.34 KB

The attached patch is also trying to remove the states wrapper from composite elements.

Status: Needs review » Needs work

The last submitted patch, 4: 2946681-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
2946681-6.patch8.44 KB

The attached patch still has a broken test but is a better long-term solution.

Status: Needs review » Needs work

The last submitted patch, 6: 2946681-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • jrockowitz committed 1ab1e1a on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...

  • jrockowitz committed b1fca3d on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...

  • jrockowitz committed f33527d on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...

  • jrockowitz committed 70a550b on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...
nodecode’s picture

nodecode CreditAttribution: nodecode commented

After seeing the commit, updated to the latest dev and it hasn't changed anything.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

The commits are to a feature branch called '2946681-sub-element-validation'

nodecode’s picture

nodecode CreditAttribution: nodecode commented

Gotcha. I'll sit tight until it's ready for testing.

  • jrockowitz committed d407f9e on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
2946681-16.patch0 bytes

I am not sure this going to be the final solution but I want to get this patch tested.

Status: Needs review » Needs work

The last submitted patch, 16: 2946681-16.patch, failed testing. View results

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
2946681-18.patch5.67 KB

Status: Needs review » Needs work

The last submitted patch, 18: 2946681-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nodecode’s picture

nodecode CreditAttribution: nodecode commented

Ok, I'm ready to test as soon as it passes muster with the Testing module

  • jrockowitz committed 11a6eea on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
2946681-22.patch6.65 KB
nodecode’s picture

nodecode CreditAttribution: nodecode commented

It's a solid start as it fixes the original example, but the #states do not iterate into the Flexbox's sub-elements if they're dependent upon each other. I was able to break it again with the following simple example:

checkbox:
  '#type': checkbox
  '#title': 'Checkbox (leave unchecked and submit the form)'
flexbox:
  '#type': flexbox
  '#states':
    visible:
      ':input[name="checkbox"]':
        checked: true
  radios:
    '#type': radios
    '#title': Radios
    '#options':
      'No': 'No'
      'Yes': 'Yes'
    '#required': true
  text_field:
    '#type': textfield
    '#title': 'Text field'
    '#required': true
    '#states':
      visible:
        ':input[name="radios"]':
          '!value': 'No'
nodecode’s picture

nodecode CreditAttribution: nodecode commented
Status: Needs review » Needs work
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

Your example is having the flexbox #states iterate to the radios and then iterate to the text field. I don't think this can be supported. I will look into the problem a little more. The patch does fix the original example.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

I think have to explicitly code the conditions.

checkbox:
  '#type': checkbox
  '#title': 'Checkbox (leave unchecked and submit the form)'
flexbox:
  '#type': flexbox
  '#states':
    visible:
      ':input[name="checkbox"]':
        checked: true
  radios:
    '#type': radios
    '#title': Radios
    '#options':
      'No': 'No'
      'Yes': 'Yes'
    '#states':
      required:
        ':input[name="checkbox"]':
          '': ''
  text_field:
    '#type': textfield
    '#title': 'Text field'
    '#states':
      visible:
        ':input[name="radios"]':
          value: 'Yes'
      required:
        ':input[name="radios"]':
          value: 'Yes'
nodecode’s picture

nodecode CreditAttribution: nodecode commented

I guess I'm fundamentally misunderstanding the problem, but if the accepted logic is such that a required element inside a Flexbox should no longer be required when that Flexbox is hidden... is it not logical/possible for ALL sub-elements of that Flexbox to have their "required" property overridden, regardless of additional states logic within the flexbox? It's not even a matter of iteration in this case, just the blanket application of a property setting.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

The visible #states for the 'text_field' is taking precedences over parent flexbox element states.

text_field:
   '#required': true
    '#states':
      visible:
        ':input[name="radios"]':
          '!value': 'No'

We are not cascading the #states from parent element to a child element, when the child element is defining its own #states.

  • jrockowitz committed ecad7b4 on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...
nodecode’s picture

nodecode CreditAttribution: nodecode commented

I always appreciate your patient explanations. I'm sure this behavior choice was made for good reason that I don't fully grasp at the moment. In this isolated case I don't see the logic, but if that's how Webform works best, then my testing shows the issue is fixed (with the workaround for more complex situations).

My concern is simply that a hidden field is throwing an error message and issues like this may continue to bog down the issue queue in the future as adoption of the module increases.

Thank you!

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

Cascading conditionals is really hard to manage and test. I am open to any help improving the Webforms conditional logic. Keep in mind that Webform is leveraging Drupal's #states API which can also be improved.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Looks propagation works fine

+++ b/src/Plugin/WebformElementBase.php
@@ -877,6 +905,11 @@ class WebformElementBase extends PluginBase implements WebformElementInterface {
+    if (isset($element['#states'])) {
+      $element['#element']['#_webform_states'] = $element['#states'];

better use !empty() here, IMO

  • jrockowitz committed 4abf494 on 2946681-sub-element-validation 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
interdiff-2946681-22-33.txt7.35 KB
2946681-33.patch14.09 KB

@andypost Thanks for the suggestion. The attached patch uses !empty() and removes some duplicate code.

  • jrockowitz committed e49ad6d on 8.x-5.x 
    Issue #2946681 by jrockowitz: Sub-elements of hidden Flexbox shouldn't...
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs review » Fixed
nodecode’s picture

nodecode CreditAttribution: nodecode commented

Just to be clear for anyone who's wondering in the future. This patch does fix the original issue but does not fix the example in #23 unless you use the workaround in #26.

Status: Fixed » Closed (fixed)

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