Prevent aria-describedby broken reference in `comment_form()`

[afercia]

The comment_form() generates a complete commenting form, mainly used by themes.

The email field markup has an aria-describedby="email-notes" attribute that targets a description. Actually the description is the default comment_notes_before argument and outputs the following HTML (simplified code):

<p class="comment-notes"><span id="email-notes">Your email address will not be published.</span></p>

However, it is possible to pass the comment_notes_before a completely different HTML (which may not contain the email-notes ID) or an empty string to completely omit the description. See for example the comment form on this page:

​https://andersnoren.se/themes/chaplin/2014/08/03/babela-armchair/

It's understandable developers are misled: the comment_form() function allows to do this, while it should ensure semantic markup. I'd tend to think this is a core responsibility that can't be delegated to the developers attention to details.

In these cases, the aria-describedby attribute points to a broken reference. This should be avoided because the attribute communicates user agents and assistive technologies there's a description but then the referenced description is missing. It will be flagged as failure by any accessibility automated checker tool and, most importantly, is potentially confusing for assistive technologies users.

Related standard:
WCAG Success Criterion 1.3.1 Info and Relationships
​https://www.w3.org/TR/WCAG21/#info-and-relationships

Ideally, the aria-describedby attribute should be printed out only if there's a description and only if the targeted ID actually exists. One option could be checking for a non empty comment_notes_before and wrap an element with ID email-notes around its output.

[joedolson]

Need to check for ‘email-notes’ within the description string. If they’re not present, but the string is non-empty, add the id to the wrapper and print the aria-describedby attribute. If the string is empty, don’t print the attribute or add any wrapper. May also need to check for the presence of an alternate wrapping element, as custom text might already include a <p> element.

[dinhtungdu]

I use regex to match the valid and non-empty #email-notes. The regex information and testing can be found here: ​https://regex101.com/r/VPv2tw/8/.

Is using regex overkill for this ticket?

The simpler version should be check for the presence of id="email-notes" in the comment note, then the theme developers are responsible for keeping the comment note valid.

Replying to joedolson:

Need to check for ‘email-notes’ within the description string. If they’re not present, but the string is non-empty, add the id to the wrapper and print the aria-describedby attribute.

I don't think we should add the wrapper if the string is non-empty, because the comment note can contain other information than email description.

[SergeyBiryukov]

Replying to dinhtungdu:

Is using regex overkill for this ticket?

The simpler version should be check for the presence of id="email-notes" in the comment note, then the theme developers are responsible for keeping the comment note valid.

Thanks for the patch! That regex does seem like an overkill here, I think just checking for id="email-notes" should be enough.

Replying to joedolson:

Need to check for ‘email-notes’ within the description string. If they’re not present, but the string is non-empty, add the id to the wrapper and print the aria-describedby attribute.

I don't think we should add the wrapper if the string is non-empty, because the comment note can contain other information than email description.

I tend to agree. Looking at the ​themes from the directory overriding comment_notes_before with a non-empty string, some of them have the "Your email address will not be published" string without id="email-notes", however there's no reliable way to check that the string indeed refers to the email field.

One example would be the ​Yoko theme, which adds Required fields are marked * as comment_notes_before, but that's not a description for the email field.

[dinhtungdu]

Replying to SergeyBiryukov:

Thanks for the patch! That regex does seem like an overkill here, I think just checking for id="email-notes" should be enough.

Okay. I will make another patch to make it easier to discuss/test.

[donmhico]

Refreshed patch + unit tests.

[donmhico]

Thanks for the patch @dinhtungdu. Your solution works. However I wasn't able to apply the patch cleanly against the trunk at this time of writing so I refreshed the patch. Are you using the latest trunk? Also, I added unit tests for the patch

[SergeyBiryukov]

Replying to donmhico:

However I wasn't able to apply the patch cleanly against the trunk at this time of writing so I refreshed the patch. Are you using the latest trunk?

It's my fault :) Some parts of the function were reformatted in [46088] for better readability.

Thank you for the refreshed patch!

[donmhico]

@SergeyBiryukov Oh no problem :)

[SergeyBiryukov]

In 46090:

Accessibility: Comments: In comment_form(), remove aria-describedby attribute from the email field if there's no associated description in the comment_notes_before argument.

The attribute is meant to communicate to user agents and assistive technologies that the element has a description. If the referenced description is missing, it will be flagged as failure by any automated accessibility checker tool and, most importantly, is potentially confusing for assistive technologies users.

Props afercia, joedolson, dinhtungdu, donmhico, SergeyBiryukov.
Fixes #47975.