1280397 - [a11y] Make the component, product, and other latches accessible

Status: RESOLVED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

There are some icon buttons to show and hide the descriptions for the component, product, and possibly other items that can be seen in view mode. To me they appear like you need to click on them to show and hide the components, but I am not sure about that, since I cannot visually verify if a hover does the same. Anyway, these need to be made accessible with an already known pattern:

Preferred method is to just turn these into <button> elements that show those icon fonts. And give them an aria-label of "show component description" or "hide component description" or similar, whichever is appropriate fo reach.

However if that isn't possible, go about the usual way:

1. role="button"

2. tabindex="0"

3. Keyboard handler reacts to enter and space like a mouse click.

4. aria-label changes according to the showing or hiding of the actual descriptions.

Additional step: Because the button or div appears *before* the actual field value in the DOM order, one could put an aria-owns on their common container like this:

aria-owns="id_of_field_value id_of_description_toggle"

which would put the button *after* the field value for screen reader users and make stuff more readable and less chatty.

Comment 1

[David Lawrence [:dkl]]

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

Comment 2

[David Lawrence [:dkl]]

Attachment: 1280397_1.patch

I ended up going with role="button" as are trying to do away with <buttons> on the modal form.

dkl

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8825956 [details] [diff] [review]
1280397_1.patch

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

Cleared r?. Forgot the CC latch which needs to the same treatment.

Comment 4

[David Lawrence [:dkl]]

Attachment: 1280397_2.patch

Added aria for cc section.

dkl

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 8826052 [details] [diff] [review]
1280397_2.patch

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

::: extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl
@@ +344,5 @@
> +              data-for="product">&#9656;</span>
> +        <div role="button" aria-label="show product information" tabindex="0"
> +             class="spin-toggle" id="product-name" data-latch="product" data-for="product">
> +          [% bug.product FILTER html %]
> +        </div>

Actually, this should be:

      <div aria-owns="product-name product-latch">

because we want the description showing and hiding closest to where the description would appear. That's at least how I understand the logic from my original bug description. If the logic has changed, and the children are actually in the most logical order already, aria-owns would actually not be needed at all, and that part of my original description would be obsolete. You know the UI better than I, so you decide, but anyway this aria-owns needs either changing or removing, otherwise it won't have any effect at all, since the IDs are currently in the order the children are appearing already.

@@ +387,5 @@
> +              data-for="component">&#9656;</span>
> +        <div role="button" aria-label="show component description" tabindex="0"
> +             class="spin-toggle" id="component-name" data-latch="#component-latch" data-for="component">
> +          [% bug.component FILTER html %]
> +        </div>

Same here. Either change order of IDs, or remove aria-owns if there was a change that already put the children in the correct order.

@@ +690,5 @@
> +              ELSE;
> +                bug.cc.size _ " people";
> +              END;
> +            %]
> +          </span>

And here.

Comment 6

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

Comment on attachment 8826052 [details] [diff] [review]
1280397_2.patch

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

r-

I think the spin-latch is a visual-only option.

also marco's review points need to be addressed as well.

Interestingly, the behavior here is quite close to details/summary html5 tags.

::: extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl
@@ +338,5 @@
>          hide_on_edit = can_edit_product
>          help         = "describecomponents.cgi"
>      %]
> +      <div aria-owns="product-latch product-name">
> +        <span role="button" aria-label="show product description" tabindex="0"

I think the spin-latch should be silent/"hidden" from the screen reader, since (per a chat with marco in #bmo) it doesn't do anything different than the spin-toggle.

Comment 7

[Marco Zehe (:MarcoZ)]

Of course, with some of these now having been identified as purely visual, there are some simplifications:

1. The dance with aria-owns no longer needs to happen at all.
2. The latch div should be aria-hidden="true" (yes, aria-hidden, since its text children need to be hidden as well.
3. To indicate whether a product or other thing is currently showing the description or not, the other div, like product-name, should get an aria-expanded="true"/"false" depending on desc showing or not.
4. The aria-label should then not be used, but instead let the text children provide the label for the role="button".

Sorry for the noise and the confusion. The problem is that when I filed the bugs, I didn't have any visual assistance and was trying to figure out how this UI worked all by myself.

Comment 8

[David Lawrence [:dkl]]

Attachment: 1280397_3.patch

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 8828933 [details] [diff] [review]
1280397_3.patch

>+           aria-exapanded="false" class="spin-toggle" id="product-name"

Typo: One a too many in aria-expanded. ;)

>+           aria-exapanded="false" class="spin-toggle" id="component-name"

Same.

Other than that, this patch looks good from my end.

Comment 10

[David Lawrence [:dkl]]

(In reply to Marco Zehe (:MarcoZ) from comment #9)
> Comment on attachment 8828933 [details] [diff] [review]
> 1280397_3.patch
> 
> >+           aria-exapanded="false" class="spin-toggle" id="product-name"
> 
> Typo: One a too many in aria-expanded. ;)
> 
> >+           aria-exapanded="false" class="spin-toggle" id="component-name"
> 
> Same.
> 
> Other than that, this patch looks good from my end.

Oops :) Thanks for spotting that!

Comment 11

[David Lawrence [:dkl]]

Attachment: 1280397_4.patch

New patch with typo corrected.

dkl

Comment 12

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

Comment on attachment 8829302 [details] [diff] [review]
1280397_4.patch

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

r=dylan

Does what it says on the tin.

Comment 13

[David Lawrence [:dkl]]

To https://github.com/mozilla-bteam/bmo.git
   3c1d21e..c768148  master -> master

Comment 14

[Marco Zehe (:MarcoZ)]

Argh, I just saw that this was pushed to production, and that it actually breaks things. Right now, I see the component or product link, and then the button to expand or collapse the description. Unfortunately, due to my not understanding all of this markup correctly, the role="button" and aria-label now overrides the actual information, which I didn't realize is on the component-name and product-name elements. So I see the button, but not that this is the component for modal. Or the product is bugzilla.

So to correct this, we'd have to remove the aria-hidden from the latch, move the button and aria-label there, and largely leave the product-name and component-name alone so they can still show the correct information. The latch should become the button we want to simulate.

Sorry for the confusion! :-( dkl, should I file a new bug for this, or should we reopen this one and work this into a followup patch?

Comment 15

[Marco Zehe (:MarcoZ)]

Same goes for, for example, the cc-summary. The CC latch is already correct, but the cc-summary also became a button, and its aria-label overrides the actually valuable information.

Comment 16

[David Lawrence [:dkl]]

(In reply to Marco Zehe (:MarcoZ) from comment #14)
> Argh, I just saw that this was pushed to production, and that it actually
> breaks things. Right now, I see the component or product link, and then the
> button to expand or collapse the description. Unfortunately, due to my not
> understanding all of this markup correctly, the role="button" and aria-label
> now overrides the actual information, which I didn't realize is on the
> component-name and product-name elements. So I see the button, but not that
> this is the component for modal. Or the product is bugzilla.
> 
> So to correct this, we'd have to remove the aria-hidden from the latch, move
> the button and aria-label there, and largely leave the product-name and
> component-name alone so they can still show the correct information. The
> latch should become the button we want to simulate.
> 
> Sorry for the confusion! :-( dkl, should I file a new bug for this, or
> should we reopen this one and work this into a followup patch?

No problem. It has been a good learning experience for me. I will back out this change and we can work on a new revision for next weeks push.

dkl

Comment 17

[David Lawrence [:dkl]]

Reverted.

To https://github.com/mozilla-bteam/bmo.git
   6cadcc1..2fcef7a  master -> master

dkl

Comment 18

[Marco Zehe (:MarcoZ)]

In case this wasn't clear: The patch itself is good in principle, but due to me giving you some confused advice, some of this stuff ended up on the wrong elements.

Comment 19

[David Lawrence [:dkl]]

Attachment: 1280397_5.patch

To https://github.com/mozilla-bteam/bmo.git
   ef84c52..f31f740  development -> development

I have uploaded a revised version of this patch to our test instance at https://bugzilla-dev.allizom.org for your feedback.

Thanks
dkl

Comment 20

[Marco Zehe (:MarcoZ)]

Comment on attachment 8831332 [details] [diff] [review]
1280397_5.patch

>+        <span aria-owns="cc-latch cc-summary">

Actually you want cc-summary and cc-latch reversed, so the screen reader speaks the CC summary first, then the "show CC list" button. aria-owns only makes sense if you want to *change* the order of children, not confirm it.

>+          <span role="button" aria-label="show cc list" tabindex="0"
>+                id="cc-summary" data-count="[% bug.cc.size FILTER none %]">

Please remove the aria-label here. You already have it on the latch, and aria-label on the cc-summary would cause the same problem as with the previous patch: Remove the actually interesting CC summary from the screen reader's view.

Also, further above, I now remember what I wanted aria-owns for, there, too.

To the *parent* of these elements:

>+      <span role="button" aria-label="show product information" aria-expanded="false" tabindex="0"
>+            class="spin-latch" id="product-latch" data-latch="product" data-for="product">&#9656;</span>
>+      <div title="show product information" tabindex="0" class="spin-toggle"
>+           id="product-name" data-latch="product" data-for="product">

add aria-owns="product-name product-latch" so screen readers, when browsing the page, will read the actual product name first and don't have to first muddle through the button. Visually, this may make sense to have it the way it is, but for a sequential medium like speech, the important info should come first, and that, in this case is the product-name, not the button.

Same here for the parent of these two:

>+      <span role="button" aria-label="show component description" aria-expanded="false" tabindex="0"
>+            class="spin-latch" id="component-latch" data-latch="component" data-for="component">&#9656;</span>
>+      <div title="show component information" tabindex="0" class="spin-toggle" id="component-name"
>+           data-latch="#component-latch" data-for="component">

add aria-owns="component-name component-latch"

With these changes, the patch is good to go.

Comment 21

[David Lawrence [:dkl]]

Attachment: 1280397_6.patch

Thanks Marco.

Comment 22

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

Comment on attachment 8832058 [details] [diff] [review]
1280397_6.patch

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

r=dylan

Comment 23

[David Lawrence [:dkl]]

To https://github.com/mozilla-bteam/bmo.git
   d0cc4c7..e184203  master -> master