275494 - XUL Tree can't work with gok

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

XUL tree element can't work with gok. This is a general issue. At present, when
clicking keyboard for XUL tree in gok, mozilla will hang. For example:

1. start mozilla and open preference dialog (nightly build from trunk)
2. start gok (nightly build from trunk)
3. click gok->UI Grab
4. click "Catogory" keyboard

Mozilla will hang.

Comment 1

[Louie Zhao]

Attachment: patch v1

This patch fixs several problems of XUL Tree Accessible:

1. fix the gap between "getColumnHeaderCB" of AtkTableIface and
"GetColumnHeader" of nsIAccessibleTable:
"getColumnHeaderCB" defined in AtkTableIface should return object whose role is
"ATK_ROLE_TABLE_COLUMN_HEADER", which is implemented by
nsXULTreeColumnitemAccessible; but "GetColumnHeader" defined in
nsIAccessibleTable returns nsXULTreeColumnsAccessibleWrap, which exports
nsIAccessibleTable and is "ROLE_LIST". It's wrong to return
"nsIAccessibleTable::GetColumnHeader" as "getColumnHeaderCB" of AtkTableIface
wanted.

2. Override "GetChildCount" in nsXULTreeAccessibleWrap.

In ATK, each tree cell accessible will be the direct child of tree accessible.
The children count of tree accessible should be "row count * col count +
treecols count". The crossflatform nsXULTreeAccessible defines "children count
= row count + treecols count". So we need to override "GetChildCount" for
nsXULTreeAccessibleWrap.

Question: Why nsXULTreeAccessible defines "children count = row count +
treecols count"? Is it because tree row accessible is direct child of tree
accessible and tree cell accessible is child of tree row accessible in
MSAA/MAC?

3. Override "ChangeSelection" in nsXULTreeAccessibleWrap
nsXULTreeAccessible::ChangeSelection can't work properly on Linux. 

Question:Now, the new code is in nsXULTreeAccessibleWrap. Actually, these are
crossplatform code and can be moved into nsXULTreeAccessible. Can someone try
it on MSAA/MAC to ensure it works fine on other platforms than Linux.

4. Change the role of "tree"/"tree item"/"tree col item" accessible on Linux

5. Support multiple actions for "tree item"
Expandable "tree item" should have 2 actions: activate and "expand/collapse".
The latest GOK has supported multiple actions of accessible. After applying
this to mozilla, one can manipulate mozilla tree freely with gok.

6.At accessible/src/xul/nsXULTreeAccessible.cpp @@ -586,12 +600,17 @@, there is
another small fix: The next sibling of the last cell should be null.

Comment 2

[Louie Zhao]

Attachment: patch v1

This patch fixs several problems of XUL Tree Accessible:

1. fix the gap between "getColumnHeaderCB" of AtkTableIface and
"GetColumnHeader" of nsIAccessibleTable:
"getColumnHeaderCB" defined in AtkTableIface should return object whose role is
"ATK_ROLE_TABLE_COLUMN_HEADER", which is implemented by
nsXULTreeColumnitemAccessible; but "GetColumnHeader" defined in
nsIAccessibleTable returns nsXULTreeColumnsAccessibleWrap, which exports
nsIAccessibleTable and is "ROLE_LIST". It's wrong to return
"nsIAccessibleTable::GetColumnHeader" as "getColumnHeaderCB" of AtkTableIface
wanted.

2. Override "GetChildCount" in nsXULTreeAccessibleWrap.

In ATK, each tree cell accessible will be the direct child of tree accessible.
The children count of tree accessible should be "row count * col count +
treecols count". The crossflatform nsXULTreeAccessible defines "children count
= row count + treecols count". So we need to override "GetChildCount" for
nsXULTreeAccessibleWrap.

Question: Why nsXULTreeAccessible defines "children count = row count +
treecols count"? Is it because tree row accessible is direct child of tree
accessible and tree cell accessible is child of tree row accessible in
MSAA/MAC?

3. Override "ChangeSelection" in nsXULTreeAccessibleWrap
nsXULTreeAccessible::ChangeSelection can't work properly on Linux. 

Question:Now, the new code is in nsXULTreeAccessibleWrap. Actually, these are
crossplatform code and can be moved into nsXULTreeAccessible. Can someone try
it on MSAA/MAC to ensure it works fine on other platforms than Linux.

4. Change the role of "tree"/"tree item"/"tree col item" accessible on Linux

5. Support multiple actions for "tree item"
Expandable "tree item" should have 2 actions: activate and "expand/collapse".
The latest GOK has supported multiple actions of accessible. After applying
this to mozilla, one can manipulate mozilla tree freely with gok.

6.At accessible/src/xul/nsXULTreeAccessible.cpp @@ -586,12 +600,17 @@, there is
another small fix: The next sibling of the last cell should be null.

Comment 3

[Aaron Leventhal]

Comment on attachment 169263 [details] [diff] [review]
patch v1

Louie, let's move the #ifdef'd code to the platform-specific *Wrap classes,
whenever we touch stuff. Or, do you have other fixes that are dependent on this
change?

Also, should a tree view really always be exposed as a table in ATK? What if
it's a column-less tree view like a list of folders?

Comment 4

[Aaron Leventhal]

Comment on attachment 169263 [details] [diff] [review]
patch v1

+  nsresult rv = GetRowAtIndex(aIndex, &rowIndex);
+
+  nsCOMPtr<nsITreeSelection> selection;
+  rv = mTreeView->GetSelection(getter_AddRefs(selection));
+  
+  if (selection && NS_SUCCEEDED(rv)) {

The first rv is not used. Also I wouldn't bother with the if () statement
unless it can sometimes be false. Just assert(selection).
No need ever to check rv if you're checking to see if the pointer is non-null.

Comment 5

[Louie Zhao]

Attachment: patch v2

Based on patch v1; several changes are made:

1. About the Role of XUL tree and tree item
The role of XUL tree & tree item should depend on the column count: if tree has
only 1 column, ROLE_OUTLINE (ATK_ROLE_TREE) is set; if tree view has more than
1 column, it's actually a table and ROLE_TREE_TABLE (ATK_ROLE_TREE_TABLE) is
set. This is also applied to XUL tree item: ROLE_OUTLINEITEM
(ATK_ROLE_LISTITEM) for tree item in 1-column tree  and ROLE_CELL
(ATK_ROLE_TABLE_CELL) for tree item in multiple-column tree.
NOTE: This change won't affect interaction with GOK. Screen reader will
distinguish the difference and read more exactly.

2.Change the way to calculate childcount of nsXULTreeAccessible following
Aaron's suggestion
The way getting column count of tree in nsXULTreeAccessible::GetChildCount is
not precise because the count of column headers is not alway equal to the count
of columns. For example, In "Bookmarks->Manage Bookmarks" dialog, a lot of
columns are hidden and have no cooresponding column headers.

Comment 6

[Aaron Leventhal]

Comment on attachment 169441 [details] [diff] [review]
patch v2

Do these 3 things and you have my r=

1. Define ROLE_TREE_TABLE for MSAA, in nsIAccessible.idl
enum { ROLE_TREE_TABLE = 35U }; // Same as ROLE_OUTLINE

2. I'm not sure how this will act in Windows screen readers with ROLE_CELL for
the items. For now For now, override GetRole in
msaa/nsXULTreeitemAccessibleWrap to always return ROLE_OUTLINEITEM. I'll fix it
to use ROLE_CELL later, once I get Window-Eyes and JAWS to handle that
situation.

3. do the small cleanup nit I suggest in comment 4

Comment 7

[Louie Zhao]

*** Bug 257095 has been marked as a duplicate of this bug. ***

Comment 8

[Louie Zhao]

Attachment: patch v3

Fix the 3 things Aaron mentioned.

Comment 9

[Louie Zhao]

Patch checked in.