Problem/Motivation
States on radios and checkboxes elements are currently broken until #994360: #states cannot check/uncheck checkboxes elements is resolved, but when that issue is fixed another appears.
When the states API manipulates the 'disabled' or 'checked' state of form elements, it looks for the closest 'form-item' or 'form-wrapper' element of the state target and changes the attribute on all of its children.
'checkboxes' and 'radios' elements do not have the 'form-wrapper' class, which causes the wrapper element higher in the hierarchy to be found and have all of its children affected instead of just the desired element.
e.g. with the following form, when the radios element is changed from 'one' to 'two', the checkboxes should be hidden. Instead, the entire fieldset is hidden.
$form['group'] = [
'#type' => 'fieldset',
'#tree' => TRUE,
];
$form['group']['radios'] = [
'#type' => 'radios',
'#title' => 'Radios',
'#options' => [
'one' => 'One',
'two' => 'Two',
],
'#default_value' => 'one',
];
$form['group']['checkboxes'] = [
'#type' => 'checkboxes',
'#title' => 'Checkboxes',
'#options' => [
'alpha' => 'Alpha',
'beta' => 'Beta',
],
'#states' => [
'visible' => [
[':input[name="group[radios]"]' => ['value' => 'one']],
],
];
Proposed resolution
The issues #2041845: Remove theme_checkboxes() and call theme('container') instead and #2041825: Remove theme_radios() and call theme('container') instead intended to remove the 'checkboxes' and 'radios' templates in favour of a generic 'container' template, which would have the 'form-wrapper' class. These templates can no longer be removed for BC reasons, but they should be made to match the classes of the container template.
Comment | File | Size | Author |
---|---|---|---|
#27 | Screencast from 11-12-2023 05 43 56 ΜΜ.mp4 | 1.6 MB | dimitriskr |
#11 | drupal-2945727-11-radios-checkboxes-form-wrapper.patch | 3.4 KB | gapple |
#4 | drupal-2945727-4-radios-checkboxes-form-wrapper.patch | 3.57 KB | gapple |
Comments
Comment #2
gappleThis patch adds
js-form-wrapper
,form-wrapper
, and the relevant element class to all radios/checkboxes templates in core.This also highlights an inconsistency in the current templates: all of the current checkboxes templates add the
form-checkboxes
class (system module, classy theme, stable theme), but only the classy theme's radios template adds theform-radios
class.Comment #4
gappleFixed syntax error in patch
Comment #5
gappleComment #6
borisson_I'm not sure if we're allowed to do this to the stable theme.
Comment #7
andypostSure, it is no-go for stable
Comment #9
joelpittetBugs can be fixed in stable, I think there could be a case made for this, but a light touch should be made.
Comment #10
gappleThe states patch only uses
js-form-wrapper
, soform-wrapper
should be safe to remove from the patch.Core and stable applying
form-checkboxes
but notform-radios
could be broken out to a separate issue, since it's not related to this bug but is still a consistency issue.Comment #11
gappleI've opened #2988461: Radios template does not apply form-radios class to address the consistency issue with the
form-radios
class.The attached patch applies the minimal change of only adding the
js-form-wrapper
class to checkboxes and radios elements.Comment #12
gappleI need to double check the behaviour here. The
Checkboxes
andRadios
render element classes useCompositeFormElementTrait
, which adds the fieldset theme wrapper but only if the element has a#title
or#description
attribute.fieldset.html.twig
then adds both thejs-form-item
andjs-form-wrapper
classes.It may be better to add the js-form-wrapper class via
CompositeFormElementTrait::preRenderCompositeFormElement()
only when the fieldset wrapper is not applied, otherwise states may hide the checkbox elements but not the corresponding title and description in the fieldset.Comment #13
dwwThis is yet another good example of how our policy generates deadlock and prevents fixing bugs.
Marking this blocked on #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption
Comment #14
dwwSorry for the noise, but #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption has evolved, and now I see we have #2352949: Deprecate using Classy as the default theme for the 'testing' profile as the immediate solution to our bugfix deadlock.
Comment #16
lauriiiSince this is only adding a js- prefixed class, I think we could proceed with the plan. js- prefixed classes are less likely to collide with pre-existing class names, and they also don't attach any styles. Since this bug fix might require template change from theme authors that have overridden this template, we should provide a change record explaining the necessary steps for them to take. I also tagged this issue for another review later.
Comment #17
lauriiiComment #26
dimitriskr CreditAttribution: dimitriskr commented--deleted--
Comment #27
dimitriskr CreditAttribution: dimitriskr commentedAttached is a screencast with the form code from the IS, running in latest 11.x with Olivero and cannot reproduce the bug.
If someone else sees it, please write some steps to reproduce