The screen reader experience is a whole lot more friendly when each field has correct names and descriptions.

I'll look at your C++ changes if you separate them out and explain them. But I can't easily review changes to files I don't build.

Comment on attachment 190731 [details] [diff] [review] Long but simple: 1) Hook up label & description, 2) Fix whitespace & description rules in a11y 3) Fix single selection in a11y, 4) make selection match focus for pref icons (spoken better) Separating out CPP changes for Neil's review

1) Allow accessible description to use retrive it either from aaa:describedby attribute or <xul:description control="[id]".../> 2) Allow values to be part of an accessible name for a XUL container, so that something like: "Get mail every _3__ minutes" gets the accessible name "Get mail every 3 minutes", otherwise the form label for that control makes little sense "Get mail every" or "Get mail every minutes". We already do this for HTML. 3) Don't compress whitespace while aggregating accessible names, this just leads to words jammed up against each other. The screen reader can remove extra whitespace as necessary. We already do this for HTML. 4) There was a simple mistake in AppendFlatStringFromSubtreeRecurse where it was supposed to call itself again, not call it's caller which removed trailing spaces each time. This let to results where spaces were removed in the middle of a label after a mnenomenic, because were are walking the anon content there, and the underlined letter has its own node. 5) Bump kAncestorLevelsToSearch up to 5, because we weren't finding the description when there was a xul:grid with controls in it, and a description outside of the grid. 6) Don't assume that focused elements in a single select widget are selected. This may not be the case when the list first gets focused. The selected state needs to be managed by anyone using DHTML a11y (which we use at times in XUL) 7) Similarly don't fire selection events for a single select widget based on a focused change. The selection needs to be managed separately from focus in single select widgets.

Comment on attachment 190731 [details] [diff] [review] Long but simple: 1) Hook up label & description, 2) Fix whitespace & description rules in a11y 3) Fix single selection in a11y, 4) make selection match focus for pref icons (spoken better) > // Expose preference group choice to accessibility APIs as an unchecked list item > // The parent group is exposed to accessibility APIs as a list >- radio.setAttributeNS("http://www.w3.org/TR/xhtml2", "role", "wairole:listitem"); >- radio.setAttributeNS("http://www.w3.org/2005/07/aaa", "checked", "false"); shouldn't this comment go away too? >- <description>&downloadManager.intro;</description> >+ <description control="viewDownloads" >&downloadManager.intro;</description> nit: extra space r=me

Comment on attachment 190946 [details] [diff] [review] CPP changes described below You're still calling description.CompressWhitespace before returning (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsAcce ssible.cpp#232). Won't that jam words together too? Or were the multiple calls to CompressWhitespace the problem?

(In reply to comment #6) > (From update of attachment 190946 [details] [diff] [review] [edit]) > You're still calling description.CompressWhitespace before returning > (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsAcce > ssible.cpp#232). Won't that jam words together too? Or were the multiple calls > to CompressWhitespace the problem? > No, because a description is treated as a separate island of text for a widget, and may or may not be spoken depending on verbosity settings in the screen reader.

Comment on attachment 190946 [details] [diff] [review] CPP changes described below >Index: accessible/src/base/nsAccessible.cpp >+ nsIAtom *descAtom = isXUL ? nsAccessibilityAtoms::tooltiptext : nsAccessibilityAtoms::title; nit: wrap long lines at 80 chars >+ if (NS_CONTENT_ATTR_HAS_VALUE == >+ content->GetAttr(kNameSpaceID_None, descAtom, description)) { >+ nsAutoString name; >+ GetName(name); >+ if (name.IsEmpty() || description == name) { >+ // Don't use tooltip for a description if this objet s/objet/object/ ? >+ aContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::tooltiptext, >+ textEquivalent)) { >+ AppendNameFromAccessibleFor(aContent, aFlatString, PR_TRUE /* use value */); nit: break long lines at 80 chars :) sr=darin

Checking in toolkit/components/passwordmgr/resources/content/passwordManager.xul; /cvsroot/mozilla/toolkit/components/passwordmgr/resources/content/passwordManager.xul,v <-- passwordManager.xul new revision: 1.10; previous revision: 1.9 done Checking in toolkit/content/widgets/preferences.xml; /cvsroot/mozilla/toolkit/content/widgets/preferences.xml,v <-- preferences.xml new revision: 1.31; previous revision: 1.30 done Checking in toolkit/mozapps/preferences/changemp.js; /cvsroot/mozilla/toolkit/mozapps/preferences/changemp.js,v <-- changemp.js new revision: 1.2; previous revision: 1.1 done Checking in toolkit/mozapps/preferences/changemp.xul; /cvsroot/mozilla/toolkit/mozapps/preferences/changemp.xul,v <-- changemp.xul new revision: 1.2; previous revision: 1.1 done Checking in toolkit/mozapps/preferences/fontscaling.xul; /cvsroot/mozilla/toolkit/mozapps/preferences/fontscaling.xul,v <-- fontscaling.xul new revision: 1.2; previous revision: 1.1 done Checking in toolkit/mozapps/preferences/ocsp.xul; /cvsroot/mozilla/toolkit/mozapps/preferences/ocsp.xul,v <-- ocsp.xul new revision: 1.5; previous revision: 1.4 done Checking in toolkit/mozapps/preferences/removemp.xul; /cvsroot/mozilla/toolkit/mozapps/preferences/removemp.xul,v <-- removemp.xul new revision: 1.2; previous revision: 1.1 done Checking in toolkit/mozapps/preferences/update.xul; /cvsroot/mozilla/toolkit/mozapps/preferences/update.xul,v <-- update.xul new revision: 1.3; previous revision: 1.2 done Checking in browser/components/preferences/advanced.xul; /cvsroot/mozilla/browser/components/preferences/advanced.xul,v <-- advanced.xul new revision: 1.16; previous revision: 1.15 done Checking in browser/components/preferences/changeaction.xul; /cvsroot/mozilla/browser/components/preferences/changeaction.xul,v <-- changeaction.xul new revision: 1.6; previous revision: 1.5 done Checking in browser/components/preferences/cookies.xul; /cvsroot/mozilla/browser/components/preferences/cookies.xul,v <-- cookies.xul new revision: 1.5; previous revision: 1.4 done Checking in browser/components/preferences/downloadactions.xul; /cvsroot/mozilla/browser/components/preferences/downloadactions.xul,v <-- downloadactions.xul new revision: 1.7; previous revision: 1.6 done Checking in browser/components/preferences/downloads.xul; /cvsroot/mozilla/browser/components/preferences/downloads.xul,v <-- downloads.xul new revision: 1.8; previous revision: 1.7 done Checking in browser/components/preferences/general.xul; /cvsroot/mozilla/browser/components/preferences/general.xul,v <-- general.xul new revision: 1.7; previous revision: 1.6 done Checking in browser/components/preferences/permissions.xul; /cvsroot/mozilla/browser/components/preferences/permissions.xul,v <-- permissions.xul new revision: 1.6; previous revision: 1.5 done Checking in browser/components/preferences/privacy.xul; /cvsroot/mozilla/browser/components/preferences/privacy.xul,v <-- privacy.xul new revision: 1.8; previous revision: 1.7 done Checking in browser/components/preferences/sanitize.xul; /cvsroot/mozilla/browser/components/preferences/sanitize.xul,v <-- sanitize.xul new revision: 1.9; previous revision: 1.8 done Checking in accessible/src/base/nsAccessible.cpp; /cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v <-- nsAccessible.cpp new revision: 1.165; previous revision: 1.164 done Checking in accessible/src/base/nsRootAccessible.cpp; /cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v <-- nsRootAccessible.cpp new revision: 1.130; previous revision: 1.129 done

Look's like maybe the first patch broke Tools-> Options-> Downloads-> View & Edit Actions-> and double-click ANY of the available file types. You'll receive and XML parsing error. Screenie coming up! ~B