The following bugs must be fixed by the wip patch I have, these are bug 494123, bug 441445, bug 433304, bug 417797, bug 417791.

enjoy, friends ;)

Only 528.75 KB? I think I need some lunch :)

Comment on attachment 396498 [details] [diff] [review] patch One thing I noticed is this (and similar): >+ * Return an array of cells indices currently selected. Did you mean either: "cell's indices", meaning "the indices of this particular cell), or cell indices (the indices of any one cell)? "cells indices", the way you wrote it in here, also same with "columns indices" and "rows indices", is grammatically incorrect. So it's either "cell's indices", in which case you should only change the comment, or "cell indices", in which case the method names should also be changed for this and column and row indices, to give names in correct English. David, would you agree with that?

Ok, IA2 uses cell indexes, so I'll change GetCellsIndices -> GetCellIndices and etc. Thanks for the catch

1) final IA2 IDL version 2) renamed methods as Marco suggested 3) added missed mochitest

Comment on attachment 396688 [details] [diff] [review] patch2 Hi Alex, I'll review this is patch a bit at a time. > nsIAccessible createHTMLTableCellAccessible(in nsIFrame aFrame); >- nsIAccessible createHTMLTableHeadAccessible(in nsIDOMNode aDOMNode); Is this because we won't special case table header cells? >-ifdef MOZ_XUL >-CPPSRCS += \ >- nsXULTreeAccessibleWrap.cpp \ >- $(NULL) >-endif >- >- nsXULTreeAccessibleWrap.h \ >+ nsXULListboxAccessibleWrap.h \ >+ nsXULTreeGridAccessibleWrap.h \ Yay! Since this is a large change we should test this manually with Orca and NVDA at least. So far so good, more review later. Note to self: left of in atk wrapper

(In reply to comment #8) > > nsIAccessible createHTMLTableCellAccessible(in nsIFrame aFrame); > >- nsIAccessible createHTMLTableHeadAccessible(in nsIDOMNode aDOMNode); > > Is this because we won't special case table header cells? Yes, I think it doesn't make sense to expose accessible for thead because header information can be provided by different ways including ways where thead is not used. Nobody gets anything useful if we'll alter table accessible tree.

(In reply to comment #9) > > Is this because we won't special case table header cells? > > Yes, I think it doesn't make sense to expose accessible for thead because > header information can be provided by different ways including ways where thead > is not used. Nobody gets anything useful if we'll alter table accessible tree. I should add previously thead accessible was used to expose heading information (especially in IA2). Since IA2 API has been changed then we don't need thead accessible any more as requirement of AT API.

Comment on attachment 396688 [details] [diff] [review] patch2 One thing I found so far: >diff --git a/accessible/tests/mochitest/test_table_headers_tree.xul b/accessible/tests/mochitest/test_table_headers_tree.xul > >+ testHeaderCells("listbox", headerInfoMap); This should be "tree" instead of "listbox". There is no element with id "listbox" in this particular file. The tests look good all in all, but I want to go over them again later for a second pass.

(In reply to comment #11) > (From update of attachment 396688 [details] [diff] [review]) > One thing I found so far: > >diff --git a/accessible/tests/mochitest/test_table_headers_tree.xul b/accessible/tests/mochitest/test_table_headers_tree.xul > > > >+ testHeaderCells("listbox", headerInfoMap); > > This should be "tree" instead of "listbox". There is no element with id > "listbox" in this particular file. This argument is rudiment during test evaluation :) aIdentifier argument of testHeaderCell isn't used at all, all necessary information is enclosed inside of second argument. I fixed locally. Do you need new patch for this?

No I don't need a new patch for this. Thanks!

Comment on attachment 396688 [details] [diff] [review] patch2 >+ // If the cell at the first row is column header then assume it is column >+ // header for all rows, Can you explain how we can assume that? I think I need to understand that before I can review the rest of this method; ditto for: >+ // If the cell at the first column is row header then assume it is row >+ // header for all columns, >+ if (nsAccUtils::Role(accCell) == nsIAccessibleRole::ROLE_ROWHEADER) >+ return nsAccessibleWrap::GetAtkObject(accCell); >+nsARIAGridCellAccessible::GetColumnIndex(PRInt32 *aColumnIndex) >+{ >+ prevCell->GetPreviousSibling(getter_AddRefs(tmpAcc)); We do this because of hidden cells right (GetPreviousSibling jumps over them)? >+NS_IMETHODIMP >+nsARIAGridCellAccessible::GetColumnExtent(PRInt32 *aExtentCount) >+ *aExtentCount = 1; >+ return NS_OK; Why is the row/col extents for a grid cell always 1? Is that fundamental to how we define an ARIA grid? (If so, I'm happy) >+PRBool >+nsAccUtils::IsARIASelected(nsIAccessible *aAccessible) I like that you moved this to utils. [Note to self: left of at nsAccUtils::GetHeaderCellsFor]

Comment on attachment 396688 [details] [diff] [review] patch2 >+ if (origIdx == index) { >+ // Append original header cells only. >+ PRInt32 isHeader = moveToLeft ? >+ Role(cell) == nsIAccessibleRole::ROLE_ROWHEADER : >+ Role(cell) == nsIAccessibleRole::ROLE_COLUMNHEADER; Tiny nit: Role(cell) probably inlines the code from nsAccUtils.h; maybe just check once? >-#ifndef MOZ_ACCESSIBILITY_ATK >- tag == nsAccessibilityAtoms::tbody || >- tag == nsAccessibilityAtoms::tfoot || >- tag == nsAccessibilityAtoms::thead || >-#endif So nice to get rid of this preprocessing :) >+nsITableCellLayout* >+nsHTMLTableCellAccessible::GetCellLayout() >+{ I guess the callers check for IsDefunct() already? [left off at nsHTMLTableAccessible::GetSelectedCells]

(In reply to comment #15) > Tiny nit: Role(cell) probably inlines the code from nsAccUtils.h; maybe just > check once? Oops I mean 'call' once

Comment on attachment 396688 [details] [diff] [review] patch2 [left off here: nsHTMLTableAccessible::GetColumnExtentAt]

(In reply to comment #14) > (From update of attachment 396688 [details] [diff] [review]) > > >+ // If the cell at the first row is column header then assume it is column > >+ // header for all rows, > > Can you explain how we can assume that? I think I need to understand that > before I can review the rest of this method; ditto for: Here's I handle most expected case for example like: <table> <thead> <tr> <th>header</th> <th>header</th> </tr> </thead> <tr> <td>bla</td><td>bla</td> </tr> </table> Here's cells on the first row are column headers and they really describe whole columns. I can't think of better assumption for ATK. It sounds ATK was designed for cases when headers have simple structure and can be easily recognized like we have for XUL tree for example (header cells are contained by list separately from data cells, there is only one header cell for all cells in the column). But I think ATK isn't very suitable for HTML and ARIA since headers structure can be not trivial. > >+nsARIAGridCellAccessible::GetColumnIndex(PRInt32 *aColumnIndex) > >+{ > > >+ prevCell->GetPreviousSibling(getter_AddRefs(tmpAcc)); > > We do this because of hidden cells right (GetPreviousSibling jumps over them)? I didn't consider this case. I don't think it makes sense, at least I don't understand how it can be used. We could run by DOM nodes and get their accessibles to check the role. But walking through accessible tree is also good I think. > > >+NS_IMETHODIMP > >+nsARIAGridCellAccessible::GetColumnExtent(PRInt32 *aExtentCount) > > >+ *aExtentCount = 1; > >+ return NS_OK; > > Why is the row/col extents for a grid cell always 1? Is that fundamental to how > we define an ARIA grid? (If so, I'm happy) I think so, I don't think ARIA has a way to define column or row spans. If author needs them then he should use ARIA + HTML table but this case is handled by nsHTMLTableAccessible and etc. > >+PRBool > >+nsAccUtils::IsARIASelected(nsIAccessible *aAccessible) > > I like that you moved this to utils. that's great :) because I have doubts on this

(In reply to comment #15) > (From update of attachment 396688 [details] [diff] [review]) > > >+ if (origIdx == index) { > >+ // Append original header cells only. > >+ PRInt32 isHeader = moveToLeft ? > >+ Role(cell) == nsIAccessibleRole::ROLE_ROWHEADER : > >+ Role(cell) == nsIAccessibleRole::ROLE_COLUMNHEADER; > > Tiny nit: Role(cell) probably inlines the code from nsAccUtils.h; maybe just > check once? sorry, I didn't catch you about "check once" > >-#ifndef MOZ_ACCESSIBILITY_ATK > >- tag == nsAccessibilityAtoms::tbody || > >- tag == nsAccessibilityAtoms::tfoot || > >- tag == nsAccessibilityAtoms::thead || > >-#endif > > So nice to get rid of this preprocessing :) yeah, I do not like them as well :) > >+nsITableCellLayout* > >+nsHTMLTableCellAccessible::GetCellLayout() > >+{ > > I guess the callers check for IsDefunct() already? I guess so. It's internal method and caller should ensure it's ok to use it.

(In reply to comment #16) > (In reply to comment #15) > > Tiny nit: Role(cell) probably inlines the code from nsAccUtils.h; maybe just > > check once? > > Oops I mean 'call' once Ah, you prefer to see PRUint32 role = Role(cell); PRInt32 isHeader = moveToLeft ? role == bla : role == bla? Ok, I'll fix it.

(In reply to comment #20) > Ah, you prefer to see PRUint32 role = Role(cell); PRInt32 isHeader = moveToLeft > ? role == bla : role == bla? Ok, I'll fix it. Thanks.

Comment on attachment 396688 [details] [diff] [review] patch2 >+nsITableLayout* >+nsHTMLTableAccessible::GetTableLayout() > { >+ nsCOMPtr<nsIContent> tableContent(do_QueryInterface(mDOMNode)); >+ if (!tableContent) >+ return nsnull; I think there is a place you query for nsIContent earlier in this patch and don't bail if fail (I let it go, but we should be consistent I think). Thoughts? When will nsIContent fail - we should always have a valid mDOMNode unless IsDefunct presumably. >+#define CANT_QUERY_ASSERTION_MSG \ >+"Subclass of CAccessibleTableCell doesn't implement nsIAccessibleTableCell"\ Can you rename CANT_QUERY_ASSERTION_MSG to be more specific to the message text. Maybe: #define TABLECELL_INTERFACE_UNSUPPORTED_MSG >+STDMETHODIMP >+CAccessibleTableCell::get_table(IUnknown **table) >+{ >+__try { >+ nsCOMPtr<nsIAccessibleTableCell> tableCell(do_QueryInterface(this)); >+ NS_ASSERTION(tableCell, CANT_QUERY_ASSERTION_MSG); >+ if (!tableCell) >+ return E_FAIL; >+ >+ nsCOMPtr<nsIAccessibleTable> geckoTable; >+ nsresult rv = tableCell->GetTable(getter_AddRefs(geckoTable)); >+ if (NS_FAILED(rv)) >+ return GetHRESULT(rv); >+ >+ nsCOMPtr<nsIWinAccessNode> winAccessNode = do_QueryInterface(geckoTable); >+ if (!winAccessNode) >+ return E_FAIL; >+ >+ void *instancePtr = NULL; >+ rv = winAccessNode->QueryNativeInterface(IID_IAccessibleTable2, >+ &instancePtr); It feels klunky to me that we have to query the IID_IAccessibleTable2 from nsIWinAccessNode, but I guess this is the only way? >+IMPL_IUNKNOWN_INHERITED1(nsARIAGridCellAccessibleWrap, >+ nsHyperTextAccessibleWrap, >+ CAccessibleTableCell); Nit: probably don't need the ";" >+IMPL_IUNKNOWN_INHERITED1(nsXULListCellAccessibleWrap, >+ nsHyperTextAccessibleWrap, >+ CAccessibleTableCell); same here (and elsewhere) (I'm not sure why/if we need the ;) > IMPL_IUNKNOWN_INHERITED1(nsXULTreeGridAccessibleWrap, > nsAccessibleWrap, > CAccessibleTable); But there is precedent :) >+ CAccessibleTableCell); >+NS_IMETHODIMP >+nsXULComboboxAccessible::GetDescription(nsAString& aDescription) >+{ >+ aDescription.Truncate(); >+ >+ if (IsDefunct()) >+ return NS_ERROR_FAILURE; >+ >+ // Use description of currently focused option >+ nsCOMPtr<nsIDOMXULMenuListElement> menuListElm(do_QueryInterface(mDOMNode)); >+ if (!menuListElm) >+ return NS_ERROR_FAILURE; >+ >+ nsCOMPtr<nsIDOMXULSelectControlItemElement> focusedOptionItem; Why the "focused" prefix on this name? >+ menuListElm->GetSelectedItem(getter_AddRefs(focusedOptionItem)); >+ nsCOMPtr<nsIDOMNode> focusedOptionNode(do_QueryInterface(focusedOptionItem)); >+NS_IMETHODIMP >+nsXULComboboxAccessible::GetActionName(PRUint8 aIndex, nsAString& aName) >+ // Our action name is the reverse of our state: >+ // if we are closed -> open is our name. >+ // if we are open -> closed is our name. Nit: "close" >--- a/accessible/src/xul/nsXULSelectAccessible.cpp >+++ b/accessible/src/xul/nsXULListboxAccessible.cpp >@@ -15,47 +15,44 @@ > * The Original Code is mozilla.org code. > * > * The Initial Developer of the Original Code is > * Netscape Communications Corporation. > * Portions created by the Initial Developer are Copyright (C) 1998 > * the Initial Developer. All Rights Reserved. > * > * Contributor(s): >- * Original Author: Eric Vaughan (evaughan@netscape.com) >- * Kyle Yuan (kyle.yuan@sun.com) >+ * Aaron Leventhal <aaronl@netscape.com> (original author) >+ * Kyle Yuan <kyle.yuan@sun.com> >+ * Alexander Surkov <surkov.alexander@gmail.com> Oops, what about "Eric Vaughan" ? [left off here: nsXULListboxAccessible::GetSelectedCells]

Comment on attachment 396688 [details] [diff] [review] patch2 > nsCOMPtr<nsIDOMXULMultiSelectControlElement> control = >- do_QueryInterface(mDOMNode); >+ do_QueryInterface(mDOMNode); Not sure what this change is, but indenting is good here I think. >+NS_IMETHODIMP >+nsXULListCellAccessible::GetTable(nsIAccessibleTable **aTable) >+{ >+ nsCOMPtr<nsIAccessible> table; >+ thisRow->GetParent(getter_AddRefs(table)); >+ if (nsAccUtils::Role(table) != nsIAccessibleRole::ROLE_TABLE) >+ return NS_OK; >+ >+ CallQueryInterface(table, aTable); CallQueryInterface doesn't null check table. We should probably check the success of GetParent above. >+ if (role == nsIAccessibleRole::ROLE_CELL || >+ role == nsIAccessibleRole::ROLE_GRID_CELL || >+ role == nsIAccessibleRole::ROLE_ROWHEADER || >+ role == nsIAccessibleRole::ROLE_COLUMNHEADER) Util method? (Do we check these elsewhere?) >+nsXULListCellAccessible::GetColumnExtent(PRInt32 *aExtentCount) >+nsXULListCellAccessible::GetRowExtent(PRInt32 *aExtentCount) Again, are we ok that these are always 1? OK! That's my first pass through the accessible module changes except for the idl stuff which I presume is rudimentary changes based on Pete's idls. I didn't review the mochitests or the toolkit changes. I'd like to see the final patch again; after Neil and Marco catch all the things I missed :)

(In reply to comment #22) > > * Contributor(s): > >- * Original Author: Eric Vaughan (evaughan@netscape.com) > >- * Kyle Yuan (kyle.yuan@sun.com) > >+ * Aaron Leventhal <aaronl@netscape.com> (original author) > >+ * Kyle Yuan <kyle.yuan@sun.com> > >+ * Alexander Surkov <surkov.alexander@gmail.com> > > Oops, what about "Eric Vaughan" ? > > [left off here: nsXULListboxAccessible::GetSelectedCells] fight for historical justice :) This comes from bug 102021 where Aaron copied/pasted the name of Eric. However Aaron fixed this as reviewer requested but eventually reviewer landed previous patch.

(In reply to comment #22) > (From update of attachment 396688 [details] [diff] [review]) > >+nsITableLayout* > >+nsHTMLTableAccessible::GetTableLayout() > > { > >+ nsCOMPtr<nsIContent> tableContent(do_QueryInterface(mDOMNode)); > >+ if (!tableContent) > >+ return nsnull; > > I think there is a place you query for nsIContent earlier in this patch and > don't bail if fail (I let it go, but we should be consistent I think). > Thoughts? When will nsIContent fail - we should always have a valid mDOMNode > unless IsDefunct presumably. fixed > Can you rename CANT_QUERY_ASSERTION_MSG to be more specific to the message > text. > Maybe: #define TABLECELL_INTERFACE_UNSUPPORTED_MSG if you like :) fixed. > It feels klunky to me that we have to query the IID_IAccessibleTable2 from > nsIWinAccessNode, but I guess this is the only way? Right. That's the way to query MSAA/IA2 interface from Gecko's one. > > >+IMPL_IUNKNOWN_INHERITED1(nsARIAGridCellAccessibleWrap, > >+ nsHyperTextAccessibleWrap, > >+ CAccessibleTableCell); > > Nit: probably don't need the ";" fixed, it's not needed. > >+NS_IMETHODIMP > >+nsXULComboboxAccessible::GetDescription(nsAString& aDescription) > >+{ > >+ // Use description of currently focused option > >+ nsCOMPtr<nsIDOMXULMenuListElement> menuListElm(do_QueryInterface(mDOMNode)); > >+ if (!menuListElm) > >+ return NS_ERROR_FAILURE; > >+ > >+ nsCOMPtr<nsIDOMXULSelectControlItemElement> focusedOptionItem; > > Why the "focused" prefix on this name? I think to show combobox's description is formed from focused option name.

(In reply to comment #23) > (From update of attachment 396688 [details] [diff] [review]) > > nsCOMPtr<nsIDOMXULMultiSelectControlElement> control = > >- do_QueryInterface(mDOMNode); > >+ do_QueryInterface(mDOMNode); > > Not sure what this change is, but indenting is good here I think. That's features of big patches. You fix what you see :) If you like then I'll rollback the change but if you won't insist then I would save it. > > >+NS_IMETHODIMP > >+nsXULListCellAccessible::GetTable(nsIAccessibleTable **aTable) > >+{ > > >+ nsCOMPtr<nsIAccessible> table; > >+ thisRow->GetParent(getter_AddRefs(table)); > >+ if (nsAccUtils::Role(table) != nsIAccessibleRole::ROLE_TABLE) > >+ return NS_OK; > >+ > >+ CallQueryInterface(table, aTable); > > CallQueryInterface doesn't null check table. We should probably check the > success of GetParent above. the 'if' statement above guarantees table is not null. > > >+ if (role == nsIAccessibleRole::ROLE_CELL || > >+ role == nsIAccessibleRole::ROLE_GRID_CELL || > >+ role == nsIAccessibleRole::ROLE_ROWHEADER || > >+ role == nsIAccessibleRole::ROLE_COLUMNHEADER) > > Util method? (Do we check these elsewhere?) I thought about this we have couple of similar checks but it would be hard to to form them as util method. Somewhere we don't check ROLE_CELL, somewhere we check row and column headers only, somewhere nsIAccessibleRole::GetRole is used, somewhere ARIA role is used. > > >+nsXULListCellAccessible::GetColumnExtent(PRInt32 *aExtentCount) > > >+nsXULListCellAccessible::GetRowExtent(PRInt32 *aExtentCount) > > Again, are we ok that these are always 1? Yes because XUL listbox don't support column and row spans. > > OK! That's my first pass through the accessible module changes except for the > idl stuff which I presume is rudimentary changes based on Pete's idls. Right. It's up to dated version of IA2 interfaces.

Comment on attachment 396688 [details] [diff] [review] patch2 r=me with comments addressed; (and with Marco and Neil's addressed of course) Thanks dude.

Comment on attachment 396688 [details] [diff] [review] patch2 Found one thing: >+ "Wrong rows number of " + prettyName(aIdentifier)); >+ "Wrong columns number of " + prettyName(aIdentifier)); The wording is wrong. "Wrong number of rows" and "Wrong number of columns", or "Wrong rows count" and "Wrong columns count" would be better, whichever you prefer. The rest of the test part looks great!

David's and Marco's comments are fixed

Neil, friendly ping.

Hey, it's a big patch! Rome wasn't built in a day :-P

Yeah, I know. His size (not Rome's one :) of course ) makes me sad.

up to dated to trunk

Comment on attachment 399683 [details] [diff] [review] patch4 I'd prefer if you used columnCount/rowCount (i.e. without an s). [I'd also prefer selectedRow/Column/CellCount, and deselectRow/Column, but you weren't renaming those.] I noticed that nsXULTreeGridAccessible::GetSelected* methods are very inefficent, please file a followup bug on converting them to use selection->GetRangeAt.

(In reply to comment #34) > (From update of attachment 399683 [details] [diff] [review]) > I'd prefer if you used columnCount/rowCount (i.e. without an s). [I'd also > prefer selectedRow/Column/CellCount, and deselectRow/Column, but you weren't > renaming those.] > I noticed that nsXULTreeGridAccessible::GetSelected* methods are very > inefficent, please file a followup bug on converting them to use > selection->GetRangeAt. Ok. Thank you!

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/37b976412c04 with Neil's comments (excepting unselect -> deselect to keep our API closer to IA2 one's)

mochitests fix - http://hg.mozilla.org/mozilla-central/rev/0e157c793c5c

Comment on attachment 399683 [details] [diff] [review] patch4 Needed for completing a11y support of tables of all sorts for 3.6. Needed by IBM and Freedom Scientific, among others. See https://wiki.mozilla.org/Accessibility/Remaining_Mozilla-1.9.2_Nominations for more info.

Comment on attachment 399683 [details] [diff] [review] patch4 a192=beltzner

landed on 1.9.2 - http://hg.mozilla.org/releases/mozilla-1.9.2/rev/154ccb49f1e6 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9b4df7a2fde7

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)

Looking at this, I'm not sure whether this is really a documentable change or just an implementation detail. Can someone enlighten me?

(In reply to comment #42) > Looking at this, I'm not sure whether this is really a documentable change or > just an implementation detail. Can someone enlighten me? nsIAccessibleTable interface has been changed and new nsIAccessibleTableCell interface has been added. As well 'table-cell-index' object attribute on the cell accessible is no longer useful, it can be marked as deprecated.

We have a reference for this interface now: https://developer.mozilla.org/en/XPCOM_Interface_Reference/IAccessibleTable2

(In reply to comment #44) > We have a reference for this interface now: > > https://developer.mozilla.org/en/XPCOM_Interface_Reference/IAccessibleTable2 Well, it's not XPCOM interface. Perhaps it shouldn't be under XPCOM reference.