[DX] Form API: Adding #attributes to fields of type container breaks #states

[klonos]

This is a sibling issue to #3817...

It is a known issue that '#markup' form elements do not support #states. So this won't work:

    $form['controller'] = array(
      '#type' => 'checkbox',
      '#title' => t('Show/hide markup element'),
    );

    $form['warning'] = array(
      '#markup' => t('blah blah...'),
      '#states' => array(
        'visible' => array(
          ':input[name="controller"]' => array('checked' => TRUE),
        ),
      ),
    );

The workaround is to add a '#type' => 'container' around them, and use #states on the container form element instead of directly on the markup element.

This works fine:

    $form['controller'] = array(
      '#type' => 'checkbox',
      '#title' => t('Show/hide container'),
    );

    $form['some_warning'] = array(
      '#type' => 'container',
      '#states' => array(
        'visible' => array(
          ':input[name="controller"]' => array('checked' => TRUE),
        ),
      ),
    );
    $form['some_warning']['message'] = array(
      '#markup' => t('blah blah...'),
    );

This issue here is not about solving that problem...

Everything works as expected with the above-mentioned workaround, until you try adding any class to the container form element (via #attributes).

This does NOT work:

    $form['controller'] = array(
      '#type' => 'checkbox',
      '#title' => t('Show/hide container'),
    );

    $form['some_warning'] = array(
      '#type' => 'container',
      '#attributes' => array('class' => array('messages', 'warning')), // Adding this breaks #states.
      '#states' => array(
        'visible' => array(
          ':input[name="controller"]' => array('checked' => TRUE),
        ),
      ),
    );
    $form['some_warning']['message'] = array(
      '#markup' => t('blah blah...'),
    );

To make it work again, one needs to manually add the form-wrapper class to the container element:

    $form['controller'] = array(
      '#type' => 'checkbox',
      '#title' => t('Show/hide container'),
    );

    $form['some_warning'] = array(
      '#type' => 'container',
      // The "form-wrapper" class is required here, otherwise #states doesn't
      // work.
      '#attributes' => array('class' => array('form-wrapper', 'messages', 'warning')),
      '#states' => array(
        'visible' => array(
          ':input[name="controller"]' => array('checked' => TRUE),
        ),
      ),
    );
    $form['some_warning']['message'] = array(
      '#markup' => t('blah blah...'),
    );

[klonos]

Simple PR up for review: backdrop/backdrop#3689 ...makes sure that any #attributes set in the FAPI container element are not being overwritten in theme_container() + always adds the form-wrapper class to the <div> (which is what makes #states work).

Should be easy to test this on your local:

• set up your local with a fresh, vanilla Backdrop instance
• pick any random form, like system_site_maintenance_mode() in core/modules/system/system.admin.inc for example.
• add the snippets of code I provide in the issue summary, one-by-one
• verify that they either work or don't work as expected, as per issue summary
• patch your local with the changes from Issue #5157: Fix #states when adding #attributes to container FAPI elements backdrop#3689
• test the 3rd snippet provided in the issue summary
• confirm that the snippet works as expected now, and report back here

Thanks

[klonos]

@laryn mentioned on Zulip:

@klonos We should make sure the fix doesn't "unfix" the issue that first made that change, I suppose:
#711

[klonos]

Thank you @indigoxela for pointing the obvious in the PR 😉

I've pushed another change, which makes sure that #711 does not regress. To test it, add these form elements to your form:

$form['toggle'] = array(
  '#type' => 'radios',
  '#title' => t('Toggle me'),
  '#options' => array('hide' => 'hide', 'show' => 'show'),
  '#default_value' => 'hide',
);

$form['option'] = array(
  '#type' => 'checkboxes',
  '#title' => t('Options (should not be shown if options hidden)'),
  '#options' => array('one', 'two', 'three'),
  '#attributes' => array('class' => array('some', 'classes')),
  '#description' => t('Some description (should not be shown if options hidden).'),
  '#states' => array(
    'visible' => array(
      ':input[name="toggle"]' => array('value' => 'show'),
    ),
  ),
);

Then play with the "Toggle me" radios, and confirm that when the checkboxes are hidden, the respective label and help text are also hidden with them. (as a bonus, also confirm that the some and classes classes are preserved in the checkboxes)

[laryn]

Works for me. Tested as you described and also with the latest version of Conditional Fields, which has a bit of an awkward workaround required currently (conditional fields need to be put inside of a field group) but which works as expected without any extraneous field group using this PR. 👍

[klonos]

Thanks for testing @laryn 🙏🏼 ...now I need to fix those 100s of failing tests + plus adjust the inline comments and the description in the docblock of the theme_container() function, to explain that it also handles '#type' => 'radios' and '#type' => 'checkboxes' (since #197, with backdrop/backdrop#324 ...for which BTW I don't think that we have a change record).

[jenlampton]

I added the change record for that old issue (and removed it's label) https://docs.backdropcms.org/change-records/functions-theme_radios-and-theme_checkboxes-have-been-removed

[laryn]

@klonos The failing tests are the only issue on this one, is that right?

[klonos]

@laryn I think so, yes. I'll rebase, in order to get GHA tests, and will work to finalize this.

[klonos]

Sorry for taking this long. This is finally done! 🎉

[laryn]

I have tested again in the sandbox using the Conditional Fields module as described earlier, and it functions as expected. Also confirmed that all the checks are green! 👍

[bradbulger]

this looks like it fixes #3489 as well, nice. PR looks good to me.

[quicksketch]

Thanks @bradbulger and @laryn for the reviews! And @jenlampton for making that change record! I've merged @klonos's backdrop/backdrop#3689 into 1.x and 1.20.x.