While working on the issue #1645328: Add test to ensure fieldset allows any non-empty-string #title I accidentally discovered this bug (see comments #55 and #57. According to @Liam Morland an empty legend tag should not be generated when the title of a fieldset is empty.
Inside any .module file (from a currently active module on your site) paste the following code:
function [module name]_form_[form id]_alter(array &$form, FormStateInterface $form_state, $form_id) {
$form['test_empty'] = [
'#type' => 'fieldset',
'#title' => '',
'#description' => '',
];
$form['test_full'] = [
'#type' => 'fieldset',
'#title' => 'Lorem ipsum',
'#description' => 'Lorem ipsum',
];
}
Replace [module name] with the machine name of the module and [form id] with the id of any form you can "mess" with.
Clear the site cache
Access the form related to the form id you used previously.
You should see two new fields one empty one with "Loren ipsum" as the title and description.
Access the browser dev tools (usually pressing F12) ans inspect these new elements.
The empty field has a <legend> element inside.
Expected behavior: There should not be an empty title element (see the description field for comparison, when there's no description an empty description element is not created.)
Add an if condition to the title section(core/modules/system/templates/fieldset.html.twig)
{% if legend.title %}
<legend{{ legend.attributes }}>
<span{{ legend_span.attributes.addClass(legend_span_classes) }}>{{ legend.title }}</span>
</legend>
{% endif %}
Before
After
- Discuss the proposed solution
- Create a patch or MR with the solution + tests
- Review
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | After.jpg | 983.3 KB | ranjith_kumar_k_u |
| #15 | Before.jpg | 1.15 MB | ranjith_kumar_k_u |
| #13 | 3283883-13.patch | 2.53 KB | ranjith_kumar_k_u |
| #7 | 3283883-7.patch | 2.17 KB | ranjith_kumar_k_u |
| #7 | 3283883-7-test-only.patch | 1.3 KB | ranjith_kumar_k_u |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare changes, plain diff MR !3895
Comments
Comment #2
andregp commentedUpdating priority.
Comment #3
andregp commentedfixing some markup.
Comment #4
jcbautista97 commentedThis is my contribution to solve the issue, please check.
I add the conditional to fieldset create the element title when is not empty.
Comment #7
ranjith_kumar_k_u commented#4 provided the solution for the Seven theme, but the theme is no more with the core.
Claro and Olivero don't have this issue but the issue exists in "core/modules/system/templates/fieldset.html.twig" file.
Created patch with tests, please review
Comment #8
ranjith_kumar_k_u commentedComment #9
smustgrave commentedCan we also get a positive assertion. Meaning if the fieldset isn't empty and the title appears the count should be 1.
Rest looks good.
Comment #13
ranjith_kumar_k_u commentedAttached patch for 11.x.
(I tried to create an issue fork branch from 11.x but I couldn't, 3283883-11.x this is not the correct branch for 11.x please ignore this )
Comment #14
smustgrave commentedIssue summary could use some up to date screenshots.
Also since this is changing a core template will need a change record
Comment #15
ranjith_kumar_k_u commentedCreated a change record - https://www.drupal.org/node/3367586
Comment #16
smustgrave commentedThink this is ready for committer review.
Comment #17
lauriiiWe need FEFM review to decide in which themes this should be fixed.
Comment #18
bnjmnmHere's the FEFM / A11y review, but I regret to say it's not a clean response to what was being asked above.
It will likely be necessary to come at this from a different angle. The fact that it is currently possible to have empty
<legend>elements is already not good for accessibility. Making it possible for<legend>to be truly optional worsens the accessibility.To be level A accessible (level A is the bare minimum), fieldsets must have a legend as the first child element .
So something should be addressed, but not by allowing elements to be less accessible. These are the main things to consider
<legend>. This should be enforced#titlefor fieldsets. We can't suddenly start enforcing this in a non-major release, though, as it could break sites. Adding a deprecation warning in 10 for non-empty titles in 10 (possibly with an additional logged warning?) then enforcing in 11 could work.Comment #19
bnjmnmIssue summary will need updating given the wrench I just tossed into this.
Comment #20
kentr commented@bnjmnm
A quick experiment shows that we can probably do this by accommodating
fieldset#933004: Test that all form elements have a #title for accessibility, which I'm working on.I propose adding
fieldsetto the scope of that issue.Comment #21
kentr commentedI think
fieldsetis already in scope of #933004: Test that all form elements have a #title for accessibility, really. It's a form element, after all. I'll go ahead and commit the experiment.Comment #23
kentr commentedComment #24
mgifford@kentr if fieldset is in scope of #933004: Test that all form elements have a #title for accessibility we should just list this status as postponed or even duplicate, right?