Closed
Bug 338048
Opened 18 years ago
Closed 18 years ago
Richlistbox in Add-ons window not accessible to screenreaders
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta2
People
(Reporter: pilgrim, Assigned: pilgrim)
References
Details
(Keywords: access, fixed1.8.1)
Attachments
(1 file, 7 obsolete files)
4.58 KB,
patch
|
mconnor
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Comment 2•18 years ago
|
||
This also occurs in Thunderbird version 2 alpha 1 (20060529). Should this therefore be considered a core accessibility bug?
Comment 3•18 years ago
|
||
(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.
Comment 5•18 years ago
|
||
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
Comment 6•18 years ago
|
||
(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
Comment 7•18 years ago
|
||
Mark, want to take it from here?
Attachment #229513 -
Flags: review?(pilgrim)
Updated•18 years ago
|
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 8•18 years ago
|
||
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)
Assignee | ||
Comment 9•18 years ago
|
||
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-
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Updated•18 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Updated•18 years ago
|
Assignee: nobody → pilgrim
Status: ASSIGNED → NEW
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
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 12•18 years ago
|
||
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)
Updated•18 years ago
|
Whiteboard: [needs new patch]
Target Milestone: --- → mozilla1.8.1beta2
Assignee | ||
Comment 13•18 years ago
|
||
Assignee | ||
Comment 14•18 years ago
|
||
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)
Assignee | ||
Comment 15•18 years ago
|
||
Attachment #231453 -
Attachment is obsolete: true
Attachment #231463 -
Flags: first-review?(bugs.mano)
Attachment #231453 -
Flags: first-review?(bugs.mano)
Comment 16•18 years ago
|
||
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-
Comment 17•18 years ago
|
||
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.
Assignee | ||
Comment 18•18 years ago
|
||
(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
Comment 19•18 years ago
|
||
You do re-set startEl one line before.
Assignee | ||
Comment 20•18 years ago
|
||
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)
Comment 21•18 years ago
|
||
*** Bug 347425 has been marked as a duplicate of this bug. ***
Comment 22•18 years ago
|
||
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-
Assignee | ||
Comment 23•18 years ago
|
||
*** Bug 347876 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 24•18 years ago
|
||
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 25•18 years ago
|
||
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+
Comment 26•18 years ago
|
||
FYI FWIW, I'm planing to move this code back to the richlistitem implementaion in the 1.9 cycle.
Comment 27•18 years ago
|
||
(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).
Assignee | ||
Comment 28•18 years ago
|
||
fixed nits
Attachment #232853 -
Attachment is obsolete: true
Attachment #232902 -
Flags: approval1.8.1?
Assignee | ||
Updated•18 years ago
|
Whiteboard: [needs new patch] → [checkin needed]
Comment 29•18 years ago
|
||
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+
Comment 30•18 years ago
|
||
mozilla/toolkit/mozapps/extensions/content/extensions.xml 1.34
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Comment 31•18 years ago
|
||
mozilla/toolkit/mozapps/extensions/content/extensions.xml 1.17.2.13
Keywords: fixed1.8.1
Comment 32•5 years ago
|
||
Keywords: sec508
You need to log in
before you can comment on or make changes to this bug.
Description
•