406355 - New autocomplete has a11y issues

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

After fix for bug 399664, which introduces a new widget called Richlistbox, accessibility has regressed severely compared to builds of November 29 and earlier. On Windows, the widget is rendered as a context menu with no real focused menu item.

Suggestion: Render the Richlistbox widget as a ListView component on Windows, with each line being represented as one ListView column. On Linux, this should be represented as a Tree table. This way, all information is available in both speech and braille without the user having to switch between lines.

Other suggestions welcome!

Comment 1

[Aaron Leventhal]

Marco, richlistbox already existed, although not as part of a dropdown. It's used in the download manager, for example. We already expose it via MSAA and ATK/AT-SPI, and it's accessible.

I have tried the new autocomplete list with JAWS, and it worked for me, although it said "listbox" every time I focused a new item.

What actual regression are you seeing?

Comment 2

[Aaron Leventhal]

Since it's a regression in a particular chrome component where the UI changed I'm putting in that category.

Comment 3

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> I have tried the new autocomplete list with JAWS, and it worked for me,
> although it said "listbox" every time I focused a new item.

And on my system, it only speaks the characters I typed as the name for each list item. Braille display gives a bit more contextual information, but with speech, you're completely hosed. The older-style auto-complete was a nice listview that had both the URL and the title of the selected item displayed. This new one only gives either the title or the URL on the braille display, depending on what matched, and in speech, it will only speak the partial word I typed.

Comment 4

[Scott Haeger]

The AT-SPI hierarchy for this structure looks fine.  The list items do not support the text interface but this is not a problem because the information can be obtained via the four children objects.  

An Orca bug will be filed to address this change.

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> Marco, richlistbox already existed, although not as part of a dropdown. It's
> used in the download manager, for example. We already expose it via MSAA and
> ATK/AT-SPI, and it's accessible.

I compared the DM and the Location AutoComplete, and here's what I found:
1. In DM, there is a nameless list which contains lists for as many files as it has. The names of those lists (AccName) are a combination of name, size and domain this came from. The child nodes are the name, size text, and Open and Information buttons.

2. In Location AutoComplete, there is also a nameless list, with x children that are also lists. However--and this is the difference--, the AccName of those lists only consists of the portion of the URL *I* typed in. So for my favourite newsticker page, if I only typed "ne", all these 25 or so lists will have names of "ne". The children of those lists then are text elements that contain the actual items that are displayed onscreen.

So the difference is: While in DM the lists get a meaningful name, in Location AutoComplete, the name only consists of the text the user types, which is what gets spoken by screen readers.

Can we have that changed?

Comment 6

[Aaron Leventhal]

Seth, this is a critical regression for us. Can you look at it soon?
Here is where the accessible name for a richlistitem is created:
http://mxr.mozilla.org/seamonkey/source/accessible/src/xul/nsXULSelectAccessible.cpp#226
Right now it just contains the text you have typed.

Marco, another problem is that when the autocomplete opens there is now a flood of mutation events, which cause MSAA show/hide/reorder or ATK children-changed events, and will thus likely cause perf issues.

Comment 7

[(not reading, please use [e-mail] instead)]

> Can we have that changed?

first, let me apologize for making the url bar less accessible without any warning.

second, from what marco describes in comment #5, it sounds like we are only finding the emphasized label, and not the content in the description.

if you were to look at an item in dom inspector you would see something like:

<xul:description>this <xul:label>is a</xul:label>test</xul:description>

Marco / Aaron, can you see what happens when you click on the drop down icon on the right hand side of the url bar?  This is supposed to show you the 50 most recently typed urls, but I bet AccName will be empty for all of them, because in DOM inspector, you'd see:

<xul:description>this is a test<xul:label></xul:label></xul:description>

it seems like what I want it to some how indicate, in my binding (that extends the richlistitem binding) what the "label" is for the complex item is.

 on each of these complex item, the "url" attribute is set to the complete url text and the "title" attribute is set to the complete title (and the "type" attribute that indicate if the match is a bookmark or a tag.)

results, when read aloud by a screen reader, would be something like:

"Mozilla Cross-Reference http://lxr.mozilla.org tag"
"Google http://www.google.com bookmark"
"Yahoo! http://www.yahoo.com"

Can I just set the label on the richlistitem to this concatenated value, and then, since listcell inherits the label, it will inherit it?  (see http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/listbox.xml#880)

note, I cant just concatenate the result "type", as "tag" and "bookmark" need to be localized as they will be read aloud by screen readers.  (right now, it is just used to set the class, as in "ac-result-type-" + type.)
 
note, as a temporary work around for users who are blocked until we fix this, can you can disable this feature by setting browser.urlbar.richResults to false.

Comment 8

[(not reading, please use [e-mail] instead)]

I'm using microsoft narrator on windows xp.  zoinks! I really broke the url bar.  my apologies again!

hopefully, that will be good enough to verify my idea and then test a complete fix.

Comment 9

[Aaron Leventhal]

Hi Seth, you can also set a breakpoint where I pointed in comment 6. I suggest looking at what it's doing correctly for the download manager vs. here. It's been a while so I don't remember myself.

BTW Marco's first day as a MoCo employee was yesterday. He's available to help test any patches for a11y results.

Comment 10

[(not reading, please use [e-mail] instead)]

> Hi Seth, you can also set a breakpoint where I pointed in comment 6.

good idea.  that showed me that we're calling into nsAccessible::GetXULName

I wonder if I can take my richlistitem and implement nsIDOMXULSelectControlItemElement and implement a getter for label do exactly what I want?

working on that now...

Comment 11

[(not reading, please use [e-mail] instead)]

> working on that now...

this seems to work, and we do this idea elsewhere, like http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.xml#137

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: patch

note, there are l10n issues with the type ("tag" or "bookmark")

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: patch that is l10n friendly

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 291392 [details] [diff] [review]
patch that is l10n friendly

aaron / marco, do you have cyles to review and test?

Comment 15

[(not reading, please use [e-mail] instead)]

note, I've tested with the Microsoft Narrator screen reader.

for the three types of results, It reads aloud what I expect:

tag: "Mozilla Cross-Reference http://lxr.mozilla.org tag"
bookmark: "Google http://www.google.com bookmark"
favicon "Yahoo! http://www.yahoo.com"

(note that for type == favicon, it doesn't say anything.)

Comment 16

[Marco Zehe (:MarcoZ)]

Verified that this patch is indeed good and works great with JAWS as well. I also get the different types announced as advertised.

Thanks Seth for fixing this so quickly!

Comment 17

[Aaron Leventhal]

Comment on attachment 291392 [details] [diff] [review]
patch that is l10n friendly

Seth, I like the fact that the title now goes before the URL -- it's more TTS friendly.

Why does it need to now support nsIAccessibleProvider? I don't see you supplying the accessibleType. I don't think you need that part.

I'm going to let Gavin do the actual code review. I'm fine with the approach of building a label for nsIDOMXULSelectControlItemElement. I think it's the right way to go.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 291392 [details] [diff] [review]
patch that is l10n friendly

>Index: browser/base/content/urlbarBindings.xml

>+      <method name="createResultLabel">

>Index: toolkit/content/widgets/autocomplete.xml

>+      <property name="label" readonly="true">

>+            return this.parentNode.parentNode.createResultLabel(this.getAttribute("url"), 
>+                                                                this.getAttribute("title"), 
>+                                                                this.getAttribute("type"));

Doesn't this make the toolkit autocomplete binding depend on a method in the browser-specific url bar binding?

It also seems like Aaron's right that implementing nsIAccessibleProvider shouldn't be necessary.

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 20

[(not reading, please use [e-mail] instead)]

aaron / gavin, thanks for the review comments.

1)

> Why does it need to now support nsIAccessibleProvider? I don't see you
> supplying the accessibleType. I don't think you need that part.

this was copy-and-paste coding on my part.  fixed, thanks.

2)  

> Doesn't this make the toolkit autocomplete binding depend on a method in the
> browser-specific url bar binding?

as discussed over irc, what I needed was an implementation of createResultLabel() in autocomplete.xml, like we do for onPopupClick() that I can then override in urlbarBindings.xml

in the base implementation, we ignore "aType" and don't use it for the label, but in the urlbarBinding implemenation, I'll convert the "star" or "tag" icon into localized text and append it to the end of the label.

seeking re-review from gavin.

Comment 21

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

a different, cleaner way (per gavin over irc)

Comment 22

[(not reading, please use [e-mail] instead)]

Attachment: updated patch per further discussion with gavin over irc

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: updated patch per further discussion with gavin over irc (white space fixed)

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 291593 [details] [diff] [review]
updated patch per further discussion with gavin over irc (white space fixed)

>Index: toolkit/content/widgets/autocomplete.xml

>+      <property name="label" readonly="true">

>+            if (panel.createResultLabel)
>+              return panel.createResultLabel(title, url, this.getAttribute("type"));

Add a comment explaining that this is here to allow consumers that have extended popups to override the label values for the richlistitems?

>Index: browser/base/content/urlbarBindings.xml

>+      <field name="_bundle">null</field>

>+      <method name="createResultLabel">

>+              if (!this._bundle) {
>+                this._bundle = Cc["@mozilla.org/intl/stringbundle;1"].
>+                                 getService(Ci.nsIStringBundleService).
>+                                 createBundle("chrome://browser/locale/places/places.properties");

Since fields are lazily initialized now you can just put this in the field's initial value.

>Index: browser/locales/en-US/chrome/browser/places/places.properties

>+tagResultLabel=Tag
>+bookmarkResultLabel=Bookmark

Add an L10N note to describe where these are used?

If Aaron and Marco are satisfied with the results this gives, that's great. I'm a bit concerned that the simple string concatenation approach might not be optimal (does RTL matter here?), but this is certainly better than what we currently do.

Comment 25

[(not reading, please use [e-mail] instead)]

1)

> Add a comment explaining that this is here to allow consumers that have
> extended popups to override the label values for the richlistitems?

fixed, thanks.

2)

> Since fields are lazily initialized now you can just put this in the field's
> initial value.

fixed, thanks.

3)

> Add an L10N note to describe where these are used?

fixed, thanks.

4)

> If Aaron and Marco are satisfied with the results this gives, that's great. 

Marco seemed satisfied over email, but I'll let him verify.

> I'm a bit concerned that the simple string concatenation approach might not be
> optimal

I agree, and I logged bug about this.  (See bug #406907 about using punctuation, and for an example of where we concat in other parts of the code.)

5)

> does RTL matter here?

I don't think so.  I think we want the screen reader to read "title, url, type" for RTL and LTR, right?

Comment 26

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 27

[(not reading, please use [e-mail] instead)]

fixed.

thanks to gavin, aaron and marco for the help.

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.97; previous revision: 1.96
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.
xml
new revision: 1.48; previous revision: 1.47
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v
  <--  places.properties
new revision: 1.34; previous revision: 1.33
done

Comment 28

[Kadir Topal [:atopal]]

The notes for localizers are unfortunately not much of help, a rewording would be nice.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> The notes for localizers are unfortunately not much of help, a rewording would
> be nice.

Could you file a new bug, CC me and Seth, and perhaps suggest a better comment if one comes to mind? I would be grateful :)

Comment 30

[Kadir Topal [:atopal]]

I created Bug 407221, but as I was unsure about the meaning of the notes I didn't come up with a suggestion

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Thanks! I commented there.

Comment 32

[Gregory J. Rosmaita]

i don't know if i should open a NEW bug, because (a) this one has been closed and (b) the OS Version referenced in this report is Windows Vista, but the autocomplete mechanism in the location bar is still broken on Windows XP as of 2007.12.09 -- one can still not type a new URI or edit an extand URI in the location bar, due to the same VERY agressive auto-complete interface described in Bug 406355, and there still is nothing akin to the autocomplete options property sheet of SeaMonkey in Minefield...

the build i'm currently testing is:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120905 Minefield/3.0b2pre

the OS i'm running is Windows XP Professional SP2

as soon as i begin to type into the location bar, my screen reader no longer announces "context menu, list box list box, X of Y" as it first did when i reported this to dev-accesibility -- it now communicates nothing to my screen reader (JAWS9); if i type more than one character in the location bar, newly input text (what i type) is appended to the end of a randomly 
selected URI -- even when the location bar is blank, typing a URI causes a previously visited URI to be randomly chosen (without notification from my screen reader, JAWS 9.0.515) with the result that the input being typed to the location bar is appended to the randomly chosen URI...

one of the many things that needs to be addressed is an input interface seperate from the location bar, as a user may need/want one input interface to implement an autocomplete mechanism, and another to either suppress autocomplete altogether or to use a tailored version of autocomplete (controlled by an interface such as that implemented by SeaMonkey as suggested in:

http://groups.google.com/group/mozilla.dev.accessibility/browse_thread/thread/f6a57d49019972fe#b22e07ed8130ceda

Comment 33

[Aaron Leventhal]

Gregory, we're dealing with some of the issues you mention in bug 407359. I suggest CC'ing yourself there. Once we mark that FIXED then test the next day's build.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

Marco, any chance you have time to do a more-thorough verification of this fix than I? :-)  If so, could you mark it verified (others, too, whom experienced this, please feel free to chime in).

Thanks!

Comment 35

[Marco Zehe (:MarcoZ)]

Marking verified. The content of the ListItem, which was the objective of this bug, is indeed OK. The issue that Gregory is describing is actually a different part of the AutoComplete complex: The fact that right now, these popups announce themselves as menus to ATs. This is dealt with in bug 407359, as pointed out by Aaron already.

Comment 36

[Tracy Walker [:tracy]]

in-litmus- since we don't have an a11y test suite in Litmus.