496097 - Handlers dialog list items have no accessible names.

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Marco Zehe (:MarcoZ)]

The dialog that ChatZilla, among others, uses to display its "Launch an application" dialog has no accessible names for the individual list items supplied. Probably similar to bug 410461, which means a trivial one-line fix hopefully.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

I think this ought to work, but I can't test it, being on Mac.

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 381282 [details] [diff] [review]
Patch

That should do the trick. Note that you need an actual toolkit peer's review and possibly SR (for SeaMonkey/Thunderbird) on this.

Comment 3

[:Gijs (he/him)]

Attachment: Patch: s/description/label/

Better patch. And yes, I know I need actual review - just wanted to make sure the a11y is right to begin with :-)

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 381284 [details] [diff] [review]
Patch: s/description/label/

Yes, this is much better, thanks!

Comment 5

[:Gijs (he/him)]

Comment on attachment 381284 [details] [diff] [review]
Patch: s/description/label/

r+sr? From Neil? :-)

Comment 6

[[e-mail]]

Marco, I can't find the code (both in accessibility and toolkit) that actually gets the appropriate value from the richlistitem, can you link me to it?

Comment 7

[Marco Zehe (:MarcoZ)]

Neil, this is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#530

Comment 8

[[e-mail]]

The code that I see there only deals with label elements.
What does that have to do with the label attribute that the patch sets?

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 381284 [details] [diff] [review]
Patch: s/description/label/

Neil is right, this doesn't do what we claim it does. One other usage of the label property for richlistitems is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/apppicker/content/appPicker.js#127
And I'm sure there's also one for the AwesomeBar (which is what this was originally introduced for) concatenating the different information.

Gijs, if you can, please get a new patch up that concatenates the useful info and puts it in the label attribute. I think you can use your patch as a starting point.

The other part about the xul:description changing to xul:label is unaffected by this and is still correct.

Comment 10

[:Gijs (he/him)]

(In reply to comment #9)
> Gijs, if you can, please get a new patch up that concatenates the useful info
> and puts it in the label attribute.

You mean a label element, from what I can tell from comment #8?

Comment 11

[Marco Zehe (:MarcoZ)]

No, I mean the label property on a richlistitem element. See the patch on bug 410461 for an example of that use.

Comment 12

[:Gijs (he/him)]

Attachment: Better patch

So, like this?

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 383488 [details] [diff] [review]
Better patch

This is in principle the right approach and the correct use of the label property.

But I wonder if we can do more here, since there is additional info added that may also be visible on the screen. For example, we could think of alternative texts for the various icons conveying information and append those to the label property string, esp in the populateList method. The label property gives us great flexibility here. :)

Comment 14

[:Gijs (he/him)]

(In reply to comment #13)
> (From update of attachment 383488 [details] [diff] [review])
> This is in principle the right approach and the correct use of the label
> property.
> 
> But I wonder if we can do more here, since there is additional info added that
> may also be visible on the screen. For example, we could think of alternative
> texts for the various icons conveying information and append those to the label
> property string, esp in the populateList method. The label property gives us
> great flexibility here. :)

What exactly do you mean here? I think there are only application icons in the dialog, which don't really convey any info except for the name, which would be in the other field already...

Comment 15

[Marco Zehe (:MarcoZ)]

OK! I looked at the method and thought they conveyed whether this is a locally installed app, a link or whatever and thought if the name doesn't give that info, we should also include that other, otherwise purely visual, info. But if this is just another way of giving the same info, forget about it. ;-)

Comment 16

[:Gijs (he/him)]

(In reply to comment #15)
> OK! I looked at the method and thought they conveyed whether this is a locally
> installed app, a link or whatever and thought if the name doesn't give that
> info, we should also include that other, otherwise purely visual, info. But if
> this is just another way of giving the same info, forget about it. ;-)

Ah, no, it can use either the favicon of the webpage if it's a webbased handler, or the app icon for an application (I should note it doesn't seem to work for me, for iTunes for example, I just don't get an icon at all). Either way, it will just be an icon connected to the app, and won't tell you if it's a particular kind of app. So if I had a webpage to handle iTunes store links, and used the iTunes icon as a favicon, the icon would not help the user distinguish my web app from their actual iTunes application on their machine. This might be considered a bug, I suppose, but that's definitely far beyond the scope of this bug ;-)

Comment 17

[Marco Zehe (:MarcoZ)]

All right, then I'd say ask for review from an actual module peer. :)

Comment 18

[[e-mail]]

Comment on attachment 383488 [details] [diff] [review]
Better patch

Sorry, I don't see why you're trying to set the .label property on the richlistitems. Firstly, .label is supposed to be readonly. Secondly, it should already construct the label by reading the value of the label elements (anonymous if possible)... although I can see that it doesn't actually check all anonymous nodes, but we would have to tweak the binding to fix that.

(I don't have an objection to the <label for> change.)

Comment 19

[Marco Zehe (:MarcoZ)]

Neil, the label property was specifically introduced in bug 407359 to deal with Firefox's awesomebar popup, which is also made up of richlist items, and which contains both text and graphical information. For example, the label property gathers the title, URL, but also gathers alternative text to whether the result is a history item, bookmark, tag or whatever. These are visually represented as graphics, not text, so the label property was especially invented for accessibility to allow each dialog consumer to build their own string of accessibles.

It can be argued that we should provide a default implementation in the binding, which can then be overridden if the info given is not enough or accurate. But that's out of the scope of this bug I think.

The Add-Ons manager, and like I said, the awesome bar implementation, the Download Manager, all have implementations for the .label property. This dialog is missing that, and that's what this bug is about. :)

Comment 20

[[e-mail]]

(In reply to comment #19)
> It can be argued that we should provide a default implementation in the binding
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#530

Or were you thinking of something else?

Comment 21

[Marco Zehe (:MarcoZ)]

OK, but the populateList method doesn't create label children, which is what that impl explicitly searches for. It just sets a few attributes on the richlistitem.

Fact is that the items are not spoken by NVDA. The accessible names of the richlist items are *empty*.

Comment 22

[[e-mail]]

(In reply to comment #19)
> the binding [...] can then be overridden if the info given is not enough
> or accurate. But that's out of the scope of this bug I think.
I think it's quite within the scope of this bug.

The binding in question is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/handling/content/handler.xml#44

Since it doesn't put its labels into its first box, the default implementation doesn't find them.

Comment 23

[:Gijs (he/him)]

Attachment: Patch modifying richlistbox [untested]

So, we can make richlistbox care about the other labels, of course... Surkov, do you think that's a workable approach (given that you wrote this originally?).

Comment 24

[alexander :surkov (:asurkov)]

Approach looks correct, I have nits only

var anonCount = 0;

it's not "anon" because it might be explicit, and I would prefer count -> index.

Comment 25

[alexander :surkov (:asurkov)]

Gijs showed me an example http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/content/updates.xml#186 which will be broken by this patch. I think we shouldn't search common approach to handle all richlistitem. I think every we should follow the rule
1. get the name for richlistitem as it practiced for XUL including @label attribute
2. get the name from subtree
3. trick richlistitem binding may have own implementation of @label attribute or implement nsIXBLAccessible interface to define a name.

Comment 26

[:Gijs (he/him)]

Comment on attachment 383488 [details] [diff] [review]
Better patch

Based on that, I think this patch would be the better approach...

Comment 27

[[e-mail]]

Attachment: handler.xml change, version 1

Comment 28

[[e-mail]]

Attachment: handler.xml change, version 2

Comment 29

[[e-mail]]

Marco, surkov, are either of these two patches of any help? (I know that they change the result of the "label" property in DOM Inspector's JS Object view, but I couldn't find any change in accessible properties.)

Comment 30

[Marco Zehe (:MarcoZ)]

Comment on attachment 384077 [details] [diff] [review]
handler.xml change, version 2

I like this second approach better since Ihave a feeling it is more easily extensible in case someone adds something.

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 384077 [details] [diff] [review]
handler.xml change, version 2

r=me if we don't want to reconsider correctness of existing approach.

Technically I'm not sure label property implementation that we have and proposed by this patch goes with label attribute meaning from XUL point of view.

Comment 32

[[e-mail]]

Comment on attachment 384077 [details] [diff] [review]
handler.xml change, version 2

Pushed changeset 20774f09b6b0 to mozilla-central.

Comment 33

[[e-mail]]

Comment on attachment 383488 [details] [diff] [review]
Better patch

r=me on the dialog.xul change which I assume is still necessary.

Comment 34

[:Gijs (he/him)]

(In reply to comment #33)
> (From update of attachment 383488 [details] [diff] [review])
> r=me on the dialog.xul change which I assume is still necessary.

Pushed as 30206:7ec37fbf5e61 . I believe this is FIXED now?

Comment 35

[Marco Zehe (:MarcoZ)]

Verified fixed in ChatZilla 0.9.85-rdmsoft [XULRunner 1.9.2a1pre/20090715032214]

Comment 36

[Marco Zehe (:MarcoZ)]

Comment on attachment 384077 [details] [diff] [review]
handler.xml change, version 2

This two-part fix is low-to-no-risk and makes app pickers across varying Mozilla apps more accessible to screen readers.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 383488 [details] [diff] [review]
Better patch

a1912=beltzner, next time, please submit a single rollup patch to make sure that everything gets in.

Comment 38

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-1.9.1 in changesets
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b18b609b1310
and
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ef74b9944d5

Comment 39

[juan becerra [:juanb]]

Marco, could you help us verify this for 3.5.2?