🇬🇧United Kingdom @andrewmacpherson

This affects profiles and themes too, in addition to modules. Basically, if happens whenever an extension has no PHP files.

You can build a very elaborate theme with lots of JS/CSS libraries and Twig templates, yet without using any PHP in a .theme file. Such a theme will run into the PHPStan problem here, with no files to analyze.

Quick review of the MR so far (as of comment #2):

I think the right place to check for the "No files found to analyse" situation would be in DeprecationAnalyzer::analyze(). Perhaps by examining $process->getErrorOutput(),

I found an intriguing issue in the upstream PHPStan project: https://github.com/phpstan/phpstan/issues/9410.

The bleeding edge PHPStan now has a "zeroFiles" configuration option, to ignore the situation where there are no files to analyze. This option was only added a couple of days ago, so it's not ready to use yet. If it becomes stable, perhaps the Upgrade Status could inject this option when it runs?

quietone → credited andrewmacpherson → .

I don't really mind the white noise, or the Drupal core issues that result from it. It's nice to know people run automated tests.

aXe has some ways to turn off certain tests; the browser plugin has a button to disable the "best practice" category, and if you run it from the CLI you can exclude specific tests I think. What you can't do is flag a specific test result as a false flag.

The thing is, the aXe report here ISN'T a false report. It really would be better if we didn't skip a heading level. It's just that the impact isn't actually very severe in practice, and the proposed cure was worse than the disease.

Also I'd be wary of adding js just to prevent an accessibility flag in axe tools.

Yeah, I think that would be folly, too. It would also make us subject to ridicule and suspicion in the wider accessibility community.

Aside: there have been some disturbing reports alleging that an accessibility overlay company interfered with the WAVE accessibility checker, and WAVE's developers noticed this. Some nice bedtime reading at #accessiBe Spoofs Automated Checkers.

There is no way we can allow JS in Drupal core just to silence a test suite.

The issue is filed against Seven theme, but that's now removed from core, so this issue either has to move to contrib, or we have to keep the issue in core and change the component.

I want to keep this issue in core, because I haven't looked into it yet, and it deserves an accessibility maintainer's attention.

Tagging for review so it's on my feedicle. Moving to the one of the menu modules as a possible culprit, though this may turn out to be wrong. I have a few other hunches I want to look at.

I've discovered a few interesting things after digging around...

My website's composer.lock and composer-manifest.yml both have an entry to require drupal/color, and git blame tells me that this appeared when I upgraded to D9.5.x a few days ago. It isn't a requirement of my root composer.json.

Aside: I also have the drupal/environment_indicator contrib module as a requirement of my root composer.json. Currently I have version 4.0.5 which lists drupal:color as a dependency in the module info file, but the contrib project does not have a composer.json of it's own.

After I disable the Environment Indicator and Color modules, then remove them using composer, the core color module IS then described as deprecated on the Extend page. Now I see a deprecated link, where I didn't before.

The funnny thing is, I could also see that the the CKEditor contrib module is installed, and it does show a deprecated link, even though the contrib Color module does not.

Looking at the CKEditor and Color module files...

I have two versions of these present on my filesystem: the ones from Drupal core, and the contrib versions. Neither are explicitly mentioned as root requirements in my composer.json, but I see them in my composer.lock and composer-manifest.yml.

The Color and CKEditor versions inside web/core/modules have the lifecycle:deprecated line.

The CKEditor version in web/modules/contrib has the lifecycle:deprecated line

The Color module version in web/modules/contrib does not have the lifecycle:deprecated line.

Well now I'm confused. I follow the news, so I already knew that the Color module was deprecated in D9.5, and has been removed from D10. So I was puzzled that the Status Report page didn't complain about it.

After digging around, I think the deprecation status is working fine in Drupal core, for the core versions of deprecated modules.

However, the contrib versions of Color and CKEditor modules are behaving differently to each other.

My hunch is that the CKEditor module got the lifecycle:deprecated message added BEFORE the contrib project was created, but the Color module got this line added AFTER the contrib project was created.

Upshot: this was mightily confusing, but I'm not sure anything is actually broken, at least in core. So I'm reclassifying this as a support request.

Something in the orchestration of this core-to-contrib migration has gotten mixed up, resulting in confusing messages. Either the contrib Color module or the contrib CKEditor module is saying the "wrong" thing, but I'm not sure what they're supposed to be saying. I haven't looked at the other new contrib projects for deprecated modules.

I'm going to bed. I'll let somebody else decide which project to move this issue to :-)

andrewmacpherson → created an issue.

Re. #34 - Aha... thanks lauriii. I saw in the D9.5.3 release notes it was reverted, but I didn't realize it was still there in the D10 branches.

Re. #35 - We still have the heading, barring a brief blip in D10.0.3

That JS logic could be useful to study. I think server-side logic to write the correct level in HTML would be preferable to JS though.

This issue is already postponed, but the reason for postponement has changed.

This issue was filed as a follow-up to 🐛 Pager h4 causes accessibility flag on many pages Fixed .

Since then, I've made a very strong objection to removing the heading from the pagination template. In which case, the heading ID is still needed.

That would make this issue would a wont-fix. I'll leave it in a postponed state while the other is still being discussed.

Postponing this issue. It was filed as a follow-up to 🐛 Pager h4 causes accessibility flag on many pages Fixed , which was committed and later reverted.

Since then, I've made a very strong objection to the proposal in 🐛 Pager h4 causes accessibility flag on many pages Fixed . I don't think we should remove the heading.

So, this issue isn't currently needed, and may well turn out to be a wont-fix.

Alternative proposal: keep the heading. Can we improve the heading level?

We could deal with the missing level by promoting it to h3, and aXe would stop complaining.

The devil is in the details though...

Headings assume ownership over ALL of the content which follows, until another heading of the same level (or higher) is encountered.

For example, consider the admin/content page, with the default configuration from Standard profile.

Currently it looks like the pagination (h4) is a sub-heading under the breadcrumb navigation (the preceding h2). This is clearly nonsense, and promoting the pagination heading to h3 wouldn't improve that at all. It would still be a misleading heading structure, even though we'd eliminated the missing level which aXe complained about.

We could promote the pagination to a h2 level. Then it would be a subheading of the main page heading (h1), and a sibling of the breadcrumb and primary tabs. Much better.

Yet, a h2 assumes that the pagination relates to the main content of the page. For a views block in a sidebar region, the h2 would be too important; the views block would have it's own h2. Or, the views block might have no heading at all. Either way, giving the pagination a h2 in a views block would produce a misleading outline, suggesting it related to the main content.

Managing heading levels with a template-driven publishing system is a pain-in-the-architecture. There is no single correct level for every use of the template.

A more ambitious approach would be to compute heading levels, for the context of the assembled page. I'm not aware of prior PHP work which does this, but there's an interesting article from Heydon Pickering about how to do it in a React Javascript application: Managing Heading Levels In Design Systems. I expect there are still devils lurking in the details there too. Heydon's use of aria-level could complicate our existing CSS, but a server-side PHP approach might avoid using aria-level if we computed the heading levels late in the page render process.

I Saw this on the Drupal 9.5.3 release notes yesterday, including the reverting commit.

I think the proposal here is rather brutal. It doesn't take account of well-attested user behaviours. I don't think we should remove the heading.

@nicxvan - Thanks for reporting the issue. I appreciate the work you've done to maintain the patch here.

I'd like to take issue with the appraisal so far. Comment #8 by @bnjmnm moved this to RTBC:

To assistive tech, this solution works identically so it's not at all disruptive.

Emphasis mine. This remark is concerned with the assistive technology software, rather than the software's human users. The proposal may not be disruptive to software, but it is likely to be disruptive for users.

Previously, the pagination could be found by screen reader users in a number of ways:

1. Peruse headings. Many screen readers provide tools for this, but no widely used web browsers do so.
2. Using the web browser's find-in-page tool. It can match the content of the heading element. (Aside, you don't need actually need a screen reader for this.)
3. Peruse landmark regions. Many screen readers provide tools for this, but no widely used web browsers do so.

After implementing the proposal here, two of these methods no longer work:

1. The heading is gone, so you can't find the pager by perusing headings.
2. Using the browser's find-in-page tool no longer works. It doesn't match the content of the aria-label attribute.
3. The only method still available is to peruse landmark regions.

Will this be disruptive for actual users? Let's look at some user data.

The WebAIM Screen Reader User Survey #9 asked respondents:

When trying to find information on a lengthy web page, which of the following are you most likely to do first?

1. Navigate by headings was by far the most popular method, with approximately two-thirds of users preferring it.
2. Using a find-in-page tool was the next most popular, with approx 15% of users.
3. Navigating by landmark regions was the least popular method, at under 4% of users.

The same question was asked in earlier WebAIM surveys:

• Screen reader user surveys #8, #7, #5, and #4 showed roughly similar results to survey #9. It's been very consistent for a decade.
• Curiously, the question was omitted from survey #6.
• In surveys #2 and #3, headings and find-in-page were also the most popular methods. These surveys didn't offer landmark regions as an answer.

But the proposal here puts the kibosh on the two most popular methods, and doubles down on the least popular method!

Concerning the report from the aXe tests:

The pager by default uses an h4, this creates the "Heading levels should only increase by one" error in many use cases.

• Deque's documentation for the Heading levels should only increase by one test describes it as a "Deque best practice" of "moderate" impact.
• Deque's commentary describes why a sensible heading outline is useful, and gives recommendations for writing effective heading text.
• What the commentary doesn't address is: It completely fails to say how the skipped level presents a problem.
• The commentary doesn't encourage the removal of headings as a way to address the skipped level. On the contrary, the article goes to a lot of effort to advocate for the use of headings.

It's also worth noting that the skipped heading level IS NOT a failure of any WCAG success criteria. For a detailed explanation, see David Swallow's article: Heading off confusion: When do headings fail WCAG?

So, what's the impact (for users) of skipping a heading level? It's actually not as bad as the aXe rule title implies.

Screen readers are quite forgiving of missing heading levels. The missing heading level isn't a complete disaster.

Screen reader users can interact with headings in various ways:

1. All the screen readers I've ever used provide tools to move to the next/previous headings regardless of the heading level. Indeed, when using a touchscreen on a mobile OS, this is typically the only mechanism for moving amongst headings. The skipped heading level has no impact here whatsoever; the only thing that matters is the order of headings.
2. Many screen readers also provide commands to move among headings of a particular level, when using a keyboard. Most desktop screen readers have this feature, as do iOS VoiceOver and Android Talkback when a Bluetooth keyboard is connected. In practice this is very useful for jumping to the major sections of the page, while ignoring the intervening lower levels. It isn't quite as much use for navigating among the lower nested levels of headings; sometimes you ask for the next H4, only to be told that there isn't one.
3. A few screen readers (e.g. NVDA) provide a hierarchical tool to navigate headings as a tree. The NVDA tool is forgiving of skipped levels. It will show a H2 and H4, and still let you access the H4. AFAIK, no screen reader for a mobile OS provides a hierarchical heading browser, so the skipped heading level has no practical impact.

I'd like to stress that accessibility is about helping actual users, not satisfying opinionated best-practice rules in automated tests. It certainly isn't about keeping developer's dashboards green. Has any screen reader user asked for the heading to be removed?

Should we continue to provide a heading for the pagination? Yes. It facilitates the two most popular methods of finding information, as reported by the WebAim screen reader user survey.

Should we continue to use a landmark region? Yes. A moderate number of landmarks are very useful for finding the major sections of the page, and/or small-but-important tools on the page. Pagination for the main content is a good candidate for a landmark region.

Whilst landmark regions aren't the first tool that all users reach for, a simple landmark structure is a great accompaniment to a detailed heading outline. The belt-and-braces approach of using landmarks and headings offers choice for users.

For these reasons, I'd say no to the current proposal to remove the heading.

There's something in the evaluation approach which should be avoided in future...

#200 says:

GOOD: grep for 'aria' has the healthiest results of any library.

I was puzzled by what this meant. Elsewhere I see grepping for mentions of "aria" was part of the evaluation. But what does healthy mean here - a lot of matches, or few matches?

Never give any weight to this count. It's highly misleading because:

• The first rule of ARIA use, is that you don't use ARIA when the host language already provides the semantics and behaviour you need. If you find a lot of instances of "aria", the first reaction should be: "Why so much ARIA? Hasn't the developer heard of HTML?"
• The number of times that ARIA roles, states, properties, and relationships are used says nothing at all about whether they have been chosen well, or applied to appropriate elements. Developers just make stuff up, a lot.
• ARIA only provides semantics. Even if roles, states, and properties have been chosen well, the actual behaviour still needs to be implemented. Developers sometimes omit a lot of that.
• At best, the number of times "aria" appears can tell you which library has made the most elaborate attempt to address accessibility.

I appreciate the level of detail in the evaluation comments - so much to digest. The ARIA grep count shouldn't be given any credence though.

This is particularly important for media entities. The media bundles configured in Standard profile have a minimal set of fields, but there are lots of use cases for additional fields. For example:

• License, copyright, or access rights, etc.
• Details of how/when a recording was made.
• Participants or subjects of a recording.
• Various fields relating to learning management systems.
• Transcripts of audio/video media. These are something we need to support in core to address WCAG and ATAG time-based media accessibility criteria. Being addressed in 🌱 Provide authors with tools to manage transcripts and captions/subtitles for local video and audio Active
• Long descriptions for complex images

Re. #45: Thanks @mfairchild365.

The radiogroup role is in fact valid on a . See the HTML-ARIA spec for details.

You're right, that's a nice find! ARIA-in-HTML does say that the radiogroup role is permitted on the fieldset element, but the HTML5.2 rec says it isn't. So these two W3C standards are at odds with each other. The ARIA-in-HTML doc is only a working draft (and I think it's intended to have W3C Note status, rather than Recomendation status), but it's more recent than HTML 5.2. I don't think there's an easy way to square that.

Additionally, the W3C is no longer publishing their own version of the HTML spec, and are instead working with WHATWG in their repositority.

I'm not sure that's relevant; note that the WHATWG version of HTML doesn't offer any opinion on ARIA at all.

I'm not certain what to do about the radiogroup role at this point. I'm inclined to say it's out of scope here, because the central problem in this issue is the way we're indicating the required-ness of the radios, not which container role is most appropriate.

... but using the required attribute on each radio button will most likely yield the most robust support.

I'm in agreement with that.

I think we have established the following things:

• The required attribute should be present on the input elements, but not on the fieldset element.
• The aria-required="true" attribute can be present on the fieldset element, but not on the input elements.
• aria-required is allowed on elements with either thegroup, or radiogroup role. So it's permitted on the fieldset element regardless of whether the fieldset can have the radiogroup role.

I think it's worth checking these with assistive tech:

• Whether there is any practical difference between <fieldset> and <fieldset role="radiogroup"> in the real world, given the discrepancy in the standards.
• Whether the accessible name of the radiogroup is correctly computed from the legend element. Do we need to include an explicit aria-labelledby along with the radiogroup role.
• Whether it makes any practical difference whether the fieldset has the aria-required attribute. (Assuming the individual radio inputs have the required attribute.)

Code review of patch 38:

1. diff --git a/core/lib/Drupal/Core/Render/Element/Radios.php b/core/lib/Drupal/Core/Render/Element/Radios.php
   index 2c9797a3c1..8413113df0 100644
   --- a/core/lib/Drupal/Core/Render/Element/Radios.php
   +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -32,6 +32,31 @@ class Radios extends FormElement {
    
      use CompositeFormElementTrait;
    
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * Overwrite setAttributes to remove unnecessary attributes from the
   +   * fieldset for radio groups. These are applied to the child radio
   +   * elements instead.
   +   */
   +  public static function setAttributes(&$element, $class = []) {
   +    if (!empty($class)) {
   +      if (!isset($element['#attributes']['class'])) {
   +        $element['#attributes']['class'] = [];
   +      }
   +      $element['#attributes']['class'] = array_merge($element['#attributes']['class'], $class);
   +    }
   +    // This function is invoked from form element theme functions, but the
   +    // rendered form element may not necessarily have been processed by
   +    // \Drupal::formBuilder()->doBuildForm().
   +    if (!empty($element['#required'])) {
   +      $element['#attributes']['class'][] = 'required';
   +    }
   +    if (isset($element['#parents']) && isset($element['#errors']) && !empty($element['#validated'])) {
   +      $element['#attributes']['class'][] = 'error';
   +    }
   +  }
   +
   

   I think this is the right method to use, but the brute-force override is a bit risky from a long-term maintenance point of view. If RenderElement::setAttributes() ever changes, then Radio won't get the benefits, and we'd have to replicate some logic again in Radio::setAttributes().

   The only thing we're interested in changing is the way required and aria-required are used, not other attributes like class. It would be safer to override it more like this:

   /**
    * (@inheritdoc}
    */
   public static function setAttribute(&$element, $class = []) {
     parent::setAttribute($element, $class);
   
     // Unset the HTML required attribute which isn't permitted on <fieldset>.
     // ...
   }
   

2. +        // Only add the required attribute to radio buttons.
   +        // @see https://stackoverflow.com/questions/34300154
   

   This could be phrased better. The important thing is that we're avoiding adding the aria-required attribute to the <input type="radio"> element, because it would be invalid HTML/ARIA.
   I don't rate that stackoverflow question/answer very highly; it has some misleading information, which I've commented about there. Instead, we can refer directly to the HTML5.2 rec, in section "3.2.8.4. Allowed ARIA roles, states and properties" (https://www.w3.org/TR/html52/dom.html#allowed-aria-roles-states-and-prop...).

3. +  /**
   +   * Adds role "radiogroup" to radios fieldset wrapper.
   +   */
   +  public static function preRenderRadiosFormElement($element) {
   +    $element['#attributes']['role'] = 'radiogroup';
   +    return $element;
   +  }
   +
   

   This preRender callback is the part which produces the invalid <fieldset role="radiogroup">. It can be removed.

4. +    $elements = $this->xpath('//fieldset[@id="edit-radios-required--wrapper" and @aria-required="true"]');
   +    $this->assertCount(1, $elements)
   

   This is correct. I wonder if we should also assert that the fieldset element doesn't have the (invalid) HTML required attribute?

5. +    $elements = $this->xpath('//fieldset[@id="edit-radios-required--wrapper" and @role="radiogroup"]');
   +    $this->assertCount(1, $elements);
   

   The assertion about the radiogroup role won't be needed.

6. +    $elements = $this->xpath('//input[@id="edit-radios-required-0" and @required="required"]');
   +    $this->assertCount(1, $elements);
   +    $elements = $this->xpath('//input[@id="edit-radios-required-0" and @aria-required="true"]');
   +    $this->assertCount(0, $elements);
   

   Looks good.

7. diff --git a/core/themes/classy/css/components/form.css b/core/themes/classy/css/components/form.css
   index ff8780c03f..5a6e220c74 100644
   --- a/core/themes/classy/css/components/form.css
   +++ b/core/themes/classy/css/components/form.css
   @@ -82,6 +82,9 @@ label.option {
      background-repeat: no-repeat;
      background-size: 6px 6px;
    }
   +.form-radios .form-required:after {
   +  display: none;
   +}
   

   I couldn't see where this selector actually matches a DOM element. Is this cruft from an earlier version of the patch?

#42 - I love the term "extreme usability" :-)

It's the normal behaviour of most screen readers to announce each individual radio button as required. The important thing is their radio button role is announced, because that's what conveys the fact that it has a single-selection behaviour for the group. The "required" constraint applies to the group as a whole, so it's fine to repeat this for each radio button. (When you change from one radio option to another, you are still technically on the same control group as far as assistive tech sees it, even if they are separate inputs in HTML terms).

It really isn't necessary to enforce a pre-selected default value for radio buttons. Most screen readers will announce them as "unselected, required" when they are first encountered. As soon as you select one explicitly using the spacebar (or maybe implicitly, using arrow keys; screen readers vary a bit in this) then they are announced as "selected, required" from that point onwards.

I agree that it's important to test with multiple screen reader and browser combinations, and I regularly do. I'm not sure what you mean by "keep them all happy"; that's a bit vague. The experience with each screen reader doesn't have to be identical, and numerous quirks are to be found. They don't all announce fieldsets the same way, or list the number of options. Some announcements are user-configurable. The important thing is that all the necessary information is conveyed in the accessibility tree. There are some combinations of browser and screen reader which have bugs beyond our control, and I don't think it's worth jumping through hoops to satisfy uncommon choices. I usually test IE11, Edge, Firefox, Firefox ESR, Safari, and Chrome browsers, alongside JAWS, NVDA, Narrator, Talkback, and macOS VoiceOver. I could do with more practice with iOS VoiceOver/Safari, which is a popular combination; it has lots of reported quirks and bugs, and has been a bit unstable between different iOS versions.

Updating proposed resolution and tasks.

Thanks for working on this everyone. This issue has taken some twists and turns, and isn't quite right yet. It's had some scope creep, and misunderstandings. Fiddly!

Firstly, my original instructions were faulty. I said we needed to put an aria-required attribute on the <input type"radio"> element. That turned out to be incorrect - @2pha and @jrockowitz caught it in #31-32. I've checked the ARIA recs, and I agree. The input element needs a required attribute, but not an aria-required attribute.

Re. #33-#34:

• <fieldset role="radiogroup"> is invalid. The HTML5.2 recommendation does not list radiogroup as a permitted role on the fieldset element.
• The example linked in #34 doesn't backup the use of role="radiogroup" on a fieldset element; the example there is using the radiogroup role on a DIV.
• I don't think we need a radiogroup role here at all. The purpose of that role is to identify which radio buttons form a group, so the accessibility tree can be correctly built. (Especially if the radios are something other than HTML input elements, like the <div role="radio"> in the example linked to).
• With proper HTML radios, the radio button button group is computed from the name attribute on the input element.
• Since the fieldset element isn't allowed a radiogroup role, it isn't allowed an aria-required attribute either. The default role for a fieldset is "group", for which aria-required isn't permitted. According to HTML5.2, the fieldset element is only allowed ARIA states and properties which would be allowed for the group or presentation roles.

Upshot: the fieldset element shouldn't have the radiogroup role, or aria-required.

Re. #37:
The fieldset element has a required attribute in that screenshot, which isn't valid HTML.

Testing patch #38:

Before patching:

• <fieldset> has aria-required="true". INCORRECT, not permitted on fieldset, remove it.
• <fieldset> has required="required". INCORRECT, not permitted on fieldset, remove it.
• <input type="radio"> doesn't have the aria-required="true" attribute. CORRECT, no need for it.
• <input type="radio"> doesn't have the required="required" attribute. INCORRECT, we need to add this.

After patch #38:

• <fieldset> has role="radiogroup". INCORRECT, not permitted on fieldset per HTML, remove it.
• <fieldset> has aria-required="true". INCORRECT, not permitted on fieldset, remove it.
• <fieldset> has required="required". INCORRECT, not permitted on fieldsest,remove it.
• <input type="radio"> doesn't have the aria-required="true" attribute. CORRECT, no need for it.
• <input type="radio"> has the required="required" attribute. CORRECT.

Needs work:

1. Remove the role="radiogroup" attribute from the fieldset, because it's not a permitted role, per HTM.
2. Remove the aria-required="true" attribute from the fieldset, because it's not permitted for the fieldset element's default role of group.
3. Remove the required="required" attribute from the fieldset, because it's not valid HTML.

@2pha - re. #31:

After trying to validate my page, I am being flagged for having aria-required set on radio buttons.

Good catch, thanks for reporting this.

After searching around the interwebs, it seems that a radiobutton group that is required should have a default value

This is NOT the case as far as I know. The WAI-ARIA recommendation (version 1.0 or 1.1) certainly doesn't state this. The HTML 5.2 recommendation is clear that it is permitted to have a group of radio buttons where none of them is in the checked state (emphasis mine):

Constraint validation: If an element in the radio button group is required, and all of the input elements in the radio button group have a checkedness that is false, then the element is suffering from being missing.

If none of the radio buttons in a radio button group are checked when they are inserted into the document, then they will all be initially unchecked in the interface, until such time as one of them is checked (either by the user or by script).

The part about "suffering from being missing" is relevant at validation time, but it doesn't mean that there must be a checked radiobutton. when the form is first rendered.

@bnjmnm - thanks for the big survey!

tagging, relevant for WCAG guideline 3.3 Input assistance.

A good one to work on at the Nashville sprints. Novices could try

- Get screenshots form Bartik + Seven, confirm no visual regression
- Get an assessment of whether this is safe enough to include in the Classy theme, to answer my question in the previous comment. Maybe track down a front-end framework manager and ask them in person :-)
- Write a functional test for the markup.

Review of patch #11. Good progress!

• Fixes code + comment standards issues noted in #10, great.
• Addresses the CSS TODO from #10. It works, so the asterisk appears after the fieldset legend, but not the individual radio inputs.
  

  +.form-radios .form-required:after {
  +  display: none;
  +}
    

  The .form-radios targets the div wrapper which around the radio options, but inside the fieldset.
  The .form-required class targets the individual labels inside the radios set.

The CSS works, and targets FAPI #radios only, so I think this would be fine if we put it in the CSS for Bartik and Seven themes.

But is it safe enough to include in the Classy theme, so that lots of themes benefit? This issue corrects an accessibility problem in markup at the form rendering level, but needs some CSS to avoid a visual regression in any theme inheriting form Classy. I think a more experienced CSS/front-end contributor should answer this better than I can. Leaving at needs review, tagging for others to find.

@mfairchild365 - thanks for the patch in #11.

A few tips for contributions generally:

• Include a issue-number and comment-number in the patch filename, to help reviewers be sure which one version they apply at the command line. Something like "2951317-11.patch". It really helps where issues go on beyond a few issues.
• Have you tried making interdiffs yet? This patch is small so it's easy to follow, but for larger patches, or issues where patches go though lots of changes, interdiffs help reviewers. It's a good thing to include, but a bit fiddly to learn. There's a guide at Creating an interdiff → . I've included one here. It shows how the comment wrapping was fixed, for example.

Thanks for working on this @mfairchild365.

Manual testing of patch #9:

1. The required + aria-required attributes are no longer on the fieldset element. Good.
2. Individual radio input elements get required and aria-required instead. Good.
3. Screen readers correctly announce that the radio is required, when focused on a radio. Good. (Tested with Chrome 65 + ChromeVox, Win7 + NVDA + Chrome 65, Win7 + NVDA + IE11, Win7 + NVDA + Firefox 52 ESR.)
4. Every radio gets a red asterisk. The patch doesn't touch CSS yet, so I was expecting this. Needs work.
   

Code Review of patch #9. Looking good so far, with a few minor issues:

1. diff --git a/core/includes/form.inc b/core/includes/form.inc
   index 052c9ce87a..05078d08bd 100644
   --- a/core/includes/form.inc
   +++ b/core/includes/form.inc
   @@ -222,6 +222,13 @@ function template_preprocess_fieldset(&$variables) {
        // Add the description's id to the fieldset aria attributes.
        $variables['attributes']['aria-describedby'] = $description_id;
      }
   +  
   +  //Remove invalid attributes from the fieldset for radio groups. These are applied to the child radio elements instead.
   

   The first line added here had some trailing spaces in it, which should be removed for coding standards.

2. +  //Remove invalid attributes from the fieldset for radio groups. These are applied to the child radio elements instead.
   

   Use a space after the the double slash to start the comment, and split this into two lines, wrapping at 80 characters.
   I think this comment could be worded better. The word "invalid" sounds ambiguous here, because that's like the name of one of the attributes. Perhaps "Remove unnecessary attributes ..."?

3. && 'radios' === $variables['element']['#type'])
   We don't generally see Yoda conditions in Drupal. Not actually part of our PHP coding standards yet, but being discussed in #2372543: [policy, no patch] Explicitly disallow yoda conditions → . Until that's resolved it's probably best to stick to the non-Yoda way.

4. +    unset($variables['attributes']['aria-invalid']);
   

   Good catch. I think this is appropriate. AFAIK aria-invalid doesn't work on fieldset elements.

TODO: update CSS to fix the issue of too many asterisks. This comes from form.cssin Classy theme, so let's try updating that, to avoid adding the asterisk it to required radios which are inside fieldsets (not sure how specific it will need to be). The framework maintainers might say no to changing Classy, in which case we can put separate overrides in Bartik and Seven. But let's try updating Classy CSS first for simplicity.

andrewmacpherson → created an issue. See original summary → .

@jwilson3 - re: #25 - can you make that comment in the #2913321: Proposal to experiment with React for building Drupal’s administrative UIs → ? That sounds like the kind of concern they want to hear about.