290354 - Multiple selection for accessible DHTML

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Aaron Leventhal]

The following example has multiple selection:
www.mozilla.org/access/samples/dhtml-spreadsheet.html

If you hit ctrl+arrow and then ctrl+space it toggles selection on cells.

At the moment, no further testcases needed than what we have.

Comment 1

[Aaron Leventhal]

Attachment: Work in progress, need to be able to support nsIAccessibleSelectable on anything with STATE_MULTISELECT

Comment 2

[Aaron Leventhal]

Attachment: Multipart fix, see comments

1) Use AttributeChanged() and newly fired DOMItemSelected/DOMItemUnselected to
determine MSAA events to fire. We fire SELECTION_WITHIN on the multiselect
container and SELECTION_ADD/SELECTION_REMOVE on the individual item. [Louie,
what should be done for ATK]
2) Support nsIAccessibleSelectable for any DHTML multiselect container [Louie,
should nsIAccessibleSelectable be changed to deal with multiselects that have
deep subtrees? Many of the methods refer to child numbers ]
3) Fix single selects so that ctrl+arrow does not operate on them - focus
should always mirror selection for single selects
4) Fix onchange event so it always fires when ctrl+space is used to toggle
selection on an item

Comment 3

[Aaron Leventhal]

Comment on attachment 186848 [details] [diff] [review]
Multipart fix, see comments

No response from Louie in 3 weeks. Timeless?

Comment 4

[timeless]

Comment on attachment 186848 [details] [diff] [review]
Multipart fix, see comments

>+NS_IMETHODIMP nsAccessible::GetSelectedChildren(nsIArray **aSelectedAccessibles)

if !length you don't set aSelectedAccessibles
but you return NS_OK

>Index: accessible/src/base/nsDocAccessible.cpp
>+if (aAttribute == nsAccessibilityAtoms::selected) {

you lost whitespace :o

>+    rv = target->AddEventListener(NS_LITERAL_STRING("select"), NS_STATIC_CAST(nsIDOMXULListener*, this),
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "failed to register listener");

things like that should be able to fail for oom
which you can't assert won't happen

please file a separate bug to fix all of those

>+nsHTMLOptionElement::DispatchDOMEvent(const nsAString& aName)
>+    domDoc->CreateEvent(NS_LITERAL_STRING("Events"),
>+                        getter_AddRefs(selectEvent));

you're in a void function, so you don't indicate that createevent failed to
your caller
otoh i suppose there's probably nothing you can do anyway

>+      selectEvent->InitEvent(aName, PR_TRUE, PR_TRUE);

can't fail?

>+      nsCOMPtr<nsIDOMEventTarget> target =
>+        do_QueryInterface(NS_STATIC_CAST(nsIDOMNode*, this));

there's no way to skip the QI?

>+  DispatchDOMEvent(aValue ? NS_LITERAL_STRING("DOMItemSelected") : NS_LITERAL_STRING("DOMItemUnselected"));

i have a feeling this will cause some poor compiler to die unhappily

Comment 5

[Aaron Leventhal]

Attachment: Fix Timeless' nits

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 189119 [details] [diff] [review]
Fix Timeless' nits

>+nsresult nsAccessible::QueryInterface(REFNSIID aIID, void** aInstancePtr)
>+{

Here, it might be cleaner / less error-prone to use NS_INTERFACE_MAP_* and just
insert your custom handling for nsIAccessibleSelectable before
NS_INTERFACE_MAP_END_INHERITING.  Up to you.

>+already_AddRefed<nsIAccessible>
>+nsAccessible::GetMultiSelectFor(nsIDOMNode *aNode)
>+{
...
>+  nsIAccessible *returnAccessible = accessible;
>+  NS_IF_ADDREF(returnAccessible);
>+  return returnAccessible;

Here, use nsCOMPtr swap() to avoid the extra addref and release.

>+already_AddRefed<nsIAccessible>
>+nsAccessible::GetNextWithState(nsIAccessible *aStart, PRUint32 matchState)
>+{
...
>+  nsIAccessible *returnAccessible = current;
>+
>+  return current;

I think the addref logic in the return here will lead to a double-free, since
you return already_AddRefed but the reference will be dropped when |current|
goes out of scope.  I'd say you want to use swap() and return returnAccessible,
as I mentioned above.

>+// return the nth selected child's nsIAccessible object
>+NS_IMETHODIMP nsAccessible::RefSelection(PRInt32 aIndex, nsIAccessible **aSelected)

You mean the nth selected descendent, right?

Also, maybe you should factor out the code that gets the nth child?  I see it
repeated in a few places.

>--- accessible/src/base/nsDocAccessible.cpp	1 Jul 2005 18:06:40 -0000	1.65
>+++ accessible/src/base/nsDocAccessible.cpp	12 Jul 2005 23:57:01 -0000
>@@ -753,7 +753,31 @@ nsDocAccessible::AttributeChanged(nsIDoc
>   }
> 
>   PRUint32 eventType = 0;
>-  if (aNameSpaceID == kNameSpaceID_StatesWAI_Unofficial) {
>+if (aAttribute == nsAccessibilityAtoms::selected) {

Indentation looks off here.

>--- accessible/src/base/nsRootAccessible.cpp	21 Jun 2005 20:07:26 -0000	1.123
>+++ accessible/src/base/nsRootAccessible.cpp	12 Jul 2005 23:57:01 -0000
>+    // capture Form change events 
>+    rv = target->AddEventListener(NS_LITERAL_STRING("DOMItemSelected"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // capture Form change events 
>+    rv = target->AddEventListener(NS_LITERAL_STRING("DOMItemUnselected"), NS_STATIC_CAST(nsIDOMFormListener*, this), PR_TRUE);
>+    NS_ENSURE_SUCCESS(rv, rv);

Does the "Capture Form change events" comment apply to these new lines?  Why
are you casting to nsIDOMXULListener for DOMItemSelected and nsIDOMFormListener
for DOMItemUnselected?

>@@ -307,6 +317,8 @@ nsresult nsRootAccessible::RemoveEventLi
>   if (target) { 
>     target->RemoveEventListener(NS_LITERAL_STRING("focus"), NS_STATIC_CAST(nsIDOMFocusListener*, this), PR_TRUE);
>     target->RemoveEventListener(NS_LITERAL_STRING("select"), NS_STATIC_CAST(nsIDOMFormListener*, this), PR_TRUE);
>+    target->RemoveEventListener(NS_LITERAL_STRING("DOMItemSelected"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);
>+    target->RemoveEventListener(NS_LITERAL_STRING("DOMItemUnselected"), NS_STATIC_CAST(nsIDOMXULListener*, this), PR_TRUE);

These casts should match what you did in AddEventListener.

Looks ok otherwise, sr=me with those fixes.

Comment 7

[Aaron Leventhal]

Comment on attachment 189119 [details] [diff] [review]
Fix Timeless' nits

I will address Bryner's comments when I check in.

Comment 8

[Aaron Leventhal]

Attachment: Final patch that is getting checked in

Comment 9

[Aaron Leventhal]

Checking in accessible/src/base/nsAccessibilityAtomList.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v  <-- 
nsAccessibilityAtomList.h
new revision: 1.25; previous revision: 1.24
done
Checking in accessible/src/base/nsAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsAccessible.h,v  <--  nsAccessible.h
new revision: 1.68; previous revision: 1.67
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.158; previous revision: 1.157
done
Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.124; previous revision: 1.123
done
Checking in accessible/src/html/nsHTMLSelectAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLSelectAccessible.h,v  <-- 
nsHTMLSelectAccessible.h
new revision: 1.27; previous revision: 1.26
done
Checking in accessible/src/xul/nsXULSelectAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULSelectAccessible.h,v  <-- 
nsXULSelectAccessible.h
new revision: 1.21; previous revision: 1.20
done
Checking in layout/forms/nsListControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsListControlFrame.cpp,v  <--  nsListControlFrame.cpp
new revision: 1.368; previous revision: 1.367
done
Checking in content/html/content/src/nsHTMLOptionElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLOptionElement.cpp,v  <-- 
nsHTMLOptionElement.cpp
new revision: 1.135; previous revision: 1.134
done

Comment 10

[Boris Zbarsky [:bzbarsky]]

Aaron, was the performance impact of this change evaluated on pages that do lots
of DOM manipulation of selects (eg the bugzilla query page)?  That tends to do a
lot of intermediate option selecting/unselecting as the DOM is mutated...

Comment 11

[Boris Zbarsky [:bzbarsky]]

I filed bug 300818 on the tinderbox Tp regression from this bug...

Comment 12

[Firefox Bug Husbandry Bot]

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