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

VERIFIED FIXED

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access, verified1.9.1})

Trunk
access, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Posted patch patch (obsolete) — Splinter Review
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?
(Assignee)

Comment 2

10 years ago
(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?
(Assignee)

Comment 3

10 years ago
(In reply to comment #2)

> Something wrong with focus I guess?

It sounds no, we fire correct internal focus events
(Reporter)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
Posted patch wip (obsolete) — Splinter Review
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.
Assignee: nobody → surkov.alexander
Attachment #374418 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 6

10 years ago
Posted patch wip2 (obsolete) — Splinter Review
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
Attachment #374592 - Attachment is obsolete: true
(Assignee)

Comment 7

10 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #374738 - Attachment is obsolete: true
Attachment #375122 - Flags: superreview?(neil)
Attachment #375122 - Flags: review?(marco.zehe)
(Assignee)

Updated

10 years ago
Attachment #375122 - Flags: review?(david.bolter)
(Assignee)

Updated

10 years ago
Attachment #375122 - Flags: review?(dolske)
(Assignee)

Comment 8

10 years ago
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).
(Assignee)

Updated

10 years ago
Flags: in-testsuite+
Does this actually work? I thought videocontrols ran with page privileges, which would prevent them accessing components.
(Assignee)

Comment 10

10 years ago
(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 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.
Attachment #375122 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 12

10 years ago
Posted patch patch2 (obsolete) — Splinter Review
Attachment #375122 - Attachment is obsolete: true
Attachment #375167 - Flags: superreview?(neil)
Attachment #375167 - Flags: review?(marco.zehe)
Attachment #375122 - Flags: review?(marco.zehe)
Attachment #375122 - Flags: review?(dolske)
Attachment #375122 - Flags: review?(david.bolter)
(Assignee)

Updated

10 years ago
Attachment #375167 - Flags: review?(david.bolter)
(Assignee)

Updated

10 years ago
Attachment #375167 - Flags: review?(dolske)
(Reporter)

Comment 13

10 years ago
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!
Attachment #375167 - Flags: review?(marco.zehe) → review+
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.
Attachment #375167 - Flags: review?(david.bolter) → review+
(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

Updated

10 years ago
Attachment #375167 - Flags: superreview?(neil) → superreview+
(Reporter)

Comment 16

10 years ago
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.
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.
(Reporter)

Comment 18

10 years ago
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?
(Assignee)

Comment 19

10 years ago
(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.
(Assignee)

Comment 20

10 years ago
(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?
(Assignee)

Comment 21

10 years ago
Justin, any chance to review toolkit's part?
(Reporter)

Updated

10 years ago
Depends on: 491443
(Reporter)

Comment 22

10 years ago
(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.
(Assignee)

Comment 23

10 years ago
(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 :) ).
Attachment #375167 - Flags: review?(enndeakin)
Attachment #375167 - Flags: review?(dolske)
Attachment #375167 - Flags: review+
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...
(Assignee)

Comment 25

10 years ago
(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)?
(Assignee)

Comment 27

10 years ago
Posted patch patch3 (obsolete) — Splinter Review
with Justin comments
Attachment #375167 - Attachment is obsolete: true
Attachment #376139 - Flags: review?(enndeakin)
Attachment #375167 - Flags: review?(enndeakin)

Comment 28

10 years ago
Comment on attachment 376139 [details] [diff] [review]
patch3

Seems ok. We should file a bug on creating a real image button type.
Attachment #376139 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 29

10 years ago
(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?
(Assignee)

Comment 30

10 years ago
Posted patch patch4Splinter Review
up to dated to trunk
Attachment #376139 - Attachment is obsolete: true
(Assignee)

Comment 31

10 years ago
checked in http://hg.mozilla.org/mozilla-central/rev/d356b4e0ab66
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 492516
(Reporter)

Comment 32

10 years ago
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)
Status: RESOLVED → VERIFIED
(Assignee)

Comment 33

10 years ago
Comment on attachment 376337 [details] [diff] [review]
patch4

how can we get this on 1.9.1?
Attachment #376337 - Flags: approval1.9.1?
Comment on attachment 376337 [details] [diff] [review]
patch4

a191=beltzner
Attachment #376337 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 35

10 years ago
checked in on 1.9.1 - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6388618abf44
Keywords: fixed1.9.1
(Reporter)

Comment 36

10 years ago
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)
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.