Test environment: Windows XP, Firefox 2 (Bon Echo) build 20060802, Inspect Objects Optional: screen reader (JAWS or Window-Eyes) Description: Two accessibility issues found on the "Main" tab of the Tools-Options dialog. 1. Missing MSAA Name in location text input field 2. Missing "Parent" group property in Download section 3. Contents of "Save files to" selection field is not available to screen reader Steps to recreate: 1. Open Tools-Options dialog (press Alt+T, then "O") 2. Select the "Main" page (Press Tab or Shift+Tab until focus is on one of the icons at the top of the page, then press Right Arrow or Left Arrow until "Main" is selected.) 3. Press Tab to move focus to the "Location" field. 4. Note that Inspect Objects shows the MSAA Name = "" (null). Some screen readers (such as JAWS) use the MSAA Name to tell the user the function of a text field. In this case, the MSAA Name should be the same as the field label, "Location". 5. Tab to the radio buttons in the "Downloads" section of the Main page. 6. If not already selected, use Up Arrow or Down Arrow to select the "Save files to" radio button. 7 When viewing Inspect Objects you will notice that the parent property is null. The screen reader does not announce the group heading. 8. The screen reader (JAWS or Window-Eyes) says only "Save files to, Radio button, checked", or something similar. It does not read the contents of the selection box (default = "Desktop"), located between the radio button and the "Browse" button. 9. Press Tab. Focus moves to the "Browse" button. Note, again, the word "Desktop" was not spoken by the screen reader. The screen reader user is unaware of the default selection "Desktop". 10. Press Spacebar to select the "Browse" button. The "Browse for Folder" dialog pops up. Note that the default selection in the "Browse for folder" dialog is NOT the same as the current selection displayed in the "Save Files to" selection box. A blind user has no indication of what the default value is, and has no way of finding out. Note: Visual constructions like this (radio button, followed by another UI element like a input field or combo box) are not very easy to understand when using a screen reader, since the screen reader often reads only one element at a time, and not the entire line, as a sighted user would naturally do. Expected results: 1. The MSAA Name property should be set for all User Interface elements. 2. The "Save files to" selection box should be added to the tab ring (even if it is only read-only) so the screen reader will read the information.

The Location-textbox disconnect should be fixed; it would be helpful if you could separate the list of problems into discrete items to be addressed so that it's easier to tackle an item and easier to verify that all issues have been fixed in the patch.

Confirmed: the location textbox now has the proper accessible name.

5-10 is a single, valid bug report. Re-titling and assigning to self.

Mark, is this fixable for Fx2 or is Firefox 3 the realistic target here?

No, we can't fix this for Firefox 2 until we fix some underlying issues in nsAccessible.GetXULName (it doesn't get the value of radio buttons, checkboxes, or filefields when constructing the name from an hbox's child nodes). That's not realistic for Firefox 2, retargeting.

<filefield> appears not to expose it's contents at all, not sure how we missed that before.

There is no nsIAccessibleProvider implementation for <filefield>. There is also no <implementation implements="nsIDOMXULLabeledControlElement"> although that would seem to fit.

Sorry, I'm wrong -- it exposes itself as a textbox with the accessible value set to the file name.

This fixes all the current uses of <filefield> except in feeds.xul

Comment on attachment 236927 [details] [diff] [review] Also remove xhtml2 and wairole namespaces where no longer needed (role no longer necessary in order to use labelledby) >Index: accessible/src/base/nsAccessible.cpp >=================================================================== Someone else should review this change. > >Index: toolkit/mozapps/preferences/preferences.xml >=================================================================== >+ <implementation implements="nsIDOMXULLabeledControlElement"> > <property name="image"> ... > </property> > <property name="label"> ... > </property> There are few more properties in this iface, please implement them (per the label, if a property does not make sense throw NOT_IMPLEMENTED).

This should inherit from control-item via extends="chrome://global/content/bindings/general.xml#control-item> The problem with doing that on branch is that <filefield> confuses label and value. The value should be what goes into the <filefield> and the label is usually to the left of that. Example: Patch to upload: [ /home/aaron/diff/mypatch.diff ] LABEL VALUE Also, <filefield> should be in toolkit/content/widgets instead of toolkit/mozapps/preferences, IMO. I suggest we fix those things on trunk. For branch I plan to submit a simpler patch.

The other fixes will come shortly. A future bug should be filed to separate the value and label, but perhaps that should be fixed only on trunk. This makes filefield support nsIDOMXULLabeledControlElement, and also fixes a bug with how the disabled attribute worked. It had a strange implemenation that would set the disabled attribute instead of removing it. As a result, filefield.disable = 'false' was removing the filefield from the tab order unintentionally.

Patch for mozilla/accessible will come up shortly and use a different reviewer.

Comment on attachment 237353 [details] [diff] [review] <filefield> binding fixes only r=mano

Comment on attachment 237354 [details] [diff] [review] Fix uses of aaa:labelledby, for filefields and in a few other places. A role is no longer needed, and an idlist can be used. Index: browser/components/preferences/advanced.xul =================================================================== <!-- Cache --> <groupbox id="cacheGroup"> <caption label="&cache.label;"/> - <hbox xhtml2:role="wairole:label" id="cacheSizeBox" align="center"> + <hbox id="cacheSizeBox" align="center"> General comment: get rid of the xhtml2 namespace in any place where it's no longer needed (probably everywhere). r=mano otherwise, assuming you've tested this.

(In reply to comment #16) > General comment: get rid of the xhtml2 namespace in any place where it's no > longer needed (probably everywhere). Yes, already done. Ragarding advanced.xul, xhtml2/wairole are still needed on 1 element Also, aaa is only needed on 1 element in that file. Do you want me to move those xmlns declarations to the element that uses them, instead of higher up in the DOM?

Comment on attachment 237493 [details] [diff] [review] Same as previous <filefield> binding fixes patch, but also explicitly declare implements="nsIDOMXULLabeledControlElement", because that is not inherited from control-item (surprising to me) OK, this will do for now, r=mano (though, really, we need a better interface here which exposes just the label and the accesskey).

Comment on attachment 237491 [details] [diff] [review] Generalize AppendFlatStringFromSubtree better, so that it can handle any labelled control element and doesn't walk into subtrees of labelled options >- else if (!aContent->GetAttr(kNameSpaceID_None, >- nsAccessibilityAtoms::tooltiptext, textEquivalent) || >- textEquivalent.IsEmpty()) { >- AppendNameFromAccessibleFor(aContent, aFlatString, PR_TRUE /* use value */); >+ else { >+ aContent->GetAttr(kNameSpaceID_None, >+ nsAccessibilityAtoms::tooltiptext, textEquivalent); > } >+ AppendNameFromAccessibleFor(aContent, &textEquivalent, PR_TRUE /* use value */); Why don't you need to check for empty tooltiptext any more?

> Why don't you need to check for empty tooltiptext any more? Neil, we always want to append the accessible value of any control, whether it had tooltiptext or not. We shouldn't have done it that way in the first place.

Can we confirm no l10n impact here?

I can't see a direct impact on l10n. Or managed to figure out a way for an indirect one.

No l10n impact

Comment on attachment 237354 [details] [diff] [review] Fix uses of aaa:labelledby, for filefields and in a few other places. A role is no longer needed, and an idlist can be used. a=schrep for accessibility fix for new prefs window.

Comment on attachment 237491 [details] [diff] [review] Generalize AppendFlatStringFromSubtree better, so that it can handle any labelled control element and doesn't walk into subtrees of labelled options a=schrep for accessibility fix for new prefs window.

Comment on attachment 237493 [details] [diff] [review] Same as previous <filefield> binding fixes patch, but also explicitly declare implements="nsIDOMXULLabeledControlElement", because that is not inherited from control-item (surprising to me) a=schrep for accessibility fix for new prefs window.