#states cannot check/uncheck checkboxes elements

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

re-roll for 9.5.x

Adding Reroll for D10

This still needs (as tagged):

• Tests. Again, #2702233: [backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked → provided core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php which is where the new tests for this issue should go.
• An issue summary update

Sadly, patch #93 lost the changes to core/misc/states.es6.js and is invalid. We need to go back to at least #92.

Back to Needs work.

Fixing tag

It took me a little while to write a failing test here. 😅 If you set #states on the specific values of a checkboxes element, it works fine, even without the patch. E.g. this works fine, before/after the patch:

    $form['checkboxes_some_checked_when_checkbox_trigger_checked'] = [
      '#type' => 'checkboxes',
      '#title' => 'Checkboxes: some checked when checkbox trigger checked',
      '#options' => [
        'value1' => 'Value 1',
        'value2' => 'Value 2',
        'value3' => 'Value 3',
      ],
      'value1' => [
        '#states' => [
          'checked' => [
            ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
          ],
        ],
      ],
    ];

The only trouble is if you're trying to bulk check/uncheck all values of a checkboxes by setting #states on the parent layer of the form array, e.g. this:

    $form['checkboxes_all_checked_when_checkbox_trigger_checked'] = [
      '#type' => 'checkboxes',
      '#title' => 'Checkboxes: all checked when checkbox trigger checked',
      '#options' => [
        'value1' => 'Value 1',
        'value2' => 'Value 2',
        'value3' => 'Value 3',
      ],
      '#states' => [
        'checked' => [
          ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
        ],
      ],
    ];

From what JavascriptStatesTest can tell us, type radios works fine, both for selecting and enable/disable.

I have no idea what people expect to happen if you tell a whole set of radio buttons to be "checked" when another trigger is hit. This would make no sense:

    $form['radios_checked_when_checkbox_trigger_checked'] = [
      '#type' => 'radios',
      '#title' => 'Radios checked when checkbox trigger checked',
      '#options' => [
        'value1' => 'Value 1',
        'value2' => 'Value 2',
      ],
      '#states' => [
        // Which value is supposed to be checked now? WTF does this code expect? 😂
        'checked' => [
          ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
        ],
      ],
    ];

I believe you should have to say this, which works fine:

    $form['radios_checked_when_checkbox_trigger_checked'] = [
      '#type' => 'radios',
      '#title' => 'Radios checked when checkbox trigger checked',
      '#options' => [
        'value1' => 'Value 1',
        'value2' => 'Value 2',
      ],
      'value1' => [
        '#states' => [
          'checked' => [
            ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
          ],
        ],
      ],
    ];

Meanwhile, all of the variations of trying to enable/disable either specific checkboxes or radios, or the entire element, seems to work fine without the patch. I don't think the hunk in form.inc is needed. Updating the title and summary to clarify this is only really about fixing the bulk check/uncheck on checkboxes.

However, given all the confusion in this issue, and since I already spent the time to add all the different permutations of the test coverage, I think it's worth expanding the scope a bit here to ensure we're covering all these cases, even if only 1 of them is actually broken in core and solved by the rest of the patch. I hope the committers agree we don't need a follow-up to add the additional coverage of the currently-working permutations.

Some self-review to document what I've done and why:

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -180,6 +180,123 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['checkboxes_all_checked_when_checkbox_trigger_checked'] = [
   +      '#type' => 'checkboxes',
   +      '#title' => 'Checkboxes: all checked when checkbox trigger checked',
   +      '#options' => [
   +        'value1' => 'Value 1',
   +        'value2' => 'Value 2',
   +        'value3' => 'Value 3',
   +      ],
   +      '#states' => [
   +        'checked' => [
   +          ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
   +        ],
   +      ],
   +    ];
   

   This is the only new form element in this test that fails without the fix to states.js.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -8,6 +8,11 @@
   + * The form being tested is JavascriptStatesForm provided by the 'form_test'
   + * module under 'system' (core/modules/system/tests/module/form_test).
   + *
   + * @see Drupal\form_test\Form\JavascriptStatesForm
   + *
   

   This seems like really helpful documentation to enable other people to expand this test coverage, so I'm not the only one doing it in the future. 😅 Please don't object to the "scope creep". 🙏

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -94,6 +99,53 @@ protected function doCheckboxTriggerTests() {
   +    $radios_all_disabled_value1 = $this->xpath('//input[@name=:name][@value=:value]', [':name' => 'radios_all_disabled_when_checkbox_trigger_checked', ':value' => 'value1']);
   

   I couldn't figure out a way to get a specific radio button inside a set of radios using only findField(). If someone knows of a more slick / concise way to write this, please let me know.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -117,6 +187,30 @@ protected function doCheckboxTriggerTests() {
   +    $this->assertTrue($checkboxes_all_checked_element_value1->isChecked());
   +    $this->assertTrue($checkboxes_all_checked_element_value2->isChecked());
   +    $this->assertTrue($checkboxes_all_checked_element_value3->isChecked());
   

   Without the fix to states.js these are the only 3 new assertions that fail. If you comment these out, and revert the fix, everything else still passes. But given there's almost no cost to running the next 20 lines worth of assertions (no new page load needed, etc), I strongly vote we keep the rest of this.

Thanks!

The last submitted patch, 96: 994360-96.test-only.patch, failed testing. View results →

Beautiful work. The fixes look simple - in both the disabled and checked cases, we only want to set the property on the input itself and not on any parent wrapper; this brings the code that handles the two cases more inline with each other. The test coverage is also comprehensive, nice to get some additional test coverage on this functionality.

#97.2 I agree that this additional documentation is helpful and is fine to add in this patch.

#97.3 As far as I can see the only way here would be to use unique labels for each radio button, but that would make the form more complicated, so I think using XPath here is fine.

While reviewing I also noted this comment in states.js:

      // Note: WebKit nightlies don't reflect that change correctly.
      // See https://bugs.webkit.org/show_bug.cgi?id=23789

This bug was fixed in 2011! I will open a separate issue to remove this comment.

This bug got almost fixed 7 years ago but got reverted because of a regression 🤯 I double checked that the regression doesn't occur anymore, but looks like we don't have test coverage for that use case. Here's a new patch that adds test coverage for that.

The addition in #100 makes sense to me. I thought about extracting the initial state test to its own method so we don't have to copy-paste and keep the two parts in sync, but it would mean having way too many arguments for each of the fields that are tested, so I think this is OK as-is.

Committed fdb1fa9 and pushed to 10.1.x. Thanks!

Wohooooooooooooo!! Thank you all so much! Fixed after 13 years!! This makes me so happy to see.
Honestly, thank you!

O.M.G!
Thank you all for having the energy to push this to the end!

Re: #100: Thanks for the info and updated test coverage!

Re: #101: Yeah, I had the same inital "yuck" thought, but indeed, it'd be clumsy to pass all those elements to a private helper, so this is probably fine.

I asked in Slack, and @longwave and @lauriii agreed this was eligible for backport. The commit should cherry-pick cleanly to 10.0.x already, but in the 9.5.x branch, we're still using states.es6.js. So here's an updated patch.

Excited to see this make it!

Thanks! Updated the summary to include the current remaining tasks (to make it easier for whichever committer sees this in the RTBC queue)...

@dww Thanks for the clear instructions.

Cherry-picked fdb1fa9ff0 to 10.0.x and committed #106 to 9.5.x. Thanks everyone!

Yay, thanks!

If possible please help this issue: 🐛 ['#states']['required'] broken for radios Needs work
Thanks!

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

Changing
$(e.target).prop('disabled', e.value).closest('.js-form-item, .js-form-submit, .js-form-wrapper').toggleClass('form-disabled', e.value).find('select, input, textarea').prop('disabled', e.value);

to

$(e.target).closest('.js-form-item, .js-form-submit, .js-form-wrapper').toggleClass('form-disabled', e.value).find('select, input, textarea').prop('disabled', e.value);

has broken disabling items if there is no `.js-form-wrapper` around an element.

Previously, the code worked because the element itself was targeted first. Now, the element is found and then it's children are searched for select, input, textarea. It would worked if the found element was .js-form-wrapper. But there could be cases when .js-form-wrapper could be missing.

Not sure why we are targeting different levels of selectors here - the form item and form wrapper are on different levels in the hierarchy and should not be treated in the same way (same selector query).

Like #115 I'm seeing an issue with the same change. We have a button element inside a form and it has the states settings and the js-form-submit class. Before its disabled status was being controlled due to that first `.prop('disabled', e.value)`, and not it's not working.

Probably needs to have a new issue for that, right?

Find a small workaround, you need to change the HTML structure of your button/input submit (done on 9.5.7) .

1) Remove the .js-form-submit (example here with twig)

<input{{ attributes.removeClass('js-form-submit') }} />{{ children }}

2) Add .js-form-item around your input

    $form['actions']['submit'] = [
      '#type' => 'submit',
      '#value' => t('Validate'),
      '#enabled' => FALSE,
      '#prefix' => '<div class="js-form-item">',
      '#suffix' => '</div>',
    ];

    $form['actions']['submit']['#states'] = [
      'enabled' => [
        [
          ':input[name="other_input"]' => ['empty' => FALSE],
        ],
      ],
    ];

Thanks mt-i-1. I'll go with a container element wrapped around which has the same effect. And I'll stop spamming this closed issue.

I've opened 🐛 #states disable property has stopped working for some element types Needs work as a followup with a MR to update the element types that get the disabled property set.