Do not use a CSS-only required marker in forms per WCAG 2.0

Thanks @DuaelFr - this is an interesting issue and one that we obviously didn't fully consider when making this change.

I think that part of this was the confidence that we are already providing a lot of semantic information for assistive technology users.

So in this case the semantics should be covered by both required="required" aria-required="true":
<input class="js-text-full text-full form-text required" data-drupal-selector="edit-title-0-value" id="edit-title-0-value" name="title[0][value]" value="" size="60" maxlength="255" placeholder="" required="required" aria-required="true" type="text">

It is also interesting that the text is a symbol '*' rather than a word like '(required)'. So the meaning is already symbolic in the Drupal 7 way of addressing this.

I am pretty sure that there hasn't been any testing done without any CSS, I do wonder what other errors we'll catch by doing so. I do wonder though if perhaps this is an overly generalized guideline. Are people finding this a barrier? Is disabling CSS still relevant? It's probably more common to disable JavaScript, but there's lots that you can't do in any modern system without enabling it.

Are people in D8 theming the required element more now? Would everyone just be as happy if we'd kept it like we did in D7?

Why speak: none? aria-hidden should be enough to hide it from AT.

I can't find any info about speak:none implementation. I'd forgotten about the CSS speech properties. I don't think I've heard them being recommended any time recently.

How about just "required", to keep the wording short?

So we could just add something like this:

<label for="edit-account-name" class="js-form-required form-required">Username <span class="hidden">(required)<span></label>

So that if there is no CSS, the HTML would look like "Username (required)". If CSS is read then it would be hidden. No need to use any aria-hidden elements at all.

That work?

Yes, but in the case of a screen reader user disabling CSS they would still get very clear verification that this was a required field and should sound like - "Username (required)". I'm not sure what the aria adds. If CSS is disabled they wouldn't get the '*' but they would get the text '(required)' which is good if not better.

#11 is on the right track.

I think it should work well for sighted screen reader users too. With CSS they will see an asterisk, and hear "required". Without CSS they see and hear "required" (in English, anyway).

There might a localization issue here. What if an operating system or screen reader localization announces the required attribute using one phrase, but Drupal localizes "required" as a different phrase? Like the difference between "mandatory" and "compulsory". I've never used a screen reader in any language other than English, so I don't know. It's beyond our control though.

A minor point: the span doesn't need to be inside the label - it won't form part of the accessible name for assistive tech either way. For all the scenarios in #11, the following pattern also works:

<label for="edit-account-name" class="js-form-required form-required">Username</label><span aria-hidden="true" class="hidden"> (required)<span>

The difference between the two is that in #11 the word required is clickable, but in this version it isn't clickable.

I don't mind either way, so let's put the span inside the label as a safer default.

In D8 I think this goes in form-element-label.html.twig with no need for FAPI #prefix. Themers can still move it outside the label, or move it up to the form-element.html.twig template if they like. Something like:

{% if title is not empty or required -%}
  <label{{ attributes.addClass(classes) }}>{{ title }}{% if required %} <span class="hidden" aria-hidden="true">{{ "(required)"|t }}</span>{% endif %}</label>
{%- endif %}

We should probably include a comment to document why it's there for accessibility.

Coming along nicely! @andrewmacpherson & @DuaelFr

Thanks DuaelFr. The approach in patch #16 looks good, does what we discussed so far.

However we're not normally allowed to change templates in Stable and Classy themes during minor releases. Tagging for Stable and Classy maintainers - we've made changes to Stable to fix accessibility bugs before, so maybe this will qualify. Otherwise, we'll have to copy the Classy template to Bartik and Seven, and leave the Stable and Classy templates unchanged.

Nitpick: there's an extraneous space here we could eliminate. The one between the opening SPAN and the opening twig {{

<span aria-hidden="true" class="hidden"> {{ '(required)'|t }}</span>

The templates could be more readable if we split the "if required / endif" accross several lines, and used the whitespace collapsing form of braces: {{- if required -}}. See Whitespace control in the Twig manual.

Before going ahead with this, it would be good to test with a wide variety of browser + screen reader combinations. Screenshots of with and without author CSS applied will help here too.

If we aren't allowed to change Stable/Classy, a change record will be a good way to recommend this approach for contrib/custom themes.

Overall, the progress is looking great!

The extra space - ignore me. Now I see that only one of those spaces is actually output. That's fine, the output is correct.

We're still updating Stable and Classy templates though. I'm expecting the front-end framework managers will say no to that.

Why use .hidden class and not .visually-hidden ?

Re: #23 and #24 - This is about avoiding verbosity in assistive tech, e.g. screen readers announcing "required" twice. The use of aria-hidden="true" is sufficient to ensure that span is hidden from AT, regardless of whether author CSS gets applied or not. So in this sense it doesn't really matter whether we use .hidden or .visually-hidden.

I think we should stick with .hidden for a couple of reasons:

1. .hidden will effectively hide it from legacy browser/AT combinations which don't support aria-hidden, but .visually-hidden won't.
2. .hidden is a much simpler (safer) implementation than .visually-hidden. It just uses display:none instead of all the properties needed for the 1px clip technique. Note that the .visually-hidden CSS itself needs ongoing review for nest practice. (See #2819823: CSS clip deprecated - .visually-hidden needs an update?.)

We'll need to address this in fieldset.html.twig too, to achieve the same effect for required FAPI radios and checkboxes.

Some core forms where you test this:

• Enable the Book module. Visit admin/structure/book/settings, which has required '#type' => 'radios' and '#type' => 'checkboxess'.
• Enable the experimental Content Moderation module. The Add/Edit Transition form has required text, radios, and checkboxes. Visit admin/config/workflow/workflows/manage/editorial/add_transition.

The patches so far have modified templates in Stable and Classy. We'd like to know if this will be allowed, or whether to update Seven and Bartik instead.

I noticed a similar pattern in the wild, on Expedia's forgot-password form.

Here's a screenshot of the form and markup, from Firefox's dev tools. It's not exactly the same as proposed here, but works on similar lines. The main difference is that they are using a visually-hidden approach to the "required" text hint, and are not using aria-hidden. A screen reader says the word "required" twice; from the label text, and from the required HTML attribute.

Without CSS, the label reads "Email addressrequired", without any space between words. Not bad, but our patch is already doing better:

I'm glad to see other people are thinking along the same lines as this issue. The patch here is a bit higher quality than Expedia's implementation, in my view.

Just tested core_accesible_form_labels-2921627-42.patch on Drupal Core 8.6.0-rc1 and it looks like it's working.

on my side i can apply #42 patch agains 8.6.x branch

Re-rolling this for 8.8

Will this patch apply the correct markup when the required attribute is toggled via the JS states API?

Re-roll #47 patch for latest 8.9.x

This patch makes 'required' #states add the right markup.

Under certain conditions the marker gets added multiple times.

Bumping to major, because some of the relevant WCAG success criteria are at level A. In particular, the presence of the "required"/asterisk counts as "3.3.1. labels and instructions". If you don't know it's required, you don't have full instructions.

I was chatting with @rainbreaw about another potential use for this is in Windows high-contrast mode. Instead of trying to get an asterisk to display in :after, Windows high-contrast mode could use the actual required text (i.e. don't hide it).

Fixed test, Please review.

Please ignore my previous patch, fixed test, Please review.

#59 , I've applied this patch and is working properly. I've attached the before and after screenshots below.
Before patch,

After patch,

1. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
   @@ -240,7 +240,7 @@ public function testDateField() {
   -    $field_label = $this->field->label();
   +
   
   @@ -249,7 +249,7 @@ public function testDatetimeField() {
   -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_label, 'Fieldset and label found');
   +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend/text()', NULL, 'Fieldset and label found');
   
   @@ -413,7 +413,7 @@ public function testDatelistWidget() {
   -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_label, 'Fieldset and label found');
   +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', NULL, 'Fieldset and label found');
   
   +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
   @@ -71,7 +71,7 @@ public function testDateRangeField() {
   -      $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_label, 'Fieldset and label found');
   +      $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', NULL, 'Fieldset and label found');
   
   @@ -298,7 +298,7 @@ public function testDatetimeRangeField() {
   -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_label, 'Fieldset and label found');
   +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', NULL, 'Fieldset and label found');
   
   @@ -477,7 +477,7 @@ public function testAlldayRangeField() {
   -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_label, 'Fieldset and label found');
   +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', NULL, 'Fieldset and label found');
   
   @@ -662,7 +662,7 @@ public function testDatelistWidget() {
   -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_label, 'Fieldset and label found');
   +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', NULL, 'Fieldset and label found');
   

   Why do we need these changes?

2. +++ b/core/misc/states.js
   @@ -356,9 +356,14 @@
   +          if (!$label.find('.js-required').length) {
   +            $label.append($('<span class="js-required hidden" aria-hidden="true">' + Drupal.t('(required)') + '</span>'));
   +          }
   ...
   -        $(e.target).removeAttr('required aria-required').closest('.js-form-item, .js-form-wrapper').find('label.js-form-required').removeClass('js-form-required form-required');
   +        $(e.target).removeAttr('required aria-required').closest('.js-form-item, .js-form-wrapper').find('label.js-form-required')
   +          .removeClass('js-form-required form-required')
   +          .find('.js-required').remove();
   

   Can we add test coverage for this?

3. We need to apply these changes to stable9 too
4. We should do manual testing and screenshots on all core themes to ensure there are no regressions

5. +++ b/core/misc/states.js
   @@ -356,9 +356,14 @@
   +            $label.append($('<span class="js-required hidden" aria-hidden="true">' + Drupal.t('(required)') + '</span>'));
   

   We should add a theme function for this

Rerolled the patch for 9.2.x

Status to 'Needs review' to verify the reroll did not introduce errors.

This patch needs Reroll, as it has changes in /core/misc/states.js file but /core/misc/states.es6.js file is not updated.

Update es6 file then compile the code with yarn.

For #66.

Addressed #66, Please have a look.

Bug fixes are against the latest version and the patch does not apply to 9.3.x. :-(

Rerolled the patch in #68, thanks!

Fixed CS error.

Hopefully we can get this patch re-built and bring this change into Core.

I have attached the patch for 11.x-dev. As in the earlier patch #75, Classy, stable and bartik were updated. which are no longer part of core. I have updated the latest themes. please review

Alternatively, there's this approach https://adamsilver.io/blog/how-to-highlight-required-and-optional-form-f...

Just mark the optional fields. This would be a pretty huge change in the forms design though. Definitely a major release change, but D11 could be it.

Removed some unrelated changes and added new tests

This is having to update existing templates. Is that needed?

If there really is no other way then a change record will be needed.

Adding reference to Windows High Contrast Mode.

Looks like STR is still needed.

Adding Novice tag so novices can find this for those todo items in the IS.

Are tests still needed, or can that todo item be marked as done?

This point about WHCM (aka, forced-colors) by @andrewmacpherson in #56 hasn't been addressed. Is that intentional?

I was chatting with @rainbreaw about another potential use for this is in Windows high-contrast mode. Instead of trying to get an asterisk to display in :after, Windows high-contrast mode could use the actual required text (i.e. don't hide it).

It looks like point #4 in #62 by @laurii hasn't been addressed. I don't see the screenshots from all core themes, at least. I'd say Gin should also be tested b/c it's in CMS and going into core.

We should do manual testing and screenshots on all core themes to ensure there are no regressions

Did the other points get addressed? I didn't see a reference to that comment in subsequent changes.

From a manual testing standpoint this looks good across themes - tested in Claro 11.x, Gin 5.0.x, and Olivero 11.x. Instead of screenshots I went with short videos to compare with and without CSS to illustrate the visual output as well as the output of the the aural interface.

The only detail I wonder is something we've raised in #2852874-24: [Meta] Set the required state for all fields in Core in the Accessibility Object Model (AOM) (see point 2). As you can see and hear in the three videos, the star is getting announced in the aural interface alongside the announcement of the required attribute, strictly speaking that is redundant information - basically distracting noise. so i wonder if it would make sense to fix that detail within the scope for this issue? cuz the linked issue is only about the file field type and would have required a followup issue for in general anyway? or should it be moved to a dedicated followup?

I don't think we need screenshots any more, as the video @rkoller produced should be fine

We still need:

• Needs change record
• Needs steps to reproduce

awesome thanks so much @duaelfr for the fast incorporation of that additional fix! i did another round of manual testing and recorded again for claro, olivero, and gin in safari 26.2 on macos 26.2 and in addition to that i also tested in the latest version of firefox and edge - using drupal 11.x and for testing with gin, gin 5.0.x with gin_toolbar installed.

claro and olivero look perfect to me in testing and the recorded videos. for firefox and edge i had the exact same results. perfect from my point of view. the only thing in gin the latest changes in regard to the star excluded from the aural interface are not available (the rest of the fixes still are still there as you can see ). not sure if something is missing for this MR or in case of the exclusion of the star from the aural interface an additional fix in gin would be necessary?

There were some random test failures that passed when re-run. Pipeline is green.

@rkoller

in gin the latest changes in regard to the star excluded from the aural interface are not available (the rest of the fixes still are still there as you can see ). not sure if something is missing for this MR or in case of the exclusion of the star from the aural interface an additional fix in gin would be necessary?

AFAICT that star comes from Gin's CSS (from a quick inspection in the browser with Gin 5.0.10).

ah nice thanks! so if the star comes from gin that means a followup in the gin queue would be necessary to get en par with claro and olivero. then this issue would be good to go, isnt it?

@rkoller

so if the star comes from gin that means a followup in the gin queue would be necessary to get en par with claro and olivero. then this issue would be good to go, isnt it?

I agree that a followup for Gin is needed.

If you create the issue and the STR, I'm happy to add a screenshot that shows the CSS for this MR being overridden by Gin. I'm also happy to detail that in the proposed resolution, if that's helpful.

@kentr i've opened #3566851: [PP-1] Avoid announcing the star for required fields in the aural interface . feel free to update the issue summary in case i've missed anything or phrased it in a to complicated manner. and at first i thought it might be good to postpone the followup on this issue but strictly speaking it could run in parallel if gin is overriding Claro CSS anyway?

ok after quite a few discussions with @kentr and @mgifford we came up with the following approach. first a bit of context. this issue would be ready to go. problem is due to the introduction of the fix for removing the start from the aural interface based on my suggestion in https://git.drupalcode.org/project/drupal/-/merge_requests/12981/diffs?c... this issue would be blocked by #2852874: [Meta] Set the required state for all fields in Core in the Accessibility Object Model (AOM). by removing the star fields that dont have a programmatic required label would be unidentifiable for screenreader users. :( and it turned out in my testing that not only the field is missing the required attribute but also:

• timestamp
• formatted text long
• formatted text long sum
• image
• comment

so our consensus was the following:

• revert this commit https://git.drupalcode.org/project/drupal/-/merge_requests/12981/diffs?c... on this MR. after that this issue looks good and ready for RTBC. the sooner the better it gets in.
• the changes from the reverted commit should go into the following issue instead #3568100: [PP-1] Avoid the announcement of the star for required fields in the aural interface. i've already outlined the necessary changes made on the issue and also noted to credit @duaelFr. for gin i've also created an issue before introducing the same changes #3566851: [PP-1] Avoid announcing the star for required fields in the aural interface .
• Both issues are postponed now on #2852874: [Meta] Set the required state for all fields in Core in the Accessibility Object Model (AOM), and the scope of that issue is getting expanded not only adding a required attribute to the file field but also to all other fields in core that are missing a required attribute.

so to summarize the sole step relevant for the current issue is roll back https://git.drupalcode.org/project/drupal/-/merge_requests/12981/diffs?c... , do a last recheck, finish the change record, and then this issue should be ready for RTBC imho- apologies for the back and forth :(

@duaelfr. thank you for reverting the last commit and apologies again for the back and forth. i just went ahead and retested the latest state of the MR. i've noticed one detail I haven't thought of in my previous test that raises one question. I have tested now a node edit form that contained a file and an image field around others that all were set to required. those fields are wrapped in field sets and the label of those field sets have stars too. The question is should those fieldset labels also replace the star with (Required)? At the moment only the star is not displayed anymore with CSS unavailable:

Appears to have some test failures.

Also will need a change record for the template changes.

Thanks!

I'll look into the test failures.

The test failures in the previous pipeline were random.

As part of a rebase, I omitted core/modules/contact/tests/src/Functional/ContactSitewideTest.php in the original patch because it has since been removed in main.

Concern regarding tests:

#38 says

I did not find tests for datetime_wrapper so it stays out of coverage (maybe we should consider not altering these templates).

However, the MR has changes to several instances of datetime-wrapper.html.twig.

Some of them were added in #84, but I can't tell from the patch if it adds coverage directly related to datetime-wrapper.html.twig.

Looking into it further, but for now +Needs tests.

Looks like the changes need to be applied to the new Admin theme.

I will look into that, also.

Other concerns:

1. Regarding fieldset (#104 #107) and the file and image fields in the screenshots

Currently, in Claro, those aren't fieldset elements. They're details / summary elements.

However, the MR does add the "(required)" text for fieldset elements.

So the question is, do we actually want that for fieldset elements? Because actual labels for individual fields also have "(required)".

I'm seeing this also on required Datetime fields in the Stark theme, which uses the base system template that arbitrary themes might use if they don't override it.

2. Regarding datetime-wrapper.html.twig

Per #111, several instances of datetime-wrapper.html.twig are modified.

These changes are evident with required Timestamp fields, again in Stark theme.

To me, the change creates a problem. It adds the "(required)" text to h4 elements above the actual input fields, and the fields themselves also have "(required)". So, my thought is that datetime-wrapper.html.twig should not be modified for this issue.

What are thoughts on these points?

[edited to fix typos & improve formatting]

Here's a side-by-side screenshot of required datetime, timestamp, and file fields with and without CSS in Claro.

Now it makes more sense why the required marker (aka the asterisk, or "star") was added for the legend and the h4 in the datetime wrapper: the individual field labels are visually hidden.

Olivero and Admin (the theme formerly known as Gin, or TTFKAG) also do this.

However, since the individual fields do have the text "(required)", as seen in the right side of the screenshot, it seems unnecessary to also add "(required)" to the legend and the h4 in the datetime wrapper.

Disregard my comments about changing fieldset.html.twig and datetime-wrapper.html.twig. I had forgotten about #26, and I understand the date fields better now.

I'll add detail to the IS.

IMO it also needs before / after screenshots of radios, checkboxes, and the different date/time field types. I will add these.

I think a followup is required for timestamp fields. My sense is that they should actually be in a fieldset because they consist of two related fields.

1. Regarding #107, the MR had changes to summary elements, but they had not been done in Claro or Olivero yet. Now they are.
2. I added tests for legend and summary.
   The others I saw in the MR, in \Drupal\Tests\system\Functional\Form\ElementTest::testRequiredFieldsetsAndDetails(), only apply to the starterkit_theme. It looks like applying that test file to starterkit_theme was intentional for #3304731: Update remaining tests using Classy to use Starterkit , so I left it as-is.
3. I added tests for datetime-wrapper.html.twig.
4. I updated the Admin theme templates.

Forgot to mention that I also added test coverage for datetime-wrapper.html.twig.

Attaching before / after screenshots with more fields in Claro, Olivero, Admin theme. As these are for visual regression checks, not to demonstrate the changes, the left & right sides of each should be identical.

[edited for clarification]

created again another content type containing every available field in core and created with and without css screenshots on the node edit form with claro, admin, and olivero, plus the same for the login page.

over all this looks good to me from a manual testing perspective, excellent work @kentr thank you! the only detail i consider odd are the stars on the fieldsets and then on the fields within that fieldset again - potential scenarios, sometimes a single field, sometimes several, sometimes every field within. but that is something that could be solved within #3174459: [PP-1] W3C Validation: required attribute not allowed on fieldset tag . that is out of scope for this issue imho.

added some context to the before and after screenshots section in the issue summary

@rkoller

over all this looks good to me from a manual testing perspective, excellent work @kentr thank you! the only detail i consider odd are the stars on the fieldsets and then on the fields within that fieldset again - potential scenarios, sometimes a single field, sometimes several, sometimes every field within. but that is something that could be solved within #3174459: [PP-1] W3C Validation: required attribute not allowed on fieldset tag . that is out of scope for this issue imho.

In your opinion, can we mark it as RTBC after the draft change record is added?

Still needs a change record.

If anyone thinks that's doable by a novice, please add the Novice tag.

I'm working on a change record.

I also want to confirm that all of #62 has been addressed. In particular, I'm not certain that point 5 was addressed.

Think it needs some additional assertions for JavaScript states.

Added STR for cases using the JavaScript states API.

There are cases where the space is missing between the "(required)" text and the main part of the accessible name. Here's a screenshot showing that.

Added a draft CR. It needs some work.

Updated the CR with before / after examples for template modifications.

Working on the other code and test changes.

Addressed the code items in Remaining tasks.

I think it needs additional manual testing just for the JavaScript states API. See the Steps to reproduce for details.

I just pushed the update that I meant to push before my last comment...

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.

I'll rebase it.

Rebased and passing.

Just a drive by here (sorry!).

I haven't read through the full thread, but I did look at the MR.

There may be an easier way to do this. We could do something like this with CSS only:

.form-item::after {
  /* Element content and "required" are read out */
  content: "*" / "required";
}

See https://www.stefanjudis.com/today-i-learned/css-content-accepts-alternat...

...but it wouldn't be translatable (unless we add it via JS)

@mherchel

There may be an easier way to do this. We could do something like this with CSS only:

Thanks for looking!

Were you saying to use CSS only instead of adding actual text in the markup?

If so... One of main goals is to have the textual "required" present in cases where CSS is disabled. That's what the "not use full CSS" refers to...

TBH, I think my idea is dead in the water because of lack of translation. If we went with JS, it would add a lot of complexity.

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.

I'll rebase it.

Rebased and passing.

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.

Looks like the tests still pass? I've checked out MR12981 and went through a node edit form containing all available field types in core another time. Tested that node edit form with Claro, Default Admin, and Olivero - all looked good to go. The output of the required label on fieldsets will be covered in #3174459: [PP-1] W3C Validation: required attribute not allowed on fieldset tag . A Change Record that looks good and complete is available at https://www.drupal.org/node/3580406 and the issue summary is also in a good shape in general. Not sure why the need review bot reverted the issue back to needs work? From my perspective in case there isnt anything ive missed in regard to failing tests, this issue looks good to go and ready for RTBC?

I'll rebase it.

I believe it still needs manual testing for the JavaScript states case (#130).

^^ See the Steps to reproduce for details.

Good point, thanks for the reminder, missed that detail. I've quickly tested for Javascript States pointed to in #148... Works in latest Safari, Firefox Developer Edition, and Edge:

The rest still looks good to go as well. Setting back to RTBC

Added a11y-bugsmash tag.

Removing the 'Drupal should' from the title because that is redundant since this is the Drupal core queue.
The change record clearly shows what developers/themers should for the templates changes, which is great to see. I moved some text around at the beginning to put the change first, which is the preferred style.

I updated credit.

I don't mean to rock the boat, but now that I've seen a mechanism for appending text to labels / names without modifying templates, I'm wondering if that's a better approach.

Adding the '(required)' text in templates means that existing sites with custom templates have to update their templates, and also carries the risk that it will get accidentally removed from custom templates during their lifespan.

As an alternative, it may be possible to add the text in a ::preRender(), similar to how it is done for #2848507: Indicate that grouping elements have child element errors for ux and a11y:

$element['#title'] = new TranslatableMarkup('@title <span class="visually-hidden child-error-indicator">(contains error)</span>', ['@title' => $element['#title']]);

I also wonder if translating the full string like this instead of just the '(required)' text by itself yields a better translation result for some languages.

I'm curious to hear others' opinions.

I think it's worth exploring #154, if we can handle it centrally that will save updating a lot of templates, work for contrib, and agreed it's likely to be a lot better for translation. I also think appending (required) won't necessarily work for RTL languages - was that tested?

uhhh damn, good point about RTL, at least i had forgot testing that as well. i have caught up on that oversight of mine and just tested with the current state of the MR. just switched to hebrew and the (Required) tag is to the right of for example the username label, that should be on the left instead

Kentr asked about this on the side and agree with a non template approach.

I'll work on the changes.

I tested before with a Hebrew translation file imported manually via the languages UI, and again just now with the current MR.

The text "(required)" is to the left of the Hebrew, between the input and the Hebrew.

Here's a screenshot of the user edit page with Hebrew set as the default language. It should be the same fields as in @rkoller's screenshot, I think, but a Hebrew reader would have to confirm.

I can't say why English in RTL doesn't work this way in @rkoller's screenshot. It might have something to do with the difference between a language and a script. My understanding is that languages aren't RTL, but rather it's the script that is RTL.

The MR with the new strategy is for review. I'm hiding the other MR for now.

I think it would be best to get approval on the general code strategy before more manual testing:

1. Add a #pre_render callback method to FormElementBase.
   This method appends "(required)" to #title when the criteria are met.
   Use a #pre_render callback because AFAICT it must be done after validation so that the unmodified #title is used for validation error messages.
2. With all render elements for HTML form inputs, automatically add this callback to the list of the element's callbacks in FormBuilder::handleInputElement().
   These are elements which for which $element['#input'] is TRUE.
3. With non-input render elements such as Details and Fieldset, add the new #pre_render callback explicitly in the element's ::getInfo() method.

Other notes

• There's a standalone Label render element, and I added code for it to work. I think it was previously working because it uses the same template as automatic labels.
• In states.js, I changed the logic a little to use a data-drupal-required attribute on the span instead of the CSS class js-required.
  This is because I wanted to make the PHP and JavaScript versions the same in order to have only one version of the string for translation, and it seems inappropriate to add js-required to an element in PHP.
  It also seems less likely that someone will remove the data-drupal-required attribute than a js-required class.
• I did a rough pass to test translation.
  The string shows up in the translation interface as @title <span aria-hidden="true" class="hidden" data-drupal-required> (required)</span>, which sounds like what we want based on the translation documentation.
  Here's screenshot of the login form with a translation entered for Hebrew (CSS is disabled).
  
• I rebased both MRs to make comparing them easier and also attached an interdiff between the them.

[edit: added a link to relevant code in the MR]

Tagging for subsystem maintainer review to get feedback on the strategy in #160.

Also uploading the image again. It doesn't appear to have saved.

not a forms maintainer but like this approach a lot better then before!

Left a comment on the option 1 MR as to why I personally prefer that one.

@larowlan, thanks for the review and the comments on the MR. I replied in the MR regarding the drawbacks that I see on Option 1.

Also WRT drawbacks, @catch said this about a non-template approach in #155:

I think it's worth exploring #154, if we can handle it centrally that will save updating a lot of templates, work for contrib, and agreed it's likely to be a lot better for translation. I also think appending (required) won't necessarily work for RTL languages - was that tested?

Not sure what's next for this.

It looks like @larowlan didn't have any concerns about the code in the MR for Option 2.

Option 1 would be RTBC except for #156. However, in my testing (#158) the new text was to the left of the main label as expected. I don't know if that satisfies #156 and qualifies for RTBC.

I rebased the MR for Option 1 (https://git.drupalcode.org/project/drupal/-/merge_requests/12981).

I suspect further manual testing should wait until it's clear which approach we're taking.

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.

I'll rebase it.

Rebased and passing.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Linking some examples from slack that the PHP fix would break:

* https://github.com/civictheme/uikit/blob/main/packages/twig/components/0...
* https://github.com/previousnext/mxds/blob/main/src/Form/Label/label.twig...

Discussed in slack @rikki_iki said:

I think I'd lean towards php, twig feels the natural place but the idea of there being no marker feels worse than some having two. ie two is more obvious and likely to get fixed, none could go unnoticed for some time

She makes an excellent point about it being noticeable - so I've changed my mind - let's press ahead with the PHP approach and just note the change in a release note for themers.

Rikki also pointed out we'd need to be able to customise the class.

Left a comment about making the required marker markup overidable by a theme per comment from @rikki_iki

Thanks @larowlan.

The MR was pretty behind, so I rebased it.

I made it themeable.