302359 - Options dialog polish -- correct labels and descriptions

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Aaron Leventhal]

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

Comment 1

[Aaron Leventhal]

Attachment: 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)

Comment 2

[[e-mail]]

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 3

[Aaron Leventhal]

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

Comment 4

[Aaron Leventhal]

Attachment: CPP changes described below

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 5

[Mike Connor [:mconnor]]

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 6

[parente]

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?

Comment 7

[Aaron Leventhal]

(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 8

[Darin Fisher]

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

Comment 9

[Aaron Leventhal]

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

Comment 10

[Bryan]

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

Comment 11

[Ben Basson]

Attachment: One-character XML Parsing Error fix