245370 - Autocomplete dropdowns not exposing focus or selection events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The autocomplete dropdowns (e.g. the address bar or the Location text field in
Prefs:Navigator) allow keyboard access. However, as the user arrows through the
choices, accessibility events are not being fired.

The autocomplete.xml widget in mozilla/xpfe and mozilla/toolkit use XUL trees
for the dropdown list.

Comment 1

[Aaron Leventhal]

Attachment: Fire DOMMenuItemActive events on tree view and ValueChange events for textbox . Expose nsIDOMXULMultiSelectControlElement::currentIndex currently for autocomplete tree.

Comment 2

[[e-mail]]

Would this code be simpler if we didn't try to pretend the tree was a menulist 
and instead simply sent normal tree selection events, i.e. instead of setting
this.textbox.view.selectedIndex we could use this.textbox.view.selection.select
and instead of getting we could use this.textbox.view.selection.currentIndex?

Comment 3

[Aaron Leventhal]

Neil, where are we pretending it's a menulist? The currentSelection is something
from nsIDOMXULMultiSelectElement which is something implemented by tree.xml. The
"DOMMenuItemActive" could be changed to "select", but we've been using it to
mean the focus has moved inside a list, menu or tree. The focus and selection
inside a multi select list are different, but I'm not sure we get that correct
yet with respect to accessibility API's.

I did it this way because of the following comments:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/autocomplete/resources/content/autocomplete.xml#1222
1222           // implementing these results in a crash
1223           // get selection() {},
1224           // set selection(aVal) { },

Anyway, I don't think the code would be much simpler. What would actually be
simpler is if we used the same autocomplete.xml as mozilla/toolkit which
implements it's own set of nsIAutoComplete* interfaces. But, the current patch
actually decreases the amount of code slightly overall, by simplifying how we
handle the autocomplete and regular trees in nsRootAccessible::HandleEvent()

Comment 4

[[e-mail]]

(In reply to comment #3)
>1222           // implementing these results in a crash
>1223           // get selection() {},
>1224           // set selection(aVal) { },
This is correct, that particular implementation will cause Mozilla to crash.

Comment 5

[[e-mail]]

Comment on attachment 149861 [details] [diff] [review]
Fire DOMMenuItemActive events on tree view and ValueChange events for textbox . Expose nsIDOMXULMultiSelectControlElement::currentIndex currently for autocomplete tree.

>+  if (localName.EqualsLiteral("tree")) {
>+    nsCOMPtr<nsIDOMXULMultiSelectControlElement> multiSelect =
>+      do_QueryInterface(targetNode);
I'm not too keen on this, it doesn't look as clean as the code you're
replacing, is the change only because the currentIndex isn't accurate in the
autocomplete widget? In which case I'd prefer to make the currentIndex accurate
in the autocomplete widget...

>   else if (eventType.EqualsIgnoreCase("select")) {
>-    if (treeBox && treeIndex >= 0) { // it's a XUL <tree>
>+    if (treeItemAccessible) { // it's a XUL <tree>
I'm not following this... if this is looking for a select event on a tree why
is the other code generating DOMMenuItemActive events?

Comment 6

[Aaron Leventhal]

(In reply to comment #5)
> (From update of attachment 149861 [details] [diff] [review])
> >+  if (localName.EqualsLiteral("tree")) {
> >+    nsCOMPtr<nsIDOMXULMultiSelectControlElement> multiSelect =
> >+      do_QueryInterface(targetNode);
> I'm not too keen on this, it doesn't look as clean as the code you're
> replacing
The code I'm replacing already does a localName.EqualsLiteral("tree") -- it's
just hidden away in nsXULTreeAccessible::GetTreeBoxObject. So this code does the
same thing really, but with one less QI -- and uses a QI that works for both
kids of trees.
> , is the change only because the currentIndex isn't accurate in the
> autocomplete widget? In which case I'd prefer to make the currentIndex 
> accurate in the autocomplete widget...
I don't understand, I am still using currentIndex -- but I can't use it via
nsITreeSelection without implementing the selection object on autcomplete.
Sounds like the last person who tried came up against crashes so I'm not sure
what needs to get done for that.

> >   else if (eventType.EqualsIgnoreCase("select")) {
> >-    if (treeBox && treeIndex >= 0) { // it's a XUL <tree>
> >+    if (treeItemAccessible) { // it's a XUL <tree>
> I'm not following this... if this is looking for a select event on a tree why
> is the other code generating DOMMenuItemActive events?> 
That's true, we need to check for DOMMenuItemActive or fire a "select". I think
we should use "select" for both kinds of trees.

Comment 7

[Aaron Leventhal]

Attachment: DOMMenuItemActive -> select

Comment 8

[Aaron Leventhal]

Comment on attachment 149975 [details] [diff] [review]
DOMMenuItemActive -> select

Neil, I didn't change the code that looks for the "tree" tag, my explanation is
in a comment -- mainly that we already were checking the tag. Here we're using
less code and covering more situations.

Comment 9

[Aaron Leventhal]

I want to get this in so we can finish work on bug 201922.

Comment 10

[[e-mail]]

Comment on attachment 149975 [details] [diff] [review]
DOMMenuItemActive -> select

>                 onset="this.ignoreInputEvent = true;
>                        this.mInputElt.value = val;
>                        this.ignoreInputEvent = false;
>+                       var event = document.createEvent('Events');
>+                       event.initEvent('ValueChange', true, true);
>+                       this.mInputElt.dispatchEvent(event);
>                        return val;"
This onset is getting a little big... turning it into a <setter> might be
neater...

>+            this.mTree.treeBody.parentNode.dispatchEvent(event);
I think this.mTree.element.dispatchEvent(event); should work here.

Comment 11

[Aaron Leventhal]

Comment on attachment 149975 [details] [diff] [review]
DOMMenuItemActive -> select

Ok, changes made and tested to verify they work. Will post new patch if
requested to do so.

Comment 12

[Alec Flett]

Comment on attachment 149975 [details] [diff] [review]
DOMMenuItemActive -> select

I'm with neil, make that big onset a setter, and sr=alecf

Comment 13

[Aaron Leventhal]

Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.90; previous revision: 1.89
done
Checking in xpfe/components/autocomplete/resources/content/autocomplete.xml;
/cvsroot/mozilla/xpfe/components/autocomplete/resources/content/autocomplete.xml,v
 <--  autocomplete.xml
new revision: 1.110; previous revision: 1.109
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.25; previous revision: 1.24
done

Comment 14

[Aaron Leventhal]

Relanding relevant parts of patch following aviary branch landing

Comment 15

[Firefox Bug Husbandry Bot]

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