1395178 - The new buttons that can be added to the toolbar don't announce themselves to screen readers as buttons.

Status: VERIFIED FIXED

(Firefox :: Disability Access, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

The new buttons that can be added to the toolbar don't appear to screen readers as buttons, but as grphics. The name and accessible description are OK, but the role isn't. Screen readers think these are simple graphics. But they should be buttons so screen readers know that they can click on them. The click action works, at least with NVDA, but others might not be as forgiving.

Comment 1

[Drew Willcoxon :adw]

These icons are <image> nodes.  We've had images in the urlbar for a long time now, like the bookmark star button, and as far as I can tell, we haven't used any accessibility-related markup on them.

So I have a couple of questions, Marco:

First, I'm wondering how image nodes were presented in the past.  For example, did the bookmark star button previously announce itself as a button?  Or the reader mode button or the page compatibility button?

Second, I tried adding role=button to these images, but that doesn't seem to change how NVDA's speech reader presents them.  NVDA still just speaks their tooltips.  What should I be looking for exactly?

Comment 2

[Drew Willcoxon :adw]

Oh, looks like there's an NVDA option to report roles.  Indeed, I see that these new icons are read as "graphic".  When I add role=button, they're read as "button".  Interestingly the reader mode button is also read as "button", but I don't understand why.

I also see that the Pocket button isn't being read correctly.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1395178 - The new buttons that can be added to the toolbar don't announce themselves to screen readers as buttons.

Review commit: https://reviewboard.mozilla.org/r/174614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174614/

Comment 4

[Drew Willcoxon :adw]

Summary:

* Add role=button for all <image> buttons in markup.  NVDA already speaks the reader mode button as "button", and I don't know why, but adding a role for it seems like a good idea anyway.

* When creating urlbar buttons for page actions, set role=button

* Update Pocket's button: add role=button, and also set a tooltip (actually on both images, which seems to be necessary, same as the bookmark star)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8902898 [details]
Bug 1395178 - The new buttons that can be added to the toolbar don't announce themselves to screen readers as buttons.

https://reviewboard.mozilla.org/r/174614/#review179694

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91cd19239d21
The new buttons that can be added to the toolbar don't announce themselves to screen readers as buttons. r=jaws

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/91cd19239d21

Comment 8

[:Gijs (he/him)]

Did you mean to set role=button both on the <hbox> and on the <image> for the items that have both (bookmarks star, pocket) ? That doesn't seem quite right...

Comment 9

[Marco Zehe (:MarcoZ)]

Right, that set bookmarks thing is funny, it has two buttons that sound the same, and the parent, supposedly a grouping, is announced as a button, too. The other buttons are fine in that toolbar.

Comment 10

[Drew Willcoxon :adw]

Thanks, no I don't think I meant to set role on both the hbox and its buttons, only the buttons.  I'll open a new bug once I'm clear on how to fix the bookmarks button.

Marco, this bookmarks parent box has two images in it, but only so that we can implement an animation when you click it.  Conceptually it's just one button with one image.  How should the roles be set here?  The Pocket button is similar by the way, except I didn't goof up and add role=button to its outer hbox.

Here is the bookmark button's (incorrect) markup as it's currently written, in case it helps:

<hbox id="star-button-box"
      hidden="true"
      class="urlbar-icon-wrapper urlbar-page-action"
      role="button"
      context="pageActionPanelContextMenu"
      oncontextmenu="BrowserPageActions.onContextMenu(event);"
      onclick="BrowserPageActions.bookmark.onUrlbarNodeClicked(event);">
  <image id="star-button"
         class="urlbar-icon"
         role="button"
         observes="bookmarkThisPageBroadcaster"/>
  <hbox id="star-button-animatable-box">
    <image id="star-button-animatable-image"
           role="button"
           observes="bookmarkThisPageBroadcaster"/>
  </hbox>
</hbox>

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to Drew Willcoxon :adw from comment #10)
> Marco, this bookmarks parent box has two images in it, but only so that we
> can implement an animation when you click it.  Conceptually it's just one
> button with one image.  How should the roles be set here?  The Pocket button
> is similar by the way, except I didn't goof up and add role=button to its
> outer hbox.

And you didn't add a button role to the animatable image I think, since I don't see that, only one Pocket button.

> Here is the bookmark button's (incorrect) markup as it's currently written,
> in case it helps:
> 
> <hbox id="star-button-box"
>       hidden="true"
>       class="urlbar-icon-wrapper urlbar-page-action"
>       role="button"

Remove the role.

>       context="pageActionPanelContextMenu"
>       oncontextmenu="BrowserPageActions.onContextMenu(event);"
>       onclick="BrowserPageActions.bookmark.onUrlbarNodeClicked(event);">
>   <image id="star-button"
>          class="urlbar-icon"
>          role="button"
>          observes="bookmarkThisPageBroadcaster"/>
>   <hbox id="star-button-animatable-box">
>     <image id="star-button-animatable-image"
>            role="button"

Make this role="presentation".

>            observes="bookmarkThisPageBroadcaster"/>
>   </hbox>
> </hbox>

And to be on the safe side, do the same for the Pocket button, also for consistency.

Comment 12

[Drew Willcoxon :adw]

Thanks Marco.  I've moved this to the new bug 1397609.  I'll post a patch there and a try build.

Comment 13

[Emil Ghitta, QA [:emilghitta]]

Hi Drew!

I managed to verify this issue using NVDA on Windows 10 64bit.

I didn't managed to verify this using Orca Screen reader on linux and VoiceOver on mac (didn't managed to find the options for reporting the button roles).

Could you help me out with this one or is it safe to verify this issue only with NVDA?

Comment 14

[Drew Willcoxon :adw]

IIRC Marco has said before that Firefox doesn't work well VoiceOver in general, so I'm not sure whether it's expected to work here.  I don't know about Orca.  Marco, do you know the answer to Emil's questions in comment 13?

Comment 15

[Drew Willcoxon :adw]

Emil, also, this bug didn't really fix the problem right.  It might be better for you to verify only bug 1397609 instead.

Comment 16

[Marco Zehe (:MarcoZ)]

It's safe to verify with NVDA. I don't know the Orca commands to get to buttons that aren't tabable off-hand, but if it works with NVDA on Windows, it usually works with Orca, too. It theoretically also works with VoiceOver on the Mac, but Mac a11y is currently in a stale state.

Comment 17

[Emil Ghitta, QA [:emilghitta]]

Marking this as verified fixed as per comment 13 and comment 16.