Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#47975 closed defect (bug) (fixed)

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

Reported by: afercia Owned by: joedolson
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests
Focuses: accessibility, template Cc:

Description (last modified by 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.

Attachments (4)

47975.diff (977 bytes) - added by dinhtungdu 2 years ago.
47975.2.diff (975 bytes) - added by dinhtungdu 2 years ago.
47975.3.diff (870 bytes) - added by dinhtungdu 2 years ago.
47975.4.diff (2.2 KB) - added by donmhico 2 years ago.
Refreshed patch + unit tests.

Download all attachments as: .zip

Change History (16)

#1 @afercia
2 years ago

  • Summary changed from Prevent aria-described broken reference in `comment_form()` to Prevent aria-describedby broken reference in `comment_form()`

#2 @afercia
2 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 years ago

#4 @joedolson
2 years ago

  • Owner set to joedolson
  • Status changed from new to accepted

#5 follow-up: @joedolson
2 years ago

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
2 years ago

@dinhtungdu
2 years ago

#6 in reply to: ↑ 5 ; follow-up: @dinhtungdu
2 years ago

  • Keywords has-patch added

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.

#7 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 5.3

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.

#8 in reply to: ↑ 7 @dinhtungdu
2 years ago

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.

@dinhtungdu
2 years ago

@donmhico
2 years ago

Refreshed patch + unit tests.

#9 follow-up: @donmhico
2 years ago

  • Keywords has-unit-tests added

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

#10 in reply to: ↑ 9 @SergeyBiryukov
2 years ago

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!

#11 @donmhico
2 years ago

@SergeyBiryukov Oh no problem :)

#12 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.