Closed Bug 338048 Opened 15 years ago Closed 14 years ago

Richlistbox in Add-ons window not accessible to screenreaders

Categories

(Toolkit :: XUL Widgets, defect)

1.8 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: pilgrim, Assigned: pilgrim)

References

Details

(Keywords: access, fixed1.8.1)

Attachments

(1 file, 7 obsolete files)

Steps to reproduce:

- Run Firefox 2.0a2 and Microsoft Inspect32 (or WindowEyes 5.5).
- Select Tools > Add-ons.  Extensions pane appears with first extension selected.
- WindowEyes speaks "Add-ons" (window title), then "Requires additional items.  Disabled for your protection."  Inspect32 confirms that the focus is on a list item whose accessible name is "Requires additional items.  Disabled for your protection" and accessible description is "1 of 2" (assuming clean profile with DOM Inspector and Talkback installed).  I was expecting it to speak "DOM Inspector 1.8.1a2" plus the extension's description.
- Press down arrow.  Second extension is selected.
- WindowEyes speaks "Custom control".  Inspect32 shows focus on a list item whose accessible name is "Requires additional items.  Disabled for your protection" and accessible description is "2 of 2".  I was expecting it to speak "Talkback 2.0a2" plus the extension's description.
Additional research reveals that nsAccessible::GetXULName is trying to build the accessible name by concatenating the text values of the child labels of the richlistitem (since the richlistitem itself doesn't have a label attribute).  This partially explains the results on the initial richlistitem; the accessible name ends up as a combination of two <xul:label> elements that are inside the richlistbox:

http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.xml#186

If these warnings are not to be displayed, the richlistbox is given custom attributes which sets the style of the contained labels to display:none.

http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/mozapps/extensions/extensions.css#229

DOM Inspector confirms that the individual labels have a computed style of display:none.  The XUL "hidden" attribute is still "false" though; the invisibility is accomplished entirely in CSS.

Even so, nsAccessible::GetXULName ought to recognize that the labels are, in fact, invisible and skip over them while concatenating the accessible name.  But that doesn't seem to be happening.  The relevant code appears to be in nsAccessible::AppendFlatStringFromContentNode:

http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#1252

Aaron Leventhal confirms that the proper solution is to make nsAccessible::AppendFlatStringFromContentNode skip over any element which is currently invisible, whether via a XUL "hidden" attribute or by a CSS display:none or visibility:hidden rule.
Status: NEW → ASSIGNED
This also occurs in Thunderbird version 2 alpha 1 (20060529). Should this therefore be considered a core accessibility bug?
(In reply to comment #1)
Mark, what are the chances of the core accessibility bug being fixed by Firefox 2.0? I can probably come up with a workaround in the xbl for the extension manager if necessary but I would of course prefer if the core problem was fixed.
Opened Bug 340261 for the bug as described in comment #1
Depends on: 340261
If Bug 340261 isn't fixed for Firefox 2.0 I can probably change the xbl to fix this bug with the Add-ons Mgr. but I would prefer doing so sooner rather than later so an answer to comment #3 would be appreciated. Thanks
(In reply to comment #1)
> Additional research reveals that nsAccessible::GetXULName is trying to build
> the accessible name by concatenating the text values of the child labels of 
This is not what happens in the richlistitem case.

We do check visibility. See
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#1304

The richlistitem bugs are actually caused by the richlistitem GetLabel implementation which is used when nsIDOMXULSelectControlItemElement is supported
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#1630

The richlistitem label implementation is here:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/richlistbox.xml#521
Attached patch Not tested. Should work. (obsolete) — Splinter Review
Mark, want to take it from here?
Attachment #229513 - Flags: review?(pilgrim)
Status: ASSIGNED → NEW
Component: Extension/Theme Manager → XUL Widgets
Flags: review?(pilgrim) → blocking-firefox2?
Product: Firefox → Toolkit
QA Contact: extension.manager → xul.widgets
Version: 2.0 Branch → 1.8 Branch
Comment on attachment 229513 [details] [diff] [review]
Not tested. Should work.

Mark, will you be able to do this before beta 2?
Attachment #229513 - Flags: first-review?(pilgrim)
Comment on attachment 229513 [details] [diff] [review]
Not tested. Should work.

This patch has no effect because the hidden labels are being hidden by CSS, not XUL attributes.  You will need to use getComputedStyle and check @display and @visibility styles.
Attachment #229513 - Flags: first-review?(pilgrim) → first-review-
Status: NEW → ASSIGNED
Flags: blocking-firefox2? → blocking-firefox2+
Assignee: nobody → pilgrim
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
OK, this bug is much trickier than it seems, and this patch is much uglier than I would like.

1. The items in the richlistbox in the Addons window use a <xul:hbox> for the name/version line.  This has a classname which (via CSS) is bound to an XBL binding that creates two <xul:label>s, one for name, one for version.  This is Addons-window-specific, so I feel the Addons window needs its own label property getter.

2. The description line that describes the extension/theme (such as "Inspect the DOM of HTML, XUL, and XML pages, including the browser chrome" for DOM Inspector) uses xbl:inherits="xbl:text=description" to convert the description attribute of the parent richlistitem to a series of child text nodes within a <xul:label>.  The generic richlistbox label property getter only looks at the value attribute of each <xul:label>, not child text nodes.  Arguably this is a bug in richlistbox.xml, but since we're using a custom label property getter in extensions.xml anyway, I'm not going to fix it in this bug or block on this bug.

3. Each richlistitem has a series of status messages, such as "this extension is disabled for your protection" (for blacklisted extensions) and so on.  These are each a <xul:label> inside a <xul:hbox>.  The <xul:hbox> has a classname which triggers certain CSS rules which in turn determine whether it (and therefore its containing <xul:label>) is displayed (by setting display:none if the status message does not apply).  I can't decide if this is wickedly brilliant or wickedly bad, but I had to use getComputedStyle on each <xul:label>'s parent node to determine whether the <xul:label>'s text should be added to overall label property value.

None of this points to a good clean generic solution that we can implement for any future richlistitems in richlistbox.xml, so I've left that file completely untouched and implemented everything in extensions.xml.  But beware that other windows that use richlistbox in funky ways may need custom label property getters that understand the structure of their particular richlistitems.
Attachment #229513 - Attachment is obsolete: true
Attachment #230608 - Flags: second-review?(aaronleventhal)
Attachment #230608 - Flags: first-review?(bugs.mano)
Comment on attachment 230608 [details] [diff] [review]
addons-window-specific patch to properly set label in richlistbox items

You should ask robert for a real review on this code.

Some general comments though

>Index: toolkit/mozapps/extensions/content/extensions.xml
>===================================================================

>+      <property name="label">
>+        <!-- Setter purposely not implemented; the getter returns a
>+             concatentation of label text to expose via accessibility APIs-->

well, make it <property... readonly="true">?

>+        <getter>
>+          <![CDATA[
>+            var labelText = "";
>+            var startEl = document.getAnonymousNodes(this)[0];
>+            if (startEl) {
>+              // Manually add name and version, stored as
>+              // anonymous <xul:label>s within a <xul:hbox>.
>+              var nameVersion = startEl.childNodes[1].firstChild;
>+              var nameVersionAnonNodes = document.getAnonymousNodes(nameVersion);
>+              labelText += nameVersionAnonNodes[0].value + ' ' + nameVersionAnonNodes[1].value + ' ';

How l10n freindly is this growing string?

Also, please use anonids and corresponding xbl fields, then you wouldn't need to do things like this:
 
>+              startEl = startEl.childNodes[1].childNodes[2];


>+            }
>+            if (startEl) {
>+              // Add text of selected status messages, if any are visible.
>+              // Note 1: visibility of status messages is set by CSS styles,
>+              // not XUL attributes, so we need to use getComputedStyle.

Where's that style rule set? I see hideMessage using the hidden property.

>+              // Note 2: CSS styles may be set on the label's parent node
>+              // (usually a <xul:hbox>) instead of the parent itself, so
>+              // we need to check the parent node's getComputedStyle too.
>+              var labels = 
>+                startEl.getElementsByTagNameNS('http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul',
>+                                              'label');
>+              var numLabels = labels.length;
>+              for (count = 0; count < numLabels; count ++) {
>+                var label = labels[count];
>+                var style = getComputedStyle(label, '');
>+                var labelParent = label.parentNode;
>+                var parentStyle;
>+                if (labelParent) {
>+                  parentStyle = getComputedStyle(labelParent, '');
>+                } else {
>+                  parentStyle = style;
>+                }

If it turns out you do need to use getComputedStyle, please use document.defaultView.getComputedStyle rather than assuming the script global object implements it.

>+                if (!label.collapsed && !label.hidden &&
>+                    label.className != 'text-link' &&
>+                    style.display != 'none' &&
>+                    style.visibility != 'hidden' &&
>+                    parentStyle.display != 'none' &&
>+                    parentStyle.visibility != 'hidden') {
>+                  labelText += label.value + ' ';

s/'/"/g. Again, i'm not sure how l10n friendly is this.
Attachment #230608 - Flags: first-review?(bugs.mano) → first-review-
Comment on attachment 230608 [details] [diff] [review]
addons-window-specific patch to properly set label in richlistbox items

I'll wait for the next patch.
Attachment #230608 - Flags: second-review?(aaronleventhal)
Whiteboard: [needs new patch]
Target Milestone: --- → mozilla1.8.1beta2
Attached file non-working patch for discussion (obsolete) —
Attached patch incorporated mano's feedback (obsolete) — Splinter Review
Incorporated all of mano's feedback.  WRT localization, nsAccessible does the same thing for complex widgets -- just appends child text values together in the order in which they appear in the DOM, separated by spaces (AppendFlatStringFromSubtree or something like that).
Attachment #230608 - Attachment is obsolete: true
Attachment #231442 - Attachment is obsolete: true
Attachment #231453 - Flags: first-review?(bugs.mano)
Attachment #231453 - Attachment is obsolete: true
Attachment #231463 - Flags: first-review?(bugs.mano)
Attachment #231453 - Flags: first-review?(bugs.mano)
Comment on attachment 231463 [details] [diff] [review]
patch with mano's latest feedback from irc


>+            var labelText = "";
>+            var startEl = document.getAnonymousNodes(this)[0];
>+            if (startEl) {

Why? you didn't use it (document.getAnonymousNodes(this)[0]).


>+              labelText += name.value + " " + version.value + " ";
>+              // Manually add description.
>+              var descriptionWrap = document.getAnonymousElementByAttribute(this, "anonid", "addonDescriptionWrap");
>+              labelText += descriptionWrap.value;
>+              // reset startEl to selectedStatusMsgs node
>+              startEl = document.getAnonymousElementByAttribute(this, "anonid", "addonSelectedStatusMsgs");
>+              // Add text of selected status messages, if any are visible.
>+              // Note 1: visibility of status messages is set by CSS styles,
>+              // not XUL attributes, so we need to use getComputedStyle.

My question from comment 11 remains.

>+              // Note 2: CSS styles may be set on the label's parent node
>+              // (usually a <xul:hbox>) instead of the parent itself, so

instead of the /node/ itself?

>+              // we need to check the parent node's getComputedStyle too.
>+              var labels = 
>+                startEl.getElementsByTagNameNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                              "label");
>+              var numLabels = labels.length;
>+              for (count = 0; count < numLabels; count ++) {
>+                var label = labels[count];
>+                var style = document.defaultView.getComputedStyle(label, "");
>+                var labelParent = label.parentNode;
>+                var parentStyle;
>+                if (labelParent) {
>+                  parentStyle = document.defaultView.getComputedStyle(labelParent, "");

Well, css properties could be set on the parent-parent-node as well. If you're dealing with inherited css properties, you should check the computed style of the node itself.

Anyway, this is a private binding, you only need to take care of the addon manager (and the default themes) needs and not every way a theme or an extension would try to break it:

>+                } else {
>+                  parentStyle = style;
>+                }
>+                if (!label.collapsed && !label.hidden &&
>+                    label.className != "text-link" &&
>+                    style.display != "none" &&
>+                    style.visibility != "hidden" &&
>+                    parentStyle.display != "none" &&
>+                    parentStyle.visibility != "hidden") {
>+                  labelText += label.value + " ";
>+                }
>+              }
>+            }
>+            return labelText;
>+          ]]>
>+        </getter>
>+      </property>
Attachment #231463 - Flags: first-review?(bugs.mano) → first-review-
Also, please add a xbl field instead of calling getAnonymousElementByAttribute each time, see http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/scrollbox.xml#45 for example.
(In reply to comment #16)
> (From update of attachment 231463 [details] [diff] [review] [edit])
> 
> >+            var labelText = "";
> >+            var startEl = document.getAnonymousNodes(this)[0];
> >+            if (startEl) {
> 
> Why? you didn't use it (document.getAnonymousNodes(this)[0]).

Yes I do.

var labels = startEl.getElementsByTagNameNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "label");

> >+              // Add text of selected status messages, if any are visible.
> >+              // Note 1: visibility of status messages is set by CSS styles,
> >+              // not XUL attributes, so we need to use getComputedStyle.
> 
> My question from comment 11 remains.

http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/mozapps/extensions/extensions.css#246
You do re-set startEl one line before.
also fixed regression from previous patch in accessing name/version anon nodes within <xul:hbox class="addon-name-version">
Attachment #231463 - Attachment is obsolete: true
Attachment #232802 - Flags: first-review?(bugs.mano)
*** Bug 347425 has been marked as a duplicate of this bug. ***
Comment on attachment 232802 [details] [diff] [review]
uses xbl fields for accessing nodes

>Index: toolkit/mozapps/extensions/content/extensions.xml
>===================================================================

>+      <property name="label" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            var labelText = "";
>+            // Manually add name and version, stored as
>+            // anonymous <xul:label>s within a <xul:hbox>.
>+            var nameVersion = this._nameVersion;
>+            var nameVersionChildren = document.getAnonymousNodes(nameVersion);
>+            var name = nameVersionChildren[0];
>+            var version = nameVersionChildren[1];
>+            labelText += name.value + " " + version.value + " ";

General comment: use the fields directly (i.e. never do var foo = this._foo). Fields, unlike properties, are cached.

You can just get the attributes out of this._nameVersion, the following should work

var labelText = this._nameVersion.getAttribute("name") + " " +
                this._nameVersion.getAttribute("version");


>+            // Add text of selected status messages, if any are visible.
>+            // Note 1: visibility of status messages is set by CSS styles,
>+            // not XUL attributes, so we need to use getComputedStyle.
>+            // Note 2: relevant CSS style is set on the label's parent node;
>+            // the label itself has "display:-moz-box" which is unhelpful.
>+            startEl = this._selectedStatusMsgs;

declare startEl.

>+            var labels = 
>+              startEl.getElementsByTagNameNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                            "label");
>+            var numLabels = labels.length;
>+            for (count = 0; count < numLabels; count ++) {

s/count/i/ (and declare i!).

>+              var label = labels[count];
>+              var style = document.defaultView.getComputedStyle(label, "");
>+              var labelParent = label.parentNode;
>+              var parentStyle;
>+              if (labelParent) {
>+                parentStyle = document.defaultView.getComputedStyle(labelParent, "");
>+              } else {
>+                parentStyle = style;

labelParent is never null here (at worst, its parent is startEl itself).

>+              }
>+              if (!label.collapsed && !label.hidden &&
>+                  label.className != "text-link" &&
>+                  parentStyle.display != "none" &&
>+                  parentStyle.visibility != "hidden"

As I said before, this isn't a global binding, you should only keep the checks on label.hidden, label.className and parentStyle.dispaly.

Using a strings array and returning |arrayName.join(" ")| would be much better (see http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:join).
Attachment #232802 - Flags: first-review?(bugs.mano) → first-review-
*** Bug 347876 has been marked as a duplicate of this bug. ***
Attached patch and again (obsolete) — Splinter Review
Incorporated all of Mano's feedback, despite disagreeing with what I feel is the over-optimization of the visibility check.
Attachment #232802 - Attachment is obsolete: true
Attachment #232853 - Flags: first-review?(bugs.mano)
Comment on attachment 232853 [details] [diff] [review]
and again

>Index: toolkit/mozapps/extensions/content/extensions.xml
>===================================================================

>+      <property name="label" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            var labelPieces = [];
>+            // Add name and version
>+            labelPieces.push(this._nameVersion.getAttribute("name"));
>+            labelPieces.push(this._nameVersion.getAttribute("version"));
>+            // Add description
>+            labelPieces.push(this._descriptionWrap.value);

nit: add an empty line before the comment
>+            // Add selected status messages, if any are visible.
>+            // Note 1: visibility of status messages is set by CSS styles,
>+            // not XUL attributes, so we need to use getComputedStyle.
>+            // Note 2: relevant CSS style is set on the label's parent node;
>+            // the label node itself always has "display:-moz-box" which is
>+            // not useful.

nit: s/CSS styles/CSS rules/g

>+            var labels = this._selectedStatusMsgs.getElementsByTagNameNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "label");

this line is really too long.

>+            var numLabels = labels.length;
>+            for (var i = 0; i < numLabels; i++) {
>+              var label = labels[i];
>+              var parentStyle = document.defaultView.getComputedStyle(label.parentNode, "");
>+              // Optimization: we only check a few cases here that we know
>+              // are used by the Add-ons window.  For example, the generic
>+              // richlistbox.xml label getter checks label.collapsed, but
>+              // we don't check that here because we know that the Add-ons
>+              // window doesn't use it.
>+              if (!label.hidden &&
>+                  label.className != "text-link" &&
>+                  parentStyle.display != "none") {
>+                labelPieces.push(label.value);
>+              }
>+            }

An empty line here would be welcome too.

>+            return labelPieces.join(" ");
>+          ]]>


r=mano otherwise.
Attachment #232853 - Flags: first-review?(bugs.mano) → first-review+
FYI FWIW, I'm planing to move this code back to the richlistitem implementaion in the 1.9 cycle.
(Also note you _could_ use this.getElemenetsByTagName directly instead of adding the name and version labels separately; what you have now is good enough for Fx2 though).
Attached patch final patchSplinter Review
fixed nits
Attachment #232853 - Attachment is obsolete: true
Attachment #232902 - Flags: approval1.8.1?
Whiteboard: [needs new patch] → [checkin needed]
Comment on attachment 232902 [details] [diff] [review]
final patch

a=mconnor on behalf of drivers for 1.8 branch checkin
Attachment #232902 - Flags: approval1.8.1? → approval1.8.1+
mozilla/toolkit/mozapps/extensions/content/extensions.xml 	1.34
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
mozilla/toolkit/mozapps/extensions/content/extensions.xml 	1.17.2.13 	
Keywords: fixed1.8.1
Depends on: 348343
You need to log in before you can comment on or make changes to this bug.