503727 - Reorganize implementation of XUL tree accessibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wip

bug fixing and polishing stage

Comment 2

[alexander :surkov (:asurkov)]

Attachment: wip2

very close to review

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Ok, it works somehow with NVDA, a bit worth with JAWS (strange focus behaviour). Marco, could you do some testing please? If you like then try build will be available soon.

Comment 4

[alexander :surkov (:asurkov)]

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

wrong version of patch

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

I tried Bookmarks tree with JAWS, works fine. However I see some focus problems (on tab) in mochitests where XUL tree is used and don't understand the nature of problems.

Comment 6

[alexander :surkov (:asurkov)]

Try build https://build.mozilla.org/tryserver-builds/[e-mail]-try-e332589d6a87/

Comment 7

[alexander :surkov (:asurkov)]

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

though I can reproduce strange focus behaviour with XUL trees in mochitests without my patch.

Marco, you're testing is appreciated.

Comment 8

[alexander :surkov (:asurkov)]

James, could you please test the try build as well to see possible regressions or problems?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: testcase (treeview.js file)

Comment 10

[alexander :surkov (:asurkov)]

Attachment: testcase (treelist.xul, used together with treeview.js)

I can't navigate by arrow keys through list. With and without the patch.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Created an attachment (id=388636) [details]
> testcase (treelist.xul, used together with treeview.js)
> 
> I can't navigate by arrow keys through list. With and without the patch.

btw, this might be problems with permissions

Comment 12

[alexander :surkov (:asurkov)]

Marco, you can run test_events_tree.xul and navigate through controls to XUL tree by tab with running JAWS, you should see what I see :)

Comment 13

[alexander :surkov (:asurkov)]

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

thunderbird looks ok with JAWS, so let's go then.

Comment 14

[alexander :surkov (:asurkov)]

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

Neil, would you have a chance to review this?

Comment 15

[alexander :surkov (:asurkov)]

Note, this bug fixes bug 475325 and bug 360510

Comment 16

[Marco Zehe (:MarcoZ)]

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

Nits:
>+ *    // [optional] identifier of target DOM events listeners are registered on,
>+ *    // used with 'events', if missed then 'ID' is used instead.

Should be "if missing then...".

>+      var expextedFirstChild = childCount > 0 ?

>+      is(firstChild, expextedFirstChild,

Make these "expectedFirstChild" please, you have correct "expectedLastChild" below.

In test_actions_tree.xul, you have a few actions where the actionIndex is not specified, and others where it is 1. However in test_actions_treegrid.xul, you do have those actionIndex members that are 0 specified as such. Please add them to test_actions_tree.xul as well for consistency.

And now for the big question:
>+      <tree id="list" flex="1" hidecolumnpicker="true">
>+        <treecols>
>+          <treecol id="col" flex="1" hideheader="true"/>
>+        </treecols>
>+        <treechildren/>
>+      </tree>

Why do we expose this kind of tree not as a tree view, like the folder tree in Windows Explorer or Outlook Express, but as a listbox instead? See:

>+      gQueue.push(new treeChecker("list", new nsTableTreeView(3), ROLE_LIST));

Where does this come from? Does the original reason for doing it this way still apply? And if not, we should change it while we're here.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> Why do we expose this kind of tree not as a tree view, like the folder tree in
> Windows Explorer or Outlook Express, but as a listbox instead? See:

Iirc Right panel of Windows Explorer is listbox if you chosen list view. I think it makes sense because we don't need to expose tree if this element is listbox. We don't expose tree for html:select for example so I don't see why we should expose tree in this case.

> 
> Where does this come from? Does the original reason for doing it this way still
> apply? And if not, we should change it while we're here.

It comes from bug 286029:

"Finally, because we as if each row is 1 item in MSAA, we should just expose flat
tree views as lists with list items."

I think it still apply.

Comment 18

[Marco Zehe (:MarcoZ)]

(In reply to comment #17)
> (In reply to comment #16)
> 
> > Why do we expose this kind of tree not as a tree view, like the folder tree in
> > Windows Explorer or Outlook Express, but as a listbox instead? See:
> Iirc Right panel of Windows Explorer is listbox if you chosen list view. I
> think it makes sense because we don't need to expose tree if this element is
> listbox. We don't expose tree for html:select for example so I don't see why we
> should expose tree in this case.

The right pane in Windows Explorer, if set to "Details" view, is a list view, yes. I was referring to the left pane, which displays folders in a hierarchical fashion. It does not have a columnheader either.
So does if we don't show column headers and have no primary column in xul:tree, that this is a flat list that has no hierarchical depth, e. g., all items are on one level and can't be expanded or collapsed? If that's the case, then I'm fine with keeping this as is.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > 
> > > Why do we expose this kind of tree not as a tree view, like the folder tree in
> > > Windows Explorer or Outlook Express, but as a listbox instead? See:
> > Iirc Right panel of Windows Explorer is listbox if you chosen list view. I
> > think it makes sense because we don't need to expose tree if this element is
> > listbox. We don't expose tree for html:select for example so I don't see why we
> > should expose tree in this case.
> 
> The right pane in Windows Explorer, if set to "Details" view, is a list view,
> yes. I was referring to the left pane, which displays folders in a hierarchical
> fashion. It does not have a columnheader either.
> So does if we don't show column headers and have no primary column in xul:tree,
> that this is a flat list that has no hierarchical depth, e. g., all items are
> on one level and can't be expanded or collapsed? If that's the case, then I'm
> fine with keeping this as is.

Exactly if we don't have primary column then this is flat view, no hierarchical data, nothing to expand or collapse. However even if column header is invisible but we have primary column then we will expose tree like we do in the left pane of Bookmarks dialog.

Comment 20

[Marco Zehe (:MarcoZ)]

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

r=me for the test part and the functinality, which I found not breaking.

Comment 21

[Ginn Chen]

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

+++ b/accessible/src/atk/nsXULTreeAccessibleWrap.cpp
@@ -40,409 +40,50 @@
 #include "nsIDOMElement.h"
 #include "nsITreeSelection.h"
 #include "nsITreeColumns.h"
 #include "nsXULTreeAccessibleWrap.h"
 
 // --------------------------------------------------------
 // nsXULTreeAccessibleWrap Accessible
 // --------------------------------------------------------
You may want to change the comment to nsXULTreeGridAccessibleWrap.

The patch looks good in general.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

>  // --------------------------------------------------------
>  // nsXULTreeAccessibleWrap Accessible
>  // --------------------------------------------------------
> You may want to change the comment to nsXULTreeGridAccessibleWrap.

 I think once I'll get rid this class at all because it contains table header related methods which are my next step of xul:tree reorganization. Therefore I didn't changed it this time. If you don't insist then I won't change the comment.

> 
> The patch looks good in general.

Thank you much for quick review of this huge patch!

Comment 23

[alexander :surkov (:asurkov)]

cc'ing Neil.

Comment 24

[alexander :surkov (:asurkov)]

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

cancelling request for David's review since now we have Ginn's review.

Comment 25

[[e-mail]]

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

>+NS_IMETHODIMP
>+nsXULTreeGridAccessible::SelectRow(PRInt32 aRowIndex)
>+{
>+  nsCOMPtr<nsITreeSelection> selection;
>+  mTreeView->GetSelection(getter_AddRefs(selection));
>+  NS_ENSURE_STATE(selection);
>+
>+  return selection->Select(aRowIndex);
>+}
This seems to behave very slightly differently to your other implementations; it always sets the current index to the given row even if it is already selected.

>+NS_IMETHODIMP
>+nsXULTreeGridAccessible::UnselectRow(PRInt32 aRowIndex)
>+{
>+  nsCOMPtr<nsITreeSelection> selection;
>+  mTreeView->GetSelection(getter_AddRefs(selection));
>+  NS_ENSURE_STATE(selection);
>+
>+  PRBool isSelected = PR_FALSE;
>+  nsresult rv = selection->IsSelected(aRowIndex, &isSelected);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (isSelected)
>+    return NS_OK;
>+
>+  return selection->ToggleSelect(aRowIndex);
>+}
Of course if that's intentional you could return selection->ClearRange(aRowIndex, aRowIndex); here instead.

>                  b.view.setCellValue(row.value, col.value, value);
>+
>+                 // Even checkbox cell is redrawn because of repainting on mouse
>+                 // events but we need to invalidate the cell to notify AT
>+                 // (assistive technologies) softwares.
>+                 b.invalidateCell(row.value, col.value);
The view is responsible for invalidation. Are you somehow depending on the cell to be invalidated? The default content view only bothers invalidating rows.
[Also, I wouldn't bother with the "AT" abbreviation, I would just write "assistive technology software."]

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)

> This seems to behave very slightly differently to your other implementations;
> it always sets the current index to the given row even if it is already
> selected.

Unfortunately the IA2 spec doesn't address this. Technically currentIndex means the treeitem is focused for AT, I'm not sure whether we should change focus when AT needs to select a row. However this is good point, I'll rise the question on IA2 group discussion.

> Of course if that's intentional you could return
> selection->ClearRange(aRowIndex, aRowIndex); here instead.

You're right, I'll change it on ClearRange.

> 
> >                  b.view.setCellValue(row.value, col.value, value);
> >+
> >+                 // Even checkbox cell is redrawn because of repainting on mouse
> >+                 // events but we need to invalidate the cell to notify AT
> >+                 // (assistive technologies) softwares.
> >+                 b.invalidateCell(row.value, col.value);
> The view is responsible for invalidation. Are you somehow depending on the cell
> to be invalidated? The default content view only bothers invalidating rows.

Partially we need to send state change event for checkbox cell to AT when it's clicked therefore I notify a11y event listener that cell was invalidated and possibly it's value was changed as well.

> [Also, I wouldn't bother with the "AT" abbreviation, I would just write
> "assistive technology software."]

Comment 27

[[e-mail]]

(In reply to comment #26)
> Partially we need to send state change event for checkbox cell to AT when it's
> clicked therefore I notify a11y event listener that cell was invalidated and
> possibly it's value was changed as well.
While you can expect to get some sort of invalidate event when a cell's value changes you can't guarantee it will be specific to the cell that changed; a view is perfectly entitled to invalidate the whole tree in this case. (SeaMonkey's download manager actually does currently invalidate the whole tree when a single cell changes but this is awful for perf so we are looking into fixing it!)

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> (In reply to comment #26)
> > Partially we need to send state change event for checkbox cell to AT when it's
> > clicked therefore I notify a11y event listener that cell was invalidated and
> > possibly it's value was changed as well.
> While you can expect to get some sort of invalidate event when a cell's value
> changes you can't guarantee it will be specific to the cell that changed; a
> view is perfectly entitled to invalidate the whole tree in this case.
> (SeaMonkey's download manager actually does currently invalidate the whole tree
> when a single cell changes but this is awful for perf so we are looking into
> fixing it!)

Yes, it is performance issue only. Currently (without and within this patch) we cache cell text and if the cell (or whatever) is invalidated then we check if cached text was changed also. If it was then we fire events.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch3

up to dated to trunk with Neil's comment fixed

Comment 30

[[e-mail]]

Sorry, I don't see the benefit of firing extra invalidation events if you have to do the work for existing invalidation events anyway.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> Sorry, I don't see the benefit of firing extra invalidation events if you have
> to do the work for existing invalidation events anyway.

Do you mean toolkit changes of the patch? We don't get invalidation events when user clicks on checkbox cell, therefore I added it.

Comment 32

[[e-mail]]

(In reply to comment #31)
> We don't get invalidation events when user clicks on checkbox cell
You should always get invalidation when a cell's value changes, otherwise how else would it get repainted in the new state?

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> (In reply to comment #31)
> > We don't get invalidation events when user clicks on checkbox cell
> You should always get invalidation when a cell's value changes, otherwise how
> else would it get repainted in the new state?

I tried to dig through the code but it was hard to understand because it's quite new for me. Iirc it looked like tree is redrawn on mouse events and invalidation methods of tree aren't used in this case. I just noticed we don't fire proper a11y events because we don't get invalidation events. That was a reason to add this change to toolkit code.

Comment 34

[[e-mail]]

Well I don't understand why you're not seeing an invalidate to CCing Enn.

Comment 35

[alexander :surkov (:asurkov)]

Attachment: patch4

adjustment of dispatch click code (tree actions mochitest fails) because of bug 352093 supposedly.

Comment 36

[Neil Deakin]

(In reply to comment #33)
> I tried to dig through the code but it was hard to understand because it's
> quite new for me. Iirc it looked like tree is redrawn on mouse events and
> invalidation methods of tree aren't used in this case. I just noticed we don't
> fire proper a11y events because we don't get invalidation events. That was a
> reason to add this change to toolkit code.

Which tree is this? Do you have a testcase?

If you mean the tree view defined in this patch in accessible/tests/mochitest/treeview.js, then it should be invalidating the cell/row within its setCellValue method.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> (In reply to comment #33)
> > I tried to dig through the code but it was hard to understand because it's
> > quite new for me. Iirc it looked like tree is redrawn on mouse events and
> > invalidation methods of tree aren't used in this case. I just noticed we don't
> > fire proper a11y events because we don't get invalidation events. That was a
> > reason to add this change to toolkit code.
> 
> Which tree is this? Do you have a testcase?
> 
> If you mean the tree view defined in this patch in
> accessible/tests/mochitest/treeview.js, then it should be invalidating the
> cell/row within its setCellValue method.

Yes, I meant it. If tree author should invalidate the cell inside of setCellValue then it's dangerous because checkbox cell value is changed without invalidation inside of setCellValue. I would suppose author won't invalidate because everything is fine on-screen, AT suffers only which is not evident for sighted users. Though if current paradigm is tree view author must call it then I will drop that toolkit changes and fix my mochitests. But there is something bad in this approach I think.

Comment 38

[[e-mail]]

Tree authors are also supposed to invalidate for toggling twisties. For instance, the certificate manager fails to do this. (Oops!)

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch5

remove changes from tree.xml, move cell invalidation into mochitests tree view implementation.

Comment 40

[[e-mail]]

Comment on attachment 395310 [details] [diff] [review]
patch5

>+NS_IMETHODIMP
>+nsXULTreeGridAccessible::UnselectRow(PRInt32 aRowIndex)
>+{
>+  nsCOMPtr<nsITreeSelection> selection;
>+  mTreeView->GetSelection(getter_AddRefs(selection));
>+  NS_ENSURE_STATE(selection);
>+
>+  PRBool isSelected = PR_FALSE;
>+  nsresult rv = selection->IsSelected(aRowIndex, &isSelected);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!isSelected)
>+    return NS_OK;
>+
>+  return selection->ClearRange(aRowIndex, aRowIndex);
Nit: no need to check IsSelected before calling ClearRange.

>+  cycleCell: function cycleCell(aRow, aCol)
>+  {
>+    var data = this.getDataForIndex(aRow);
>+    data.cyclerState = (data.cyclerState + 1) % 3;
Nit: should invalidate the cell whenever it gets changed; other actions may inadvertently invalidate the cell but you can't rely on it.

Comment 41

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

(In reply to comment #37)
> If tree author should invalidate the cell inside of setCellValue then it's
> dangerous because checkbox cell value is changed without invalidation inside of
> setCellValue. I would suppose author won't invalidate

Perhaps we should file a spinoff bug for further discussion?

Comment 42

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8b64b6edf62e

Comment 43

[alexander :surkov (:asurkov)]

(In reply to comment #41)
> (In reply to comment #37)
> > If tree author should invalidate the cell inside of setCellValue then it's
> > dangerous because checkbox cell value is changed without invalidation inside of
> > setCellValue. I would suppose author won't invalidate
> 
> Perhaps we should file a spinoff bug for further discussion?

I filed bug 511867.

Comment 44

[Marco Zehe (:MarcoZ)]

Comment on attachment 395310 [details] [diff] [review]
patch5

Baked on m-c for 6 weeks, caused a small regression which was fixed mid september, and is needed for completeness of table-related a11y features. See https://wiki.mozilla.org/Accessibility/Remaining_Mozilla-1.9.2_Nominations for more details.

Comment 45

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 395310 [details] [diff] [review]
patch5

a192=beltzner, please land along with bug 516047

Comment 46

[alexander :surkov (:asurkov)]

landed on 1.9.2 - http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a05dc4dfdfb2

Comment 47

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b2pre) Gecko/20091029 Namoroka/3.6b2pre (.NET CLR 3.5.30729)