Closed
Bug 489549
Opened 15 years ago
Closed 15 years ago
Buttons of HTML5 audio and video element control set have no accessible names
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
People
(Reporter: MarcoZ, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access, verified1.9.1)
Attachments
(1 file, 6 obsolete files)
32.62 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 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•15 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•15 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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Attachment #374738 -
Attachment is obsolete: true
Attachment #375122 -
Flags: superreview?(neil)
Attachment #375122 -
Flags: review?(marco.zehe)
Assignee | ||
Updated•15 years ago
|
Attachment #375122 -
Flags: review?(david.bolter)
Assignee | ||
Updated•15 years ago
|
Attachment #375122 -
Flags: review?(dolske)
Assignee | ||
Comment 8•15 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•15 years ago
|
Flags: in-testsuite+
Comment 9•15 years ago
|
||
Does this actually work? I thought videocontrols ran with page privileges, which would prevent them accessing components.
Assignee | ||
Comment 10•15 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 11•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
Attachment #375167 -
Flags: review?(david.bolter)
Assignee | ||
Updated•15 years ago
|
Attachment #375167 -
Flags: review?(dolske)
Reporter | ||
Comment 13•15 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 14•15 years ago
|
||
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+
Comment 15•15 years ago
|
||
(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•15 years ago
|
Attachment #375167 -
Flags: superreview?(neil) → superreview+
Reporter | ||
Comment 16•15 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.
Comment 17•15 years ago
|
||
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•15 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•15 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•15 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•15 years ago
|
||
Justin, any chance to review toolkit's part?
Reporter | ||
Comment 22•15 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•15 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 :) ).
Updated•15 years ago
|
Attachment #375167 -
Flags: review?(enndeakin)
Attachment #375167 -
Flags: review?(dolske)
Attachment #375167 -
Flags: review+
Comment 24•15 years ago
|
||
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•15 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)?
Comment 26•15 years ago
|
||
No.
Assignee | ||
Comment 27•15 years ago
|
||
with Justin comments
Attachment #375167 -
Attachment is obsolete: true
Attachment #376139 -
Flags: review?(enndeakin)
Attachment #375167 -
Flags: review?(enndeakin)
Comment 28•15 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•15 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•15 years ago
|
||
up to dated to trunk
Attachment #376139 -
Attachment is obsolete: true
Assignee | ||
Comment 31•15 years ago
|
||
checked in http://hg.mozilla.org/mozilla-central/rev/d356b4e0ab66
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 32•15 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•15 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 34•15 years ago
|
||
Comment on attachment 376337 [details] [diff] [review] patch4 a191=beltzner
Attachment #376337 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 35•15 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•15 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.
Description
•