489549 - Buttons of HTML5 audio and video element control set have no accessible names

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

After landing of bug 483573, we now expose the buttons to control playback and other stuff through a11y APIs.

The buttons, however, don't have accessible names yet, so NVDA reports them as "button", and the user does not know which button does what.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Marco, test shows accessible names for control button are presented but it sounds JAWS reads accessible name of previous element of audio element when focus goes on button. Any idea?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Created an attachment (id=374418) [details]
> patch
> 
> Marco, test shows accessible names for control button are presented but it
> sounds JAWS reads accessible name of previous element of audio element when
> focus goes on button. Any idea?

Something wrong with focus I guess?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> Something wrong with focus I guess?

It sounds no, we fire correct internal focus events

Comment 4

[Marco Zehe (:MarcoZ)]

NVDA sees the buttons with this patch.

A few remarks:
1. When the media is playing, the tooltiptext should change from "Play" to "Pause".
2. If the media is muted, the "Mute" tooltiptext should change to "Unmute".
3. We need to make sure these are localizable so they show up in the Firefox language in use, not hard-coded in English.
4. Progress bar names should reflect what user sees as captions visually.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: wip

mostly complete, fighting with automated tests

Started to use aria-label instead of tooltiptext because tooltiptext isn't suitable for mute button since slider control is shown on mouseover.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: wip2

latests version (button's name works)
1) slider hans't still accessible name (I'll file another bug for this)
2) action tests reorg brings regression to test_actions.xul - when click is fired then menupopup isn't open yet, we need to listen menupopup_start I guess but it isn't fired for button@type="menu", I'll file bug for this

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 8

[alexander :surkov (:asurkov)]

I disabled one tests in test_actions.xul because accessible of menuitem of the button@type="menu" isn't created yet when button is clicked, It's worth to listen here a11y events but unfortunately we don't fire any a11y events here (see bug 490288).

Comment 9

[[e-mail]]

Does this actually work? I thought videocontrols ran with page privileges, which would prevent them accessing components.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Does this actually work? I thought videocontrols ran with page privileges,
> which would prevent them accessing components.

I tested only on chrome's mochitests where it works. Yes, you're right and it doesn't work in wild. Neil, any idea how to fix this. Can xul:stringbundle element help?

Comment 11

[[e-mail]]

Comment on attachment 375122 [details] [diff] [review]
patch

I think you'll need to put the strings in a DTD and use entities for attributes in the XBL.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 375167 [details] [diff] [review]
patch2

r=me, but would like to test this just to be safe when I get back to Germany on Saturday. Have no working Windows VM with build with me here. Thanks for tying the actions test into our regular event queue infrastructure!

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 375167 [details] [diff] [review]
patch2

>+function testActions(aArray)

I like the changes to this function Alex; do you think they could cause regressions? I know we had some non-deterministic problems with these tests (in tinderbox). The changes might belong on a separate bug but I don't care (I'm not picky).

The a11y code looks okay to me. I'll defer to Justin for the video diff except I think you are missing a '+' for the new locale (hard to see because of existing +'s and your diff hides the missing one):


> + locale/@AB_CD@/global/dialog.properties               (%chrome/global/dialog.properties)
> + locale/@AB_CD@/global/tree.dtd                        (%chrome/global/tree.dtd)
> + locale/@AB_CD@/global/textcontext.dtd                 (%chrome/global/textcontext.dtd)
>+  locale/@AB_CD@/global/videocontrols.dtd               (%chrome/global/videocontrols.dtd)
> + locale/@AB_CD@/global/viewSource.dtd                  (%chrome/global/viewSource.dtd)
> + locale/@AB_CD@/global/viewSource.properties           (%chrome/global/viewSource.properties)


r=me for the a11y part and if you are confident the action tests won't regress.

Comment 15

[[e-mail]]

(In reply to comment #14)
> (From update of attachment 375167 [details] [diff] [review])
> I think you are missing a '+' for the new locale (hard to see because of
> existing +'s and your diff hides the missing one):
FYI
* - preprocessing - not suitable for locales.
+ - forced override - was used when different jar files would package the same file - now unnecessary as you can #ifdef the jar.mn instead

Comment 16

[Marco Zehe (:MarcoZ)]

Hoping to get this into 1.9.1 also, but since Firefox 3.5 is already string-frozen, we might need a different approach for branch because of the newly added localizable strings.

Comment 17

[Axel Hecht [:Pike]]

Kinda sad that the existing strings for this are in browser.

Not sure if we should make the browser context menu use the new toolkit strings on the trunk.

As this goes, I don't see a way to make the toolkit xbl use the browser strings, though we could hack up a script to create the l10n files for toolkit out of the browser strings by something like silme.

Comment 18

[Marco Zehe (:MarcoZ)]

Alex, I just tested with this patch, and noticed that we're not firing an OBJECT_NAMECHANGE event on the Play/Stop and Mute/Unmute buttons when they're being pressed. The visual caption changes, and after a refresh of the virtual buffer and a re-focusing on the button, NVDA does get the correct caption, but simply pressing the button will keep the caption the way it is in NVDA's virtual buffer. I checked with AccProbe, and we're definitely not getting an OBJECT_NAMECHANGE from MSAA. Do you want a separate bug filed on this, or weave it into this bug still?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> Do you want a separate bug filed on this, or weave
> it into this bug still?

Let's deal with separate bug because it's worth to add mochitests for these events, it may get bigger the patch.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Kinda sad that the existing strings for this are in browser.
> 
> Not sure if we should make the browser context menu use the new toolkit strings
> on the trunk.
> 
> As this goes, I don't see a way to make the toolkit xbl use the browser
> strings, though we could hack up a script to create the l10n files for toolkit
> out of the browser strings by something like silme.

Axel do you mean new strings are presented somewhere in browser locale already? If so are they used for logically same things?

Comment 21

[alexander :surkov (:asurkov)]

Justin, any chance to review toolkit's part?

Comment 22

[Marco Zehe (:MarcoZ)]

(In reply to comment #19)
> (In reply to comment #18)
> > Do you want a separate bug filed on this, or weave
> > it into this bug still?
> Let's deal with separate bug because it's worth to add mochitests for these
> events, it may get bigger the patch.

Done, bug 491443.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (From update of attachment 375167 [details] [diff] [review])
> >+function testActions(aArray)
> 
> I like the changes to this function Alex; do you think they could cause
> regressions? I know we had some non-deterministic problems with these tests (in
> tinderbox). The changes might belong on a separate bug but I don't care (I'm
> not picky).

They may bring regressions because new approach removes timeouts between actions. But since eventQueue is used some time already then it should be safely (and it's more correct approach :) ).

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 375167 [details] [diff] [review]
patch2

>--- a/toolkit/content/widgets/videocontrols.xml
...
>-                    <button class="playButton"/>
>+                    <button class="playButton"
>+                            playlabel="&playButton.label;"
>+                            stoplabel="&stopButton.label;"/>

Nit: The action performed by the button is either "play" or "pause", not "stop". I've seen "stop" imply that the play position is reset to the beginning (which pause doesn't do). Also, better to use "pause" everywhere than two terms.

I wonder if it might be worthwhile to have the entities named "playButton.playLabel" and "playButton.pauseLabel", since there's really just 1 button that gets different labels. Probably doesn't matter...

>+                setPlayButtonState: function(aPaused)
...
>+                setMuteButtonState: function(aMuted)

Nit: move these 2 functions down to after the existing toggleMute function.


>+++ b/toolkit/locales/en-US/chrome/global/videocontrols.dtd

I guess you'll need late-l10n approval to land new strings, you should make sure you talk to drivers about this.

r+ with these changes, bouncing the review over to Enn since I'm not a toolkit peer. :) I see NeilAway sr'd this, so I dunno if that's strictly needed, but anyway...

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)

> >+++ b/toolkit/locales/en-US/chrome/global/videocontrols.dtd
> 
> I guess you'll need late-l10n approval to land new strings, you should make
> sure you talk to drivers about this.

Do I need this to land the patch on trunk (1.9.2)?

Comment 26

[Justin Dolske [:Dolske]]

No.

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch3

with Justin comments

Comment 28

[Neil Deakin]

Comment on attachment 376139 [details] [diff] [review]
patch3

Seems ok. We should file a bug on creating a real image button type.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> (From update of attachment 376139 [details] [diff] [review])
> Seems ok. We should file a bug on creating a real image button type.

Could you please do this because I'm not sure I follow you completely?

Comment 30

[alexander :surkov (:asurkov)]

Attachment: patch4

up to dated to trunk

Comment 31

[alexander :surkov (:asurkov)]

checked in http://hg.mozilla.org/mozilla-central/rev/d356b4e0ab66

Comment 32

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090512 Minefield/3.6a1pre (.NET CLR 3.5.30729)

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 376337 [details] [diff] [review]
patch4

how can we get this on 1.9.1?

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 376337 [details] [diff] [review]
patch4

a191=beltzner

Comment 35

[alexander :surkov (:asurkov)]

checked in on 1.9.1 - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6388618abf44

Comment 36

[Marco Zehe (:MarcoZ)]

Verified fixed in 1.9.1 in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre (.NET CLR 3.5.30729)