523118 - we mistake 'cell' and text' xul tree seltypes for multiselects

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: first pass

Spun off from Neil's catch: bug 414302 comment 42. We should at least do this in nsXULTreeAccessible.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Alexander, what do you think?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Probably need a few more mochitests.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 407044 [details] [diff] [review]
first pass


>+    nsCOMPtr<nsITreeSelection> selection;
>+    mTreeView->GetSelection(getter_AddRefs(selection));
>+    if (selection) {
>+      PRBool single;
>+      selection->GetSingle(&single);

it can fail

technically we don't need to check if it's single because SelectAll() makes this for us (http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeSelection.cpp#537)

>+      if (!single) {
>+        *_retval = PR_TRUE;

nit: would be nice to rename it since you're here

>+        nsCOMPtr<nsITreeSelection> selection;
>+        mTreeView->GetSelection(getter_AddRefs(selection));
>+        if (selection)

it doesn't make sense to get selection second time and test it on null ;)

yeah, mochitests are good idea :)

otherwise is good :)

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

> >+        nsCOMPtr<nsITreeSelection> selection;
> >+        mTreeView->GetSelection(getter_AddRefs(selection));
> >+        if (selection)
> 
> it doesn't make sense to get selection second time and test it on null ;)
> 
> 

Ha! yeah my cut and paste forgot the cut part ;)

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

Fixes comments and adds tests.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 407091 [details] [diff] [review]
patch


>+NS_IMETHODIMP
>+nsXULTreeAccessible::SelectAllSelection(PRBool *success)

don't forget to use prefix 'a'

it's preferable to have more descriptive name like aIsMultiSelectable

> {
>-  *_retval = PR_FALSE;
>+  *success = PR_FALSE;

check by NS_ENSURE_ARG_POINTER

>+    nsCOMPtr<nsITreeSelection> selection;
>+    mTreeView->GetSelection(getter_AddRefs(selection));
>+    if (selection) {
>+          if (selection->SelectAll() == NS_OK) {

NS_SUCCEEDED in general

but nsresult of SelectAll doesn't point if tree is multiselectable, therefore you need to test this manually - I missed this in previous review.

>+            *success = PR_TRUE;

> 
>-        if (this.DOMNode.getAttribute("seltype") != "single")
>+        var seltype = this.DOMNode.getAttribute("seltype");
>+        if (seltype != "single" &&
>+            seltype != "cell" &&
>+            seltype != "text")
>           testStates(tree, STATE_MULTISELECTABLE);

this is nice, we need to keep this, possibly you should use GetSingle as well though I don't have strong opinion. But your tests don't test your patch.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> But your tests don't test your patch.

Doh! Thanks for the catches! I'll post a new patch later.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch2

Tests select all implementation for xul tree accessibles. Addresses comments.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 407294 [details] [diff] [review]
patch2


>       this.check = function check()
>       {
>         var tree = getAccessible(this.DOMNode);
> 
>         // tree states
>         testStates(tree, STATE_READONLY);
> 
>-        if (this.DOMNode.getAttribute("seltype") != "single")
>+        function ismultiselect(node) {

nit: isMultiSelectable ?

I'm not fun of function defining inside of other function

>+        // test SelectAllSelection correctly discerns multiselect vs single
>+        var accSelectable = getAccessible(this.DOMNode,
>+                                          [nsIAccessibleSelectable]);
>+        ok(accSelectable, "tree is not selectable!");
>+        if (accSelectable)
>+          is(accSelectable.selectAllSelection(), ismultiselect(this.DOMNode),
>+             "SelectAllSelection doesn't match ismultiselect");

I would prefer to have this test in separate file like test_selectable_tree.xul or something

You need to test tree selection additionally to see if all rows were selected

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: probably needs polish

Just want to get your eyes on this before you go to bed :)

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Oops please ignore straggling mods to test_states_tree - I'll remove locally.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 3

If you are still awake :)

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: better test

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 407561 [details] [diff] [review]
better test

sounds ok, thank you.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/b81fc13d9045