146400 - To merge nsIAccessibleSelectable and nsIAccessibleSelection

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Kyle Yuan]

These two interfaces are quite similar -- all they aim at selectable widget.
Merging them together can make the accessibility hierarchy clearer.

functions in nsIAccessibleSelectable:
  nsISupportsArray GetSelectedChildren();

functions in nsIAccessibleSelection:
  readonly attribute long selectionCount;
  void addSelection (in long index);
  void removeSelection (in long index);
  void clearSelection ();
  nsIAccessible refSelection (in long index);
  boolean isChildSelected (in long index);
  boolean selectAllSelection ();

I've implemented nsIAccessibleSelection. Aaron and John, if you think I go the 
right way, I'll put the patch tomorrow.

Comment 1

[Kyle Yuan]

Attachment: patch

1) merged nsIAccessibleSelectable and nsIAccessibleSelection together
2) defined a new class nsAccessibleSelectable to implement
nsIAccessibleSelectable interface
3) nsAccessibleSelectable implemented all the methods for HTML Listbox, HTML
Combobox, XUL Listbox, XUL Combobox, XUL tree has its own implementation.

Jessie has tested this patch.

Comment 2

[Aaron Leventhal]

How does this line compile?
+#include "nsIDOMXULMultSelectCntrlEl.h"

Should it be 
+#include "nsIDOMXULMultiSelectCntrlEl.h"

Comment 3

[Aaron Leventhal]

If you decide that you like the name nsIAccessibleSelection better than
nsIAccessibleSelectable, I would understand. It does not matter much, I will let
you decide.

The naming convention in Mozilla for variable names is always interCaps.
Please change things like xulmultiselect to xulMultiSelect.

I know you did not write this part, that you only copy and pasted, but please
change it for me anyway. We do not want single letter variable names, please use
|index| instead of |i|
       for (PRUint32 i = 0 ; i < length ; i++) {
         nsCOMPtr<nsIDOMNode> tempNode;
         options->Item(i,getter_AddRefs(tempNode));
There are several other places where |i| is used, please change them too.

+  if (xulmultiselect) {
+    xulmultiselect->GetSelectedCount(aSelectionCount);
+    return NS_OK;
+  }
can be 
if (xulMultiSelect)
  return xulMultiSelect->GetSelectedCount(aSelectionCount);

Would it be better to separate the XUL parts into an nsXULAccessibleSelectable
class? I will leave that up to you. If not, please wrap the XUL parts in #ifdef
MOZ_XUL

Your code is beautiful, but there is a lot of duplication. For example:
+NS_IMETHODIMP nsAccessibleSelectable::RemoveSelection(PRInt32 index)
+{
+  nsCOMPtr<nsIDOMHTMLSelectElement> select(do_QueryInterface(mDOMNode));
+  if (select) {
+    nsCOMPtr<nsIDOMHTMLCollection> options;
+    select->GetOptions(getter_AddRefs(options));
+    if (options) {
+      nsCOMPtr<nsIDOMNode> tempNode;
+      options->Item(index, getter_AddRefs(tempNode));
+      nsCOMPtr<nsIDOMHTMLOptionElement> tempOption(do_QueryInterface(tempNode));
+      if (tempOption)
+        tempOption->SetSelected(PR_FALSE);
+    }
+    return NS_OK;
+  }
and
+NS_IMETHODIMP nsAccessibleSelectable::AddSelection(PRInt32 index)
+{
+  nsCOMPtr<nsIDOMHTMLSelectElement> select(do_QueryInterface(mDOMNode));
+  if (select) {
+    nsCOMPtr<nsIDOMHTMLCollection> options;
+    select->GetOptions(getter_AddRefs(options));
+    if (options) {
+      nsCOMPtr<nsIDOMNode> tempNode;
+      options->Item(index, getter_AddRefs(tempNode));
+      nsCOMPtr<nsIDOMHTMLOptionElement> tempOption(do_QueryInterface(tempNode));
+      if (tempOption)
+        tempOption->SetSelected(PR_TRUE);
+    }
+    return NS_OK;
+  }
Please use helper methods to reduce some of the code overhead.
For example:
void ChangeSelection(PRInt32 aIndex, PRBool aAddItem)
{
  // If aAddItem is FALSE, remove item instead of adding
...
}

Same thing goes with nsXULTreeAccessible. All you would need is 
if (isSelected != aAddItem)
  ToggleSelect(index);

Perhaps to avoid the helper methods we should change the nsIAccessibleSel*
interface to use ChangeSelection(index, bool aAddItem).

I have not looked at your other methods as closely, but I think that
GetSelectedChildren(), GetSelectionCount() and possiblty RefSelection() and
SelectAllSelection() could share 1 helper method with multiple arguments to
reduce code footprint.

Comment 4

[Kyle Yuan]

aaron, thanks for your careful review. I know it is really a hard work to 
review the two big patches, but your comments are very helpful as usual. I'll 
revise this raw patch soon.

Comment 5

[Kyle Yuan]

About the hierarchy issue:

Now, the class hierarchy likes this:

                              nsAccessible
                                   |
                       nsAccessibleSelectable
                                   |
                 +----------------------------------+--------------+
            nsListboxAcc.                     nsComboboxAcc. nsXULTreeAcc.
                 |                                  |
        +----------------+                 +------------------+
nsHTMLListboxAcc. nsXULListboxAcc. nsHTMLComboboxAcc. nsXULComboboxAcc.

because I need mDOMNode member variable in nsAccessibleSelectable's 
implementation.

If I impl. nsHTMLAccessibleSelectable and nsXULAccessibleSelectable, the 
hierarchy should be this:

                              nsAccessible
                                   |
                 +----------------------------------+
            nsListboxAcc.                     nsComboboxAcc.
                 |                                  |
        +----------------+                 +------------------+
nsHTMLListboxAcc. nsXULListboxAcc. nsHTMLComboboxAcc. nsXULComboboxAcc.
        +---------------------|------------+                  + 
              |               |                               |
nsHTMLAccessibleSelectable    +-------------------------------+
                                               |
                                   nsXULAccessibleSelectable

then, I can not use any data members in nsAccessible.

Comment 6

[Kyle Yuan]

Attachment: patch v2

changes made:
1) eliminated single letter vars (|i|), non-interCaps var (|xulmultiselect|);
2) put XUL specific code into #ifdef MOZ_XUL/#endif block;
3) merged duplicate code into helper functions;

need r=

Comment 7

[Kyle Yuan]

Attachment: change all parameters from |interCap| to |aInterCap|

Comment 8

[Aaron Leventhal]

Kyle,

The "Cal" in CalSelectionCount() is not a good abbreviation for "Calculate". You
can use CalcSelectionCount()

I don't think GenerateAccessible is a good description of what these 2
overloaded methods do.
How about something like: GetAccessibleIfSelected() and AddAccessibleIfSelected()

Methods that do nothing for XUL should have a comment that says they are not
needed for XUL.

It's too bad that we can't find a good way to separate this into a XUL, HTML and
possibly base class. I think mixing the XUL and HTML stuff together makes it
less clean. Could multiple inheritence be used to solve this problem?

Either way, I'll give you r= soon.

Comment 9

[Kyle Yuan]

As Scott said:
1) The nested class is okay for portability.
2) The multiple inheritence is also portable, but it's not the best way to 
solve problem. The virtual-multiple inheritence is better, but it's not 
portable.

After skimming layout\html\forms\resources\content\select.xml, I think it is 
quite similar with XUL <tree>. So I still think that separating 
nsAccessibleSelectable into HTML part and XUL part is a good idea, and it is 
easy to implement. I'll put the new patch tonight.

Comment 10

[Kyle Yuan]

Attachment: split nsAccessibleSelectable into nsHTMLSelectableAccessible and nsXULSelectableAccessible

Patch looks more clear, but much bigger, because some code was moved.

Since the behaviors in HTML listbox/combobox and XUL listbox/combobox are
different, so I eliminated nsListBoxAccessible and nsConmoboxBoxAccessible. I
also down-graded the inheritance of nsIDOMXULListener to
nsHTMLComboboxAccessible, XUL combobox has its own method to determine whether
the dropdown window is opened.

Some functions' names were changed as Aaron's suggestions.

Comment 11

[Aaron Leventhal]

Comment on attachment 86538 [details] [diff] [review]
split nsAccessibleSelectable into nsHTMLSelectableAccessible and nsXULSelectableAccessible

great! r= aaronl

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 86538 [details] [diff] [review]
split nsAccessibleSelectable into nsHTMLSelectableAccessible and nsXULSelectableAccessible

- In nsHTMLSelectableAccessible::iterator::GetAccessibleIfSelected():

Make sure you never return w/o initializing *_retval!

+	 aAccService->CreateHTMLSelectOptionAccessible(mOption, mParent,
aContext, getter_AddRefs(tempAccess));
+	 *_retval = tempAccess;
+	 NS_ADDREF(*_retval);

Are you sure *_retval will always be non-null? What about OOM? How about
NS_IF_ADDREF() in stead?

- In nsHTMLSelectableAccessible::ChangeSelection(..., PRBool *aSelState):

Make sure you never return w/o initializing *aSelState!

+{
+  nsCOMPtr<nsIDOMHTMLSelectElement> htmlSelect(do_QueryInterface(mDOMNode));
+  if (htmlSelect) {

... How about returning early here if (!htmlSelect) in stead of putting all
this code inside this if?

- In nsHTMLComboboxAccessible, looks to me like there's a circular ownership
reference there that is never torn down. nsHTMLComboboxAccessible owns
mDOMNode, and nsHTMLComboboxAccessible sets |this| as an event listener on
mDOMNode, this will make a circular reference from
mDOMNode->listener_in_mDOMNode->mDOMNode_the_listener. You remove the listener
in the destructor of nsHTMLComboboxAccessible, but we'll never get there as
long as the listener is registerd...

Other than that this looks good, but the above needs fixing.

Comment 13

[Kyle Yuan]

Attachment: threw away the nsIDONXULListener from nsHTMLComboboxAccessible

Actually, nsIDONXULListener works incorrectly with nsHTMLComboboxAccessible. I
found a better way to determine whether the combobox ip dropped down.

I also fixed everything jst mentioned in comment 12.

Comment 14

[Aaron Leventhal]

Comment on attachment 86758 [details] [diff] [review]
threw away the nsIDONXULListener from nsHTMLComboboxAccessible

Better way to do the same thing. No need for nsIDOMXULListener.

r=aaronl

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 86758 [details] [diff] [review]
threw away the nsIDONXULListener from nsHTMLComboboxAccessible

sr=jst

Comment 16

[Kyle Yuan]

checked in!

Comment 17

[Kyle Yuan]

Jessie will be in charge of this bug.