Radios element missing "required" attribute

I wasn't sure if I should unset the required attributes (required, aria-required, and aria-invalid) from all fieldsets, or just the fieldset for radios. The required attribute is technically invalid on all fieldsets.

The last commit had a file permission change that should not have beed added.

Thank you for the review @andrewmacpherson

A new patch is attached which addresses all of your concerns.

Manual tested #11 and it looks good on Bartik and Seven

needs screenshots to compare before-after! #16 provides only after ones, so to be sure we need initial state

I have tested patch from comment #11 and result is OK

Reviewing from Drupal europe 2018 mentored by @mmbk

@mfairchild365 While reviewing the code i noticed the following function in form.inc.

if (isset($variables['element']['#type']) && $variables['element']['#type'] === 'radios') {
  unset($variables['attributes']['required']);
  unset($variables['attributes']['aria-required']);
  unset($variables['attributes']['aria-invalid']);
}

We could also choose to not set the attributes instead of unsetting them by overwriting setAttributes() in the class Radios.
I will try this to see if this could work and if so apply a patch.

Added Patch let me know what you think.

@edwin-knol thank you for your work here, for me the result looks good.

I just wonder whether we need some tests here, if not I think it's RTBC

@mmbk Thanks for your support at Drupal Europe.

What kind of test do you mean ?
User testing or automated test.
Should i be the one to put the issue on RTBC or should this be done by another Community user ?

What kind of test do you mean ?
User testing or automated test.

I mean automated tests, they were already suggested at #15

Should i be the one to put the issue on RTBC

No definately not, I'ld like to have a review by a more experienced contributor

@mmbk I'm not able to find the time to create a test for the markup right now.

Just an FYI for anyone else running into this issue...
You can override the preprocess of the radiobuttons with `MYTHEME_element_info_alter(array &$info)`.
This is the way I had to do it on a project (which required major accessibility) and it seems to work.
eg.

function MYTHEME_element_info_alter(array &$info) {
  $info['radios']['#pre_render'] = ['custom_prerender_radios'];
}

function custom_prerender_radios($element) {
  // Set the element's title attribute to show #title as a tooltip, if needed.
  // This first part is copied directly from CompositeFormElementTrait.php
  if (isset($element['#title']) && $element['#title_display'] == 'attribute') {
    $element['#attributes']['title'] = $element['#title'];
    if (!empty($element['#required'])) {

      // Append an indication that this field is required.
      $element['#attributes']['title'] .= ' (' . t('Required') . ')';
    }
  }
  if (isset($element['#title']) || isset($element['#description'])) {

    // @see #type 'fieldgroup'
    $element['#attributes']['id'] = $element['#id'] . '--wrapper';
    $element['#theme_wrappers'][] = 'fieldset';
    $element['#attributes']['class'][] = 'fieldgroup';
    $element['#attributes']['class'][] = 'form-composite';
    $element['#attributes']['class'][] = 'form-composite-radios';
  }

  // Move required to the actual radio buttons.
  if($element['#required']) {
    foreach($element['#options'] as $key => $value) {
      $element[$key]['#required'] = TRUE;
    }
    // Remove required from the fieldset.
    unset($element['#required']);
  }
  return $element;
}

It seems to me this issue should be garnering a lot more attention as no only is it bad for accessibility and screen readers, but it also produces invalid html (required on a fieldset).

Reroll #22 for 8.8.x.

This is not so good a solution because it yields a required marker on all the elements the user could choose from. That could be confusing.

Edit: On second thought, this might be our CSS.

I thought I would chime in here again.
After trying to validate my page, I am being flagged for having aria-required set on radio buttons.
After searching around the interwebs, it seems that a radiobutton group that is required should have a default value.

To move this patch along I added a test form to the form_test.module.

Below shows the patch from #30 is working when navigating to /form-test/radios-required

@2pha is right the aria-required attribute is not supported by radio buttons. I am not sure what the best approach is going to be to address this issue.

The patch adds role="radiogroup" to the radios fieldset wrapper and removes the aria-required attribute from individual radio buttons.

I think the next step is someone to confirm and verify the requirements. Once we know this is the right direction we can add some test coverage to \Drupal\Tests\system\Functional\Form\ElementTest

For #33: ARIA radiogroup and radio example

Backs up the role="radiogroup" for fieldset and also adds aria-labelledby

I tested this patch and found correct:

aria-required="true" is set on the fieldset
role="radiogroup" is set on the fieldset

However, I still saw aria-required="true" on the individual radio buttons.

This patch should remove the aria-required.

This patch confirmed working as expected.

Manual test steps:

• aria-required="true" is set on the fieldset
• role="radiogroup" is set on the fieldset
• aria-required="true" is NOT on the individual radio buttons
• required="required" is on the individual radio buttons

.
Needs accessibility review.

Patch now hash test coverage.

A problem I found when having a required attribute on each <input type="radio" is that screen readers would announce each individual radio button as required, which is confusing.
I was / am working on a government website which requires extreme usability. I am testing on 4 different devices with 4 different screen readers. The only option I could find that kept them all happy was to set a default value.
I think it is important that if these changes are going to happen, that it is tested with multiple screen readers and the outcomes are documented. This would probably just mean setting up a page somewhere with the proposed output and testing in JAWS / Talkback / Voiceover etc, then posting the results here.
By testing, I mean validating the html and the expected screen reader announcements.
I could do the testing if needed.

A couple of notes, since this is new to me too. The radiogroup role is in fact valid on a <fieldset>. See the HTML-ARIA spec for details. Additionally, the W3C is no longer publishing their own version of the HTML spec, and are instead working with WHATWG in their repositority.

I recently tested the support of aria-required on the radiogroup role. It has pretty good support, but using the required attribute on each radio button will most likely yield the most robust support.

wondering if something like this would work as a stop-gap?

function mymodule_preprocess_fieldset(&$variables) {
  unset($variables['attributes']['required']);
  unset($variables['attributes']['aria-required']);
  unset($variables['attributes']['aria-invalid']);
}

Anyone looking for a quick solution to this on a custom form you can add the following:

'#attributes' => ['required' => 'required']

e.g.

$form['my_radios'] = [
      '#type' => 'radios',
      '#title' => $this->t('adios'),
      '#options' => [
        'aaa' => $this->t('aaa'),
        'bbb' => $this->t('bbb'),
      ],
      '#attributes' => ['required' => 'required'],
      '#required' => TRUE,
    ];

That will add the required attribute to each radio and doesn't mess around with anything else

Not sure where "#radios" / "#checkboxes" came from.

I opened a MR with a rebased version of 2951317-38.patch.

Adding WCAG 1.3.1 Tag.

Attached is simply a patch version of #59 for use in composer.patches.

Hmm, @fenstrat the widget with the merge request contains a link plain diff that can be used for the composer patches.

added patch Fixed some Coding standard issue

@mmbk indeed that's how that patch was generated. Linking to diffs from MR's in composer patches is not a good idea. Anyone can change the code it would include by simply pushing to the MR. On your next build you're now including potentially broken, possibly malicious code.

On 9.3.22 Right now, i had to go with #50, that was the only solution that worked for me.

Verified and tested patch #63 on 10.1.x-dev. Patch applied cleanly.

Test Result:
1. The required attribute is present on the input elements - CORRECT
2. The aria-required="true" attribute is present on the fieldset element, but not on the input elements - CORRECT
3. As per #46, required attribute should not be present on the fieldset elements but it is present - INCORRECT

Keeping it to NR to confirm the expected solution.
Please refer attached before and after screenshots

Reviewing the remaining tasks

Remove required + and aria-required attributes from the <fieldset> element to the <input> element.

Still needs to happen

CSS to avoid visual regression. Asterisk should be after the fieldset legend, not the individual radio buttons

May still need to happen.

Adding #3267246: ['#states']['required'] broken for radios as related. We should probably fix this issue first, then circle back there to make sure #states works, too. Tagging this bug to be smashed...

FWIW we discussed this in bug smash hard problems meeting (in conjunction with the related issue dww linked).
Here's a codepen https://codepen.io/larowlan/pen/oNPVEax that demonstrates that putting required on each item is fine as suggested by mstrelan in that meeting.

One thing to think about though is how does a user unset a radio value once set, but I think that impacts the states issue more, will comment there

How can the resolution of this proceed?

Rerolled onto a new 11.x based branch. https://git.drupalcode.org/project/drupal/-/merge_requests/7341

Patch on comment #79 apply in a Drupal 10.2.7
Attribute required appear on html input radio but when you submit a form with a required radio TRUE, we don't have native error message but in console I see a mysterious javascrit error

An invalid form control with name='field_contact_email' is not focusable. <input data-drupal-selector=​"edit-field-contact-email-1" required=​"required" type=​"radio" id=​"edit-field-contact-email-1" name=​"field_contact_email" value=​"1" class=​"form-radio">​

Rerolled patch in #62 to work with Drupal 10.3.0

Patch #81 worked perfectly on Drupal 10.3.0. Thanks for the re-roll.

Tested MR:7341 works fine on 11.x
Attaching screenshots for reference.

CSS to avoid visual regression. Asterisk should be after the fieldset legend, not the individual radio buttons.

Seems this may still be needed

Rebased the MR as it was 900+ commits back.

Hi folks,
I’ve updated the merge request to remove the required attribute from the fieldset for the radio buttons — the aria-required attribute is no longer present, as expected.
I also updated the CSS to ensure that the asterisk (*) still appears in the fieldset legend for radios, even without the required attribute.

I’d like to propose a new variable: has_required_children.
Since the required attribute applies only to individual form elements and not to the fieldset itself, we can’t rely on it directly for fieldsets. This new variable would allow us to handle required state information at the fieldset level, particularly for radio buttons and checkboxes.

With this variable in place, we can also properly manage the required and aria-required attributes for fieldsets used in checkbox elements.

If a new variable is being added think it needs to be mentioned in the IS. But is that for sure the only way? By doing a twig template change we will need a CR as this fix won't hit contrib or custom themes. Ideally if possible if this can be determined before twig that would be best.

for anyone who has faced this, the patch from merge request 2951317-11.x (https://git.drupalcode.org/project/drupal/-/merge_requests/7341.patch) does not apply to 11.2.2. However, the diff file (https://git.drupalcode.org/project/drupal/-/merge_requests/7341.diff) works fine.

I also tried merging 11.x into 2951317-11.x, but had the same issue.

I've pushed a change so we don't need the new twig variable.

I'm also unsure if the CSS changes are needed anymore. I've tested both on Claro and Olivero with and without these changes and everything looks identical since we're simply allowing the form-required class to be added on the fieldset legend without the fieldset itself being required.

We can't apply the patch from merge request (https://git.drupalcode.org/project/drupal/-/merge_requests/7341.patch) anymore on drupal 11.1.6

I tested the latest changes of the merge request and works as expected.
I follow this steps.
- Create a new instance of Drupal 11.3
- Add in the basic page content type a list text field with 3 options and make it required field.

Without the changes, the required is in incorrect position and there is no validation after submit.

Wrong required attributes.

After changes applied.

Required attributes in the correct elements

Triaging the RTBC queue.

I have not read all the comments but nothing stood out as incomplete in a skim. I read the MR and made some suggested changes so setting to needs work for that. Also, the issue summary is out of date. If refers to 'aria-required' which is not in the MR and to Seven and Bartik which are no longer i core. Tagging for an IS update.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

An assertion for the fieldgroup attribute was removed by commit b0953653, in between comments #85 and #86. So this feature is currently untested. I read through the comments about the attribute. It's clear that its addition was contentious. So that's something for accessibility reviewers to consider. But the attribute either needs to be tested or removed. I'm setting the issue to Needs Work for this.

I also noticed that there are CSS additions to Claro and the Starterkit themes. Are these changes something that all themes will need to implement? Because that will require a change record.

i wanted to test the current state of the MR, but after checkout i ran into fatal errors due to recent changes in 11.x

Fatal error: Uncaught Error: Class "Doctrine\Common\Annotations\TokenParser" not found in /var/www/html/repos/drupal/core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php:192

the MR probably needs a rebase

Hi All,
I am using drupal 10.6.7 and I have created a patch using the code in the latest MR.In the latest MR, Drupal 11 has core/lib/Drupal/Core/Form/FormPreprocess.php while in D10 we have corresponding code in form.inc.
My custom patch worked fine and resolved accessibility issue we were facing due to aria-required attribute in fieldset. Apart from this patch -

• Adds required attribute to each individual element
• Adds role="radiogroup" to the radios fieldset for screen reader support
• Preserves the required asterisk indicator on the fieldset legend

I have also tested the latest MR in Drupal 11 in simplytest site but it does not apply.
Could someone update what is going on with this issue?

I have noticed one more thing, webform fieldset now getting 'error' class and aria-invalid="true" after applying this patch.
<fieldset data-drupal-selector="edit-test" id="edit-test--wrapper" class="radios--wrapper fieldgroup form-composite webform-composite-visible-title error js-webform-type-radios webform-type-radios js-form-item form-item js-form-wrapper form-wrapper" role="radiogroup" aria-invalid="true" data-once="webform-radios-required">

This is because of the code in core/lib/Drupal/Core/Render/Element/Radios.php in latest MR

    if (isset($element['#parents']) && isset($element['#errors']) && !empty($element['#validated'])) {
      $element['#attributes']['class'][] = 'error';
    }

As noted #3267246-25: ['#states']['required'] broken for radios...

I agree that the required attribute and class handling should follow WCAG (see here, here).

Since this issue changes initial class and attribute placement, it should also handle state changes.

Kicked off a new MR spun from an updated main branch. Also added states:required support.

Seems we're failing a test.

Behat\Mink\Exception\ElementNotFoundException: Element matching css ".views-messages" not found..

Drupal\Tests\views_ui\FunctionalJavascript\ConfigHandlerExtraFormTest::testExtraOptionsModalValidation().

$assert->elementTextEquals('css', '.views-messages', 'Error message Selection type field is required.');

The current direction looks good, but based on testing downstream in Conditional Fields and Require on Publish, I think we should revisit one part of the dynamic _none handling.

We may need another change here. When a radio group becomes required, we should hide/show the _none option, but we should not force its checked state on state change, which I think I currently do in this latest MR update.

In other words:

• when required becomes active, hide _none
• when required becomes inactive, show _none
• preserve whatever checked state already exists rather than auto-selecting or clearing _none

That keeps the UI behavior predictable and avoids introducing extra state changes during the transition. If _none was selected before being hidden, it should remain selected while hidden and still be there when shown again. If it was not selected, we do not need to select it programmatically.

I think that makes the states behavior cleaner and avoids conflating visibility changes with value changes.