bug fixing and polishing stage

very close to review

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 on attachment 388465 [details] [diff] [review] patch wrong version of patch

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.

Try build https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-e332589d6a87/

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.

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

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

(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

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 on attachment 388491 [details] [diff] [review] patch2 thunderbird looks ok with JAWS, so let's go then.

Comment on attachment 388491 [details] [diff] [review] patch2 Neil, would you have a chance to review this?

Note, this bug fixes bug 475325 and bug 360510

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.

(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.

(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.

(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 on attachment 388491 [details] [diff] [review] patch2 r=me for the test part and the functinality, which I found not breaking.

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.

(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!

cc'ing Neil.

Comment on attachment 388491 [details] [diff] [review] patch2 cancelling request for David's review since now we have Ginn's review.

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."]

(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."]

(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!)

(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.

up to dated to trunk with Neil's comment fixed

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

(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.

(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?

(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.

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

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

(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.

(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.

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

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

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.

(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?

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

(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 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 on attachment 395310 [details] [diff] [review] patch5 a192=beltzner, please land along with bug 516047

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

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)