719770 - Various accessibility markup problems with the XUL of easy add-on prefs

Status: RESOLVED INACTIVE

(Toolkit :: Add-ons Manager, defect)

Description

[Marco Zehe (:MarcoZ)]

1. With bols (checkboxes), the checkbox and the associated label are not linked together. As such, the checkbox itself is not labelled. From other XUL, the value attribute is used to label the checkbox, this does not seem to happen here, instead, a separate label appears to be used.
2. With a group of radio buttons, the heading label is not assiciated with the radio group. It should be via its control attribute, or alternatively, by a dynamically inserted aria-labelledby attribute on the radio group pointing to the ID of the header label.
3. There may be problems with buttons, but the extension that I saw, the Adblock Plus Customizations add-on, exposes these two problems, as will most probably do most other add-ons using these new customization UI snippets.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: patch

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 593288 [details] [diff] [review]
patch

Review of attachment 593288 [details] [diff] [review]:
-----------------------------------------------------------------

So.... this is gonna sound a little crazy at first (and possibly afterwards too). But:

IDs in XBL lead to misery and despair. Have a look at how the normal XUL checkbox binding makes itself accessible:
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/checkbox.xml

Specifically, it implements nsIAccessibleProvider, with the accessibleType property set to nsIAccessibleProvider.XULCheckbox. That corresponds to nsIDOMXULCheckboxElement, which exposes properties (like checked) used by the accessibility code. That binding also extends general.xml#basetext, which implements nsIDOMXULLabeledControlElement - and that exposes the label to the accessibility code.

It means you have to expose a few extra properties, but it gets rid of the IDs in XBL hack.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

I had TryServer make some builds of this patch. A few settings types haven't been fixed yet.
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-574bf02d5f20/

There's a new version of the example addon here, which has all the various types.
https://bugzilla.mozilla.org/attachment.cgi?id=597354&action=edit

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 597358 [details] [diff] [review]
patch v2

Unfortunately this is not right yet. The type and such were never the problem, so I don't think it is necessary to reimplement these methods for the nsIAccessibleProviders again. They inherit it from the XUL elements used. The problem is the *labels* for checkboxes, for example, and the labels for grouping elements. Radio buttons work, they even give a nice description what "on", "off", "default" etc. mean, but the other settings themselves do not yet work better.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> so I don't think it is necessary to reimplement these methods for
> the nsIAccessibleProviders again

It's a prerequisite to fix this without using hacks, since the inline prefs are actually re-implementing those widgets in XBL (using the control attribute requires using element IDs, which you can't do in XBL). 

The labels still aren't associated in this patch because the "label" property isn't hooked up right. With that fixed, it works (without hacks!). The bindings inherit from general.xml#basetext - the "label" property in that binding looks for the "label" attribute... but our bindings use an attribute called "title" instead. So you'll need to make the "label" property in setting-base look at the right attribute.

Also, settings can have additional description text... AFAIK, the accessibility code doesn't look for this in any interface, but does look for it in the "tooltiptext" attribute, so just mirror the text from the "desc" attribute.

I assume Marco only tested the automatic updates radio controls - the radio controls for inline prefs are described as having "undefined grouping".

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 597358 [details] [diff] [review]
patch v2

Thanks for the explanation, Blaire! Changing to an f+ under these circumstances so that we can move forward. I did, by the way, see those "undefined grouping" radio buttons, too.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: wip

I'm starting to get somewhere, AFAICT the checkboxes and textboxes work as they should. The radiogroup and menulist prefs have stray accessible objects that need to be got rid of but mostly they work.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Geoff: Any progress here?

Comment 9

[Geoff Lankow (:darktrojan)]

None at all, unfortunately.

Comment 10

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 11

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381