512424 - implement IAccessibleTable2

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

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

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

enjoy, friends ;)

Comment 3

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

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

Comment 4

[Marco Zehe (:MarcoZ)]

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?

Comment 5

[alexander :surkov (:asurkov)]

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

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

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

Comment 7

[alexander :surkov (:asurkov)]

https://build.mozilla.org/tryserver-builds/[e-mail]-try-56c9f07f1d93/

Comment 8

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

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

Comment 9

[alexander :surkov (:asurkov)]

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

Comment 10

[alexander :surkov (:asurkov)]

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

[Marco Zehe (:MarcoZ)]

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.

Comment 12

[alexander :surkov (:asurkov)]

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

Comment 13

[Marco Zehe (:MarcoZ)]

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

Comment 14

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

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 15

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

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]

Comment 16

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

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

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

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

[left off here: nsHTMLTableAccessible::GetColumnExtentAt]

Comment 18

[alexander :surkov (:asurkov)]

(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

Comment 19

[alexander :surkov (:asurkov)]

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

Comment 20

[alexander :surkov (:asurkov)]

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

Comment 21

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

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

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

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 ([e-mail])
>- *   Kyle Yuan ([e-mail])
>+ *   Aaron Leventhal <[e-mail]> (original author)
>+ *   Kyle Yuan <[e-mail]>
>+ *   Alexander Surkov <[e-mail]>

Oops, what about "Eric Vaughan" ?

[left off here: nsXULListboxAccessible::GetSelectedCells]

Comment 23

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

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 :)

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> >  * Contributor(s):
> >- *   Original Author: Eric Vaughan ([e-mail])
> >- *   Kyle Yuan ([e-mail])
> >+ *   Aaron Leventhal <[e-mail]> (original author)
> >+ *   Kyle Yuan <[e-mail]>
> >+ *   Alexander Surkov <[e-mail]>
> 
> 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.

Comment 25

[alexander :surkov (:asurkov)]

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

Comment 26

[alexander :surkov (:asurkov)]

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

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

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 28

[Marco Zehe (:MarcoZ)]

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!

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch3

David's and Marco's comments are fixed

Comment 30

[alexander :surkov (:asurkov)]

Neil, friendly ping.

Comment 31

[[e-mail]]

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

Comment 32

[alexander :surkov (:asurkov)]

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

Comment 33

[alexander :surkov (:asurkov)]

Attachment: patch4

up to dated to trunk

Comment 34

[[e-mail]]

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.

Comment 35

[alexander :surkov (:asurkov)]

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

Comment 36

[alexander :surkov (:asurkov)]

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)

Comment 37

[alexander :surkov (:asurkov)]

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

Comment 38

[Marco Zehe (:MarcoZ)]

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 39

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

Comment on attachment 399683 [details] [diff] [review]
patch4

a192=beltzner

Comment 40

[alexander :surkov (:asurkov)]

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

Comment 41

[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)

Comment 42

[Eric Shepherd [:sheppy]]

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

Comment 43

[alexander :surkov (:asurkov)]

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

Comment 44

[Eric Shepherd [:sheppy]]

We have a reference for this interface now:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/IAccessibleTable2

Comment 45

[alexander :surkov (:asurkov)]

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