1280384 - [a11y] Make the different module titles headings and their expand/collapse thingie accessible

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

Each module has a heading, and beside it, a control that expands or collapses that module. For easier navigation, the module titles should be headings, and for keyboard and screen reader accessibility, the expand/collapse control should be a button.

Preferred method of fixing, but more intrusive:

1. Turn each div with class module-title into an <h2> element.

2. Turn every div with class "module-spinner" into a <button> element, and give it an aria-label of "show " + module name if the module is currently collapsed, or "hide " + module name if it is currently expanded. That way you can keep your visual icon that you're using.

Less preferred, but also less intrusive way:

1. Give every div with class module-title a role of "heading" and an aria-level="2" attribute.

2. Give every div with class module-spinner a role of "button", a tabindex of 0, an aria-label that reads "Expand " + module name if it is collapsed, or "Collapse " + module name if it is currently expanded. And add a keyboard handler that toggles upon space and enter.

Comment 1

[David Lawrence [:dkl]]

We would like to fix this before we switch over to modal as default.

Comment 2

[Kohei Yoshino [:kohei]]

Will take a look. Probably requires aria-expanded as well. See [1]. Interestingly this is somewhat different from WAI-ARIA 1.0 Authoring Practices, which was recommending an implementation with tabs and tabpanels [2].

[1] https://www.w3.org/TR/wai-aria-practices-1.1/#accordion
[2] https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#accordion

Comment 3

[Kohei Yoshino [:kohei]]

Attachment: 1280384.patch

Here's a patch to solve the issue. Tested locally with NVDA and VoiceOver.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 8825236 [details] [diff] [review]
1280384.patch

>+      <div class="module-latch" role="button" tabindex="0"
>+           aria-controls="module-[% title.replace FILTER id %]-content"
>+           aria-expanded="[% collapsed ? "false" : "true" %]"
>+           aria-label="[% collapsed ? "Expand" : "Collapse" %] [% title FILTER html %] section"
The aria-label is actually not needed here. First of all, the button title and sub title are already provided by the inner divs. Second, aria-expanded already tells the screen reader to speak whether the state of this button is expanded or collapsed. Adding this info through the aria-label doubles the information unnecessarily. Furthermore, aria-label overrides text for the button derived from inner children, which would take away the information possibly provided by the sub title. So adding this aria-label is actually not necessary.

>+        <div class="module-spinner" aria-hidden="true"></div>
If possible, aria-hidden="true" should be replaced with role="presentation". aria-hidden is not consistently supported, and it is not necessary because we're not hiding a sub tree here, just a single element. For single elements, role="presentation" is designed to do just that. And since the CSS class will generate some form of on-element content for the latch spinner, this should then be hidden properly. have you tried that first, or did you just use aria-hidden because it was the most obvious from the spec?

Over all, I'm a bit weary about some of these changes. Particularly the section element has a tendency to over-talk in some screen readers, see this article for more explanation: https://www.paciellogroup.com/blog/2013/10/using-html5-section-element/. Especially JAWS has a tendency to over-talk regions, but we'll have to see this in action.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 8825236 [details] [diff] [review]
1280384.patch

Review of attachment 8825236 [details] [diff] [review]:
-----------------------------------------------------------------

To actually make this semantically really correct, one would do the following. However, that would turn the module title not clickable, since the title and sub title would be moved out of the simulated button div. But having headings in structural elements is something some screen readers don't cope with very well. NVDA and VoiceOver are both very forgiving in that regard, JAWS and some others however, are not.

So here's the theoretical continuation of the patch, but a more intrusive one:

::: extensions/BugModal/template/en/default/bug_modal/module.html.tmpl
@@ +29,4 @@
>      [%~ ' data-non-stick="1"' IF no_collapse_persist %]
>  >
>    [% IF title %]
> +    <header id="module-[% title.replace FILTER id %]-header" class="module-header" role="heading" aria-level="2">

Keep the header element, but get rid of the role and aria-level attributes.

@@ +32,5 @@
> +    <header id="module-[% title.replace FILTER id %]-header" class="module-header" role="heading" aria-level="2">
> +      <div class="module-latch" role="button" tabindex="0"
> +           aria-controls="module-[% title.replace FILTER id %]-content"
> +           aria-expanded="[% collapsed ? "false" : "true" %]"
> +           aria-label="[% collapsed ? "Expand" : "Collapse" %] [% title FILTER html %] section"

Still get rid of the aria-label, but replace it with aria-labeledby pointing to the module title below, and aria-describedby to the module subtitle element below, via their IDs.

@@ +35,5 @@
> +           aria-expanded="[% collapsed ? "false" : "true" %]"
> +           aria-label="[% collapsed ? "Expand" : "Collapse" %] [% title FILTER html %] section"
> +           data-label-expanded="Collapse [% title FILTER html %] section"
> +           data-label-collapsed="Expand [% title FILTER html %] section">
> +        <div class="module-spinner" aria-hidden="true"></div>

Still get rid of the aria-hidden, replacing it with role="presentation", if that is equivalent (needs testing). And additionally, close the latch button div here. Then, move the following two divs to the same level below the <header> element.

@@ +40,3 @@
>          <div class="module-title">[% title FILTER html %]</div>
>          [% IF subtitle != "" && subtitle.size %]
>            <div class="module-subtitle">

And make these true h2 and h3 elements, h2 for the title, h3 for the sub title.

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to Marco Zehe (:MarcoZ) from comment #5)
> @@ +32,5 @@
> > +    <header id="module-[% title.replace FILTER id %]-header" class="module-header" role="heading" aria-level="2">
> > +      <div class="module-latch" role="button" tabindex="0"
> > +           aria-controls="module-[% title.replace FILTER id %]-content"
> > +           aria-expanded="[% collapsed ? "false" : "true" %]"
> > +           aria-label="[% collapsed ? "Expand" : "Collapse" %] [% title FILTER html %] section"
> 
> Still get rid of the aria-label, but replace it with aria-labeledby pointing
> to the module title below, and aria-describedby to the module subtitle
> element below, via their IDs.

Oh and of course, turn it into a <button> element. That will save you from having to add a keyboard handler and you can get rid of the tabindex attribute, since button is focusable by default. It will also be visible when tabbing to it via the keyboard by default. You can still have the presentational div child that gets that Unicode character.

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 8825236 [details] [diff] [review]
1280384.patch

Review of attachment 8825236 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Marco Zehe (:MarcoZ) from comment #6)
> Oh and of course, turn it into a <button> element. That will save you from
> having to add a keyboard handler and you can get rid of the tabindex
> attribute, since button is focusable by default. It will also be visible
> when tabbing to it via the keyboard by default. You can still have the
> presentational div child that gets that Unicode character.

I would rather not add new <button>s to the modal form as we are trying to move away from them. If Marco is OK, let's
stick with <a role="button"> elements when possible.

r- to address Marco's other comments.

dkl

Comment 8

[David Lawrence [:dkl]]

kohei, if you want I can finish this up if you are pressed for time.

Comment 9

[Kohei Yoshino [:kohei]]

will revise my patch soon.

Comment 10

[David Lawrence [:dkl]]

Attachment: 1280384_1.patch

Kohei, i will go ahead and finish this one up. Thanks for the help so far!

dkl

Comment 11

[David Lawrence [:dkl]]

To https://github.com/mozilla-bteam/bmo.git
   286a76a..bdfa303  development -> development

Marco, I have put these changes on https://bugzilla-dev.allizom.org for your testing/feedback if you get a chance.

Thanks
dkl

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 8834626 [details] [diff] [review]
1280384_1.patch

This looks good, thanks!

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 8834626 [details] [diff] [review]
1280384_1.patch

I just had another go at playing with this on the test server, and have another comment:

>-    <div class="module-header">
>-      <div class="module-latch">
>-        <div class="module-spinner">[% collapsed ? "&#9656;" : "&#9662;" %]</div>
>-        <div class="module-title">[% title FILTER html %]</div>
>+    <header id="module-[% title.replace FILTER id %]-header" class="module-header">
>+      <div class="module-latch" role="button" tabindex="0"
>+           aria-controls="module-[% title.replace FILTER id %]-content"
>+           aria-expanded="[% collapsed ? "false" : "true" %]"
>+           aria-labeledby="module-[% title.replace FILTER id %]-title"
>+           aria-describedby="module-[% title.replace FILTER id %]-subtitle"
>+           data-label-expanded="Collapse [% title FILTER html %] section"
>+           data-label-collapsed="Expand [% title FILTER html %] section">
>+        <div class="module-spinner" role="presentation"></div>

Actually, I think this last div, the spinner, should get the button role and other ARIA attributes, not the container. Because if the container gets it, like it is now, all the heading 2 and 3 will also be announced as being a button. So the parent div should not get the ARIA stuff, the module-spinner div should, making it the dedicated button for keyboard users.

Comment 14

[David Lawrence [:dkl]]

Attachment: 1280384_2.patch

Made change suggested by Marco to latest patch. Moving feedback+ forward.

dkl

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8835729 [details] [diff] [review]
1280384_2.patch

This functions the same as before -- hopefully it works in a screen reader now!

r=dylan

Comment 16

[David Lawrence [:dkl]]

To https://github.com/mozilla-bteam/bmo.git
   c4dd847..2b95a51  master -> master