Problem/Motivation

The body field summary textarea indicates it has a description with an aria-describedby attribute, but the DOM id value points to a non-existent node. Here is the code:

<textarea class="text-summary form-textarea resize-vertical text-summary-processed" aria-describedby="edit-body-0-summary--description" id="edit-body-0-summary" name="body[0][summary]" rows="3" cols="60"></textarea>

This is a problem for users using a screen reader. Without a description, it is not apparent what the field is for.

To repeat:

  • Ensure the WYSIWYG is enabled
  • Add a description text
  • Check the <textarea> for the body and search for the aria-describedby which should point to the description ID
  • Check if there is an id for the description or if it looks like <div class="description">example</div

Proposed resolution

It seems like the description text for this field was simply not added or that the textarea should have an ARIA aria-labelledby attribute that points to the field label ID.

Remaining tasks

  1. Propose an initial patch.
  2. Review/fix initial patch.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Issue tags: +Accessibility, +Novice
SGhosh’s picture

Tested using ChromeVox screen reader. Tested on summary textarea of create new page form -

  • when nothing is given chromevox is reading the label - how?
  • when both aria-labelledby and aria-describedby are given description element id, chromevox is reading the label
  • when aria-describedby is given the description element id, chromevox is reading the description

Qs -
* Will both the above attributes be required then?
* When nothing is given how is the screen reader reading the label
* For using these attributes the id of the element to be used has to be given; but label only has for attribute. Will we need to add id to label element to provide it as label attribute to textarea for use by screen reader?

SGhosh’s picture

SGhosh’s picture

Wrong comment no. in patch name - corrected.

mgifford’s picture

What's different between textarea_accessibility_by_screenreader-2126761-4.patch & textarea_accessibility_by_screenreader-2126761-2.patch

Seems like a simple fix, just adding:
+ $element['#attributes']['aria-labelledby'] = $element['#id'] . '--description';

jviitamaki’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: textarea_accessibility_by_screenreader-2126761-4.patch, failed testing.

dsdeiz’s picture

What's different between textarea_accessibility_by_screenreader-2126761-4.patch & textarea_accessibility_by_screenreader-2126761-2.patch

I think he just corrected the patch file name.

Wrong comment no. in patch name - corrected.

dsdeiz’s picture

Status: Needs work » Needs review
mgifford’s picture

Ok, in that case, I'm not sure how adding a $element['#attributes']['aria-labelledby'] element is going to help as the problem that Jesse noted is that in $element['#attributes']['aria-describedby'] there is a reference to an ID which doesn't exist aria-describedby="edit-body-0-summary--description"

We have to make sure that the DOM ID value $element['#id'] . '--description' exists.

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Tim Bozeman’s picture

I don't really understand whats going on with $element['#id'] . '--description'
Doesn't that just add a string to the ID or is '--description' an actual flag? I changed it to $element['#description'] and now there is text showing up at least.

Tim Bozeman’s picture

I added aria-labelledby to #attributes and set it to $element['title']

mgifford’s picture

Ok, I think that gets you this:

<label for="edit-body-0-summary">Summary</label>
<textarea class="text-summary form-textarea resize-vertical text-summary-processed" aria-labelledby="Summary" aria-describedby="Leave blank to use trimmed value of full text as the summary." id="edit-body-0-summary" name="body[0][summary]" rows="3" cols="60"></textarea>

aria-labelledby, seems redundant as the label is already associated with the text area's ID.

For aria-describedby though you need to point to an ID. I think that the ID is supposed to be associated with the help text or description associated with the text area. At the moment this just has <div class="description">...</div> and there is no ID. That's why there's an error.

Tim Bozeman’s picture

Hmm. I see. Hmmm. It looks like the description div now has an ID that matches what the aria-describedby is pointing to.

<textarea ... aria-describedby="edit-body-0-summary--description"></textarea>
</div>
<div id="edit-body-0-summary--description" class="description">Leave blank to use trimmed value of full text as the summary.</div>

Maybe it was fixed in another issue? If not where's the error you speak of? :)

mgifford’s picture

That's accurate for the summary. It's accurate for the body of the text without CKEditor. However with the WYSIWYG editor enabled, the ID isn't present.

The summary seems to be fine, however, the body doesn't.

jessebeach’s picture

I think this issue has magically resolved itself and cannot be closed as cannot reproduce. Here is the code currently in D8 head:

<div class="form-textarea-wrapper">
  <textarea class="text-summary form-textarea resize-vertical text-summary-processed" aria-describedby="edit-body-0-summary--description" id="edit-body-0-summary" name="body[0][summary]" rows="3" cols="60"></textarea>
</div>
<div class="description" id="edit-body-0-summary--description">Leave blank to use trimmed value of full text as the summary.</div>

Note that the describedby attribute has an ID that points to the ID of the description element.

mgifford’s picture

This issue might be fixed on the summary (which is why this issue was created), but is still broken on the body with the WYSIWYG enabled:

That could just be a new issue, but I think it is broken because of the WYSIWYG.

<textarea id="edit-body-0-value" class="text-full form-textarea resize-vertical" placeholder="" cols="60" rows="9" name="body[0][value]" aria-describedby="edit-body-0--description" style="visibility: hidden; display: none;"></textarea>
...
<div class="description">more help text here.</div>
Tim Bozeman’s picture

It looks like <div class="description">more help text here.</div> was removed recently. On line 1350 of FormBuilder.php it checks if there is a description

    if (!empty($element['#description'])) {
      $element['#attributes']['aria-describedby'] = $element['#id'] . '--description';
    }

So I guess that it makes sense that the text area changed and doesn't have an 'aria-describedby' anymore.
<textarea placeholder="" cols="60" rows="9" name="body[0][value]" id="edit-body-0-value" class="text-full form-textarea resize-vertical" style="visibility: hidden; display: none;"></textarea>
So how does one add a description to the text area?

Tim Bozeman’s picture

I see that the description for the edit-body-0-summary text area was added with

'#description' => t('Leave blank to use trimmed value of full text as the summary.'),

on line 70 of core/modules/text/lib/Drupal/text/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php

Whats next? Extend TextItemBase in a new file?

Tim Bozeman’s picture

Status: Needs review » Needs work
mgifford’s picture

You can just add it to the content type here /admin/structure/types/manage/article/fields/node.article.body

If the descriptive text isn't there, then there's no need for the aria-describedby attribute. If it is added, then right now in Core the aria-describedby value is added to the textarea, but there is no ID added to the description.

mgifford’s picture

Why don't textarea's use theme_form_element()? The help text for the textarea just doesn't appear when I echo code within it..

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Issue summary: View changes

= D

mgifford’s picture

Ok, what is theming the textarea? Where is this defined?

mgifford’s picture

Issue tags: +aria
Tim Bozeman’s picture

I'm not too sure. In D7 I would check devel_themer. textarea.html.twig?

Cottser mentioned changing attributes should happen in preprocess. Does that sound like what we are going for to add the ID to the help text?

Tim Bozeman’s picture

Thank you very much for walking me through this far mgifford!

I'm not too sure how to figure out where it is defined or whats theming it. I see that the 'help text' field is at admin/structure/types/manage/article/fields/node.article.body

When I debug I can see the field using text-format-wrapper.html.twig, but I'm not sure how to see what is theming or defining it.

mgifford’s picture

Issue summary: View changes
mgifford’s picture

FileSize
494.24 KB

So with text-format-wrapper.html.twig we need a 3rd variable so we can insert an ID. Right now whenever this template is called, there is just no way to include the ID:
<div class="description">{{ description }}</div>

datetime has a similar issue.

Tim Bozeman’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.13 KB

I added the attributes twig thingy to line 16 of text-format-wrapper.html.twig
<div class="description" {{ attributes }}>{{ description }}</div>
and added attributes to line 68 of filter_theme() in filter.module.

'text_format_wrapper' => array(
    'variables' => array(
        'children' => NULL,
        'description' => NULL,
        'attributes' => NULL,
    ),
    'template' => 'text-format-wrapper',
);
mgifford’s picture

It's an improvement, but this produces:
<div class="description" aria-describedby="edit-body-0--description">Body help text. What will happen to the description? PATCH</div>

Rather than:
<div class="description" id="edit-body-0--description">Body help text. What will happen to the description? PATCH</div>

aria-describedby has to point to the ID for the descriptive text.

star-szr’s picture

+++ b/core/modules/filter/templates/text-format-wrapper.html.twig
@@ -13,6 +13,6 @@
-    <div class="description">{{ description }}</div>
+    <div class="description" {{ attributes }}>{{ description }}</div>

This should be:

<div class="description"{{ attributes|without('class') }}>

See https://drupal.org/node/2240005 for more details since this is a recent change.

Or better yet we can add the 'description' class in a preprocess function in filter.module and just do:

<div{{ attributes }}>

Tim Bozeman’s picture

I changed text-format-wrapper.html.twig to
<div {{ attributes }}>
and added a preprocess function that changes the aria-describedby key to id and outputs

<div class="text-full description" id="edit-body-0--description">helpful text</div>

The text-full class was already in the array.

mgifford’s picture

Status: Needs review » Patch (to be ported)

I think that's good! Great job @Tim Bozeman.

mgifford’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

oops

Tim Bozeman’s picture

@mgifford Thank you so much for walking me through that I learned a lot!

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

It's not a typo, there should be no space before {{ attributes }}, the leading space is inserted automatically. Other than that, looking good!

Tim Bozeman’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.95 KB

- <div {{ attributes }}>{{ description }}</div>
+<div{{ attributes }}>{{ description }}</div>

Thanks Cottser! :D

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Wow that was super quick! :) Reviewing this again there are a few other things I noticed.

We'll need to update some of the docs in text-format-wrapper.html.twig:

  1. Add @see template_preprocess_text_format_wrapper() now that it exists.
  2. Add boilerplate docs for the 'attribute' variable (just check other Twig templates).

And the preprocess docs could use a bit of love I think:

+++ b/core/modules/filter/filter.module
@@ -664,6 +668,24 @@ function template_preprocess_filter_guidelines(&$variables) {
+  // Add element #class and #id for screen reader.

Why the '#' here? It would be nice to explain the unset() as well with a small inline comment.

Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
2.63 KB

I added the docs and removed the #'s

There is a similar issue with datetime? Anyone got a link? Or should I start a new issue? :)

Edit: I didn't see an issue for datetime so I created one #2244923: The help text for datetime field needs an ID for screen readers.

mgifford’s picture

This comment is a bit misleading:
// Remove aria-describedby attribute in favor of id.

The aria-describedby has to point to an ID. It has to be unset because it shouldn't be showing up. Maybe:

// Remove aria-describedby attribute as as it shouldn't be visible here.

Not sure.

Tim Bozeman’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

@Tim wanted to add that my comment was pretty trivial, thanks for re-rolling it though.

This looks great to me. Tests out fine in manual tests too.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Me again…

+++ b/core/modules/filter/templates/filter-caption.html.twig
@@ -8,6 +8,9 @@
+ * - attributes: HTML attributes for the containing element.
+ *
+ * @see template_preprocess_text_format_wrapper()

I think these docs look great but this is filter-caption.html.twig which I don't think should be involved here :)

Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

That was not good. Thank you Cottser!

star-szr’s picture

Status: Needs review » Needs work

Cool, thanks @Tim Bozeman! One more small thing then I'm happy to RTBC based on @mgifford's manual testing :)

+++ b/core/modules/filter/filter.module
@@ -62,7 +62,11 @@ function filter_theme() {
+        'attributes' => NULL,

Attributes should be initialized as an empty array, see other hook_theme() definitions which define 'attributes' and _template_preprocess_default_variables()().

Jalandhar’s picture

Assigned: Tim Bozeman » Unassigned
Status: Needs work » Needs review
FileSize
2.3 KB
395 bytes

Updating with change said in #46.

star-szr’s picture

@Jalandhar thank you but you kinda stole the issue away from Tim :( it was assigned and he was turning around patches rather quickly over the past week or so. Usually we give people at least a few days if the issue is assigned to them, especially because this is not major/critical/beta blocking.

Anyway, thanks and looks good to me.

mgifford’s picture

It's still @Tim Bozeman's.. He's been pushing this issue since December. I really don't think it matters who rolls the last patch.

Also thanks to @Jalandhar, @JesseBeach, @SGhosh & @Cottser for fixing this!

Jalandhar’s picture

@Cottser: I really didn't mean to steal the patch from Tim Bozeman. I have just worked on the re-roll of the patch by seeing this patch under needs work issues, anyways the credit is for Tim Bozeman only. I am really sorry if I have done wrong.

My re-rolled patch might be a minor help for him....:)

star-szr’s picture

@Jalandhar, don't worry. Just wanted to make sure you were aware. Onwards!

Tim Bozeman’s picture

<3

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: body-field-summary-textarea-screenreader-id-2126761-48.patch, failed testing.

mgifford’s picture

Jalandhar’s picture

Status: Needs work » Needs review
Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: body-field-summary-textarea-screenreader-id-2126761-48.patch, failed testing.

Tim Bozeman’s picture

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

Bad bot...

  • Commit 14baa0b on 8.x by catch:
    Issue #2126761 by Tim Bozeman, SGhosh, Jalandhar: The body field summary...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.