aria-required attribute is redundant when required attribute is present

The W3C HTML validator gives a warning where form elements have both required and aria-required attributes: “Attribute aria-required is unnecessary for elements that have attribute required“. As it is a warning and not an error, I'm not sure whether it must be fixed to achieve compliance with W3C WCAG (https://www.w3.org/TR/UNDERSTANDING-WCAG20/ensure-compat-parses.html) - but perhaps best to fix anyway. All browsers support the native HTML5 required attribute - the aria-required attribute is no longer needed unless your supporting very old browsers like IE9 (https://caniuse.com/mdn-api_htmlinputelement_required).

Here's a first patch that removes all occurrences of aria-required, except for ckeditor.js, which is a 3rd party library and should be handled elsewhere.

Changed states.js manually by mistake in #6

I just closed #3240052: redundant wai-aria attribute for aria-required as a duplicate of this issue.
Please credit @jwilson3 for their amazing work on the issue summary that you might want to copy here.

I've in merged the issue summary and tags from #3240052: redundant wai-aria attribute for aria-required. Thanks @DuaelFr

Probably it could even be updated with feedback from comment #1 above as well.

Manually tested with 9.4 and can confirm the patch applied and worked. Setting to RTBC.

Reverting metadata changed by mistake.

It seems like this was duplicated in #3213473, which also has test coverage. Maybe the two should be combined?

#3213473: Checkbox input can't have aria-required attribute is about removing the attribute just from the checkbox field, whereas this issue attempts to remove it entirely from all input fields.
MDN states that aria-required should be used for backwards compatibility only. I think we should check compatibility of Drupal's supported browsers, and then either close #3213473 and add test coverage here, or close this one here.

I agree that #7 would be a better solution than what is being done in https://www.drupal.org/project/drupal/issues/3213473

Just applied and tested #7 and it's working fine for us. And I also agree with swirt, this patch should be used instead.

Just clarifying the testing here. We are now getting passing AXE tests on required fieldset, checkbox, and checkboxes that were formerly failing accessibility scans due to applying `aria-required=true` on these three elements that do not support it.

Regarding tests for this particular change, it seems odd to add a test for something that is completely being removed from the codebase. It is not as though conditional logic was added that needs to be tested, the patch in #7 flat out removes addition of the attribute. I suggest changing this status to RTBC

That Needs tests tag still gives me pause. Test coverage is fairly easy to add for this. And since one could reasonably argue this is a bug, we certainly don't want regression in the future.

Here's a patch that just adds a bunch of simple assertions to the existing FunctionalJavascript tests for the #states api. And then I added a unit test for the RenderElement::setAttributes function. I couldn't find any existing unit test coverage for that function so I ended up adding cases that aren't directly related to this issue. They're simple and they show that we're not accidentally breaking the setAttributes function somehow.

Those failures look perfect so I'm removing the Needs tests tag.

Here's another patch where the fix from #7 runs against the new tests.

The patch in #20 works well in our environment

The patch in #20 works well in our environment.

#20 looks good to me.
+1 RTBC

Patch #20 applied successfully on drupal version 9 and working fine.
+1 RTBC

This looks like a good change. We should default to HTML & not have redundant ARIA.

Re-rolling for 9.5.x and 10.1.x.

Tagged for 4.1.2.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tested this out on Drupal10.1.x with a standard install
Added a required field to the Basic page content type
Verified the required + the aria-required field
Applied the fix
Verified I just now see the required attribute

Running the tests without the fix

1) Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest::testJavascriptStates
Failed asserting that true is false.

Changes look good to me.

Thanks everyone for working on this!

Crediting @thatguy for the initial issue report, @mrshowerman and @danflanagan8 for work on the patch, @jwilson3 for the duplicate, @DuaelFr for triage, @mgifford for accessibility review, and @smustgrave, @DamienMcKenna, and @Glugmeister for further review.

For others: The automated testing infrastructure tells us whether the change set still applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how (rather than just "it works as expected" or "+1" or similar statements). This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

Also see the issue credit guidelines for more information on which kinds of contributions are credited.

1. +++ b/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php
   @@ -109,4 +109,106 @@ public function testPreRenderAjaxFormWithQueryOptions() {
   +  /**
   +   * @covers ::setAttributes
   +   *
   +   * @dataProvider providerTestSetAttributes
   +   */
   +  public function testSetAttributes($element, $class, $expected) {
   +    RenderElement::setAttributes($element, $class);
   +    $this->assertSame($expected, $element);
   +  }
   

   This should have parameter documentation for the parameters from the data provider, and parameter and return typehints.

2. +++ b/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php
   @@ -109,4 +109,106 @@ public function testPreRenderAjaxFormWithQueryOptions() {
   +   * Provides test data for testSetAttributes().
   +   */
   +  public function providerTestSetAttributes() {
   

   The standard format for this is "Data provider for testSetAttributes()." It should also have return value documentation with type array[], and a return typehint. The return documentation can be something like:

   Array of test data, keyed by a short description of the scenario under test. See testSetAttributes() for documentation of the test parameters.

Since this changes markup as well as the user interface for assistive tech users, it normally would not be backported to a patch release. However, since it's an accessibility bug (i.e., telling the user that the element is required twice), it may be eligible for backport. Quoting @Glugmeister:

As it is a warning and not an error, I'm not sure whether it must be fixed to achieve compliance with W3C WCAG (https://www.w3.org/TR/UNDERSTANDING-WCAG20/ensure-compat-parses.html) - but perhaps best to fix anyway. All browsers support the native HTML5 required attribute - the aria-required attribute is no longer needed unless your supporting very old browsers like IE9 (https://caniuse.com/mdn-api_htmlinputelement_required).

As far as I can tell from the linked chart, the only thing that's unclear is whether or not UC Browser supports it. UC Browser was supported by 9.5.x, but support for it was removed in Drupal 10. That would be the only reason not to backport this -- but I'm not sure it's sufficient reason not to backport it.

Tagging for accessibility maintainer review of whether it is okay to backport this to 10.0.x and 9.5.x. I considered FEFM signoff, but in this case I think the accessibility aspect is the main thing that's important.

I closed #3213473: Checkbox input can't have aria-required attribute as a duplicate and am adding credits from that issue here.

@xjm I don't think we need to backport issues that fall under WCAG SC 4.1.1. There's a big debate about removing it from WCAG 2.2, so it may not be in WCAG's future.

The thing is that most parsing errors aren't a problem for accessibility. When 4.1.1 was written, there was some assistive technology that worked with HTML source code. Now they all use the DOM or Accessibility Tree, both of which have been cleaned up by the browser.

So we're an organization that supports open standards, so it is good to fix parsing errors, but it may not make sense to see them as accessibility issues.

I tagged this with WCAG SC 4.1.2 Name, Role, Value.

Redundancy isn't good, but not sure if it needs backporting. It's a pretty low impact issue.

I have to say I also had trouble finding this reference "Where to find it in WCAG 2: 4 Robust > 4.1 Compatible > A > 4.1.2 Name, Role, Value > Warning > Redundant WAI-ARIA attribute" from the project descrption. Not sure why a link wasn't given.

Re-roll against current 10.1.x. Leaving in NW as per #34.

Tagging for accessibility maintainer review of whether it is okay to backport this to 10.0.x and 9.5.x. I considered FEFM signoff, but in this case I think the accessibility aspect is the main thing that's important.

It is OK to backport this change to 10.0.x and 9.5.x, but as mentioned in in #44 it is also not imperative that this happens.

I have created a patch file from the latest commit in the merge request.

Patch #49 applied successfully on Drupal version 10.3.6 and working fine.

left some comments on the MR.

Feedback was addressed or responded to.

Been about a month and don't want to hold it up, was hoping someone else would pick up the review.

Did verify this issue just by using the title field on a content type, verified both the required and aria-required
After the MR I just see the required attribute

Already did a code review

Believe this is ready for committer eyes.

Committed bf98f85 and pushed to 11.x. Thanks!

Will this be backported to Drupal 10? There is old discussion above about backporting but no decision.