WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 10 days 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 weeks ago.
47975.2.diff (975 bytes) - added by dinhtungdu 2 weeks ago.
47975.3.diff (870 bytes) - added by dinhtungdu 12 days ago.
47975.4.diff (2.2 KB) - added by donmhico 10 days ago.
Refreshed patch + unit tests.

Download all attachments as: .zip

Change History (16)

#1 @afercia
2 weeks ago

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

#2 @afercia
2 weeks ago

  • Description modified (diff)

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


2 weeks ago

#4 @joedolson
2 weeks ago

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

#5 follow-up: @joedolson
2 weeks 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 weeks ago

@dinhtungdu
2 weeks ago

#6 in reply to: ↑ 5 ; follow-up: @dinhtungdu
2 weeks 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
12 days 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
12 days 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
12 days ago

@donmhico
10 days ago

Refreshed patch + unit tests.

#9 follow-up: @donmhico
10 days 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
10 days 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
10 days ago

@SergeyBiryukov Oh no problem :)

#12 @SergeyBiryukov
10 days 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.