1305402 - Implement AtkTableCell interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

A while back, we added a TableCell interface to ATK and AT-SPI2. ATK docs here:
https://developer.gnome.org/atk/unstable/AtkTableCell.html

It was largely based on the IA2 table cell interface, so hopefully it won't be too much effort to expose the same information via ATK.

Thanks!

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: import atktablecell.h

This is a somewhat new header upstream that we need to import to support the
 AtkTableCell interface.  The one tricky bit is that upstream has started using
 AVAILABLE_IN_ATK_X_Y macros defined in a generated header.  To avoid needing
 to generate that header on every build or checking in generated code we just
 define the macros manually in atk.h.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: add new IPC messages needed for AtkTableCell interface

To implement the interface with 1 IPC message per method we need these new
messages.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: implement the AtkTablecell interface

This is a pretty straight forward wrapper around the internal table cell
 interface.  However we need to be careful to only expose this interface if we
 are dealing with a version of ATK new enough to understand it.

Comment 4

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

Comment on attachment 8795735 [details] [diff] [review]
implement the AtkTablecell interface

Review of attachment 8795735 [details] [diff] [review]:
-----------------------------------------------------------------

Looks okay to me, with comments:

::: accessible/atk/AccessibleWrap.cpp
@@ +66,5 @@
>      MAI_INTERFACE_TABLE,
>      MAI_INTERFACE_TEXT,
>      MAI_INTERFACE_DOCUMENT, 
> +    MAI_INTERFACE_IMAGE, /* 10 */
> +    MAI_INTERFACE_TABLE_CELL

Why do we we need to have that 10 comment? (And I guess it should be 11 now)

@@ +95,5 @@
>        return ATK_TYPE_DOCUMENT;
>      case MAI_INTERFACE_IMAGE:
>        return ATK_TYPE_IMAGE;
> +    case MAI_INTERFACE_TABLE_CELL:
> +      MOZ_ASSERT(false);

Maybe you should add a comment to the atk_if_infos definition saying "MAI_INTERFACE_TABLE_CELL intentionally left out" or something.

@@ +427,5 @@
>                                      &atk_if_infos[index]);
>        }
>      }
>  
> +    if (IsAtkVersionAtLeast(2, 12) && (interfacesBits & (1 << MAI_INTERFACE_TABLE_CELL))) {

Maybe add a comment before this check to say why we need to special case this interface?

::: accessible/atk/nsMaiInterfaceTableCell.cpp
@@ +67,5 @@
> +    *aRow = rowIdx;
> +    return true;
> +  }
> +
> +  return false;

Probably should keep consistent style. You return 0 in previous functions. Your choice.

@@ +78,5 @@
> +    TableCellAccessible* cellAcc = accWrap->AsTableCell();
> +    *aCol = cellAcc->ColIdx();
> +    *aRow = cellAcc->RowIdx();
> +    *aColExtent = cellAcc->ColExtent();
> +    *aRowExtent = cellAcc->ColExtent();return true;

nit: newline for return.

@@ +198,5 @@
> +}
> +}
> +
> +void
> +tableCellInterfaceInitCB(AtkTableCellIface* aIface)

Did you mean to have this outside extern "C" block?

Comment 5

[Trevor Saunders (:tbsaunde)]

> ::: accessible/atk/AccessibleWrap.cpp
> @@ +66,5 @@
> >      MAI_INTERFACE_TABLE,
> >      MAI_INTERFACE_TEXT,
> >      MAI_INTERFACE_DOCUMENT, 
> > +    MAI_INTERFACE_IMAGE, /* 10 */
> > +    MAI_INTERFACE_TABLE_CELL
> 
> Why do we we need to have that 10 comment? (And I guess it should be 11 now)

not sure, I just added the ',' and left it.  It does seem kind of useless.

> ::: accessible/atk/nsMaiInterfaceTableCell.cpp
> @@ +67,5 @@
> > +    *aRow = rowIdx;
> > +    return true;
> > +  }
> > +
> > +  return false;
> 
> Probably should keep consistent style. You return 0 in previous functions.
> Your choice.

funny story, that functions return type is supposed to be gboolean not gint, but somehow it compiles anyway.  I suspect gboolean is just a typedef of int :(

> > +}
> > +}
> > +
> > +void
> > +tableCellInterfaceInitCB(AtkTableCellIface* aIface)
> 
> Did you mean to have this outside extern "C" block?

yes, it doesn't need to have C linkage just like all the other interface init functions.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b044f5da3670
import atktablecell.h r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/28272f824ad0
add new IPC messages needed for AtkTableCell interface r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3152577aab9b
implement the AtkTablecell interface r=davidb

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d604653b1cfa8cead02fd81c50b951caa80d45
https://hg.mozilla.org/integration/mozilla-inbound/rev/7afc2dc9016908e58ebd9fa942ef14426230a005
https://hg.mozilla.org/integration/mozilla-inbound/rev/34bc558363a1b446d77cd945bfa1a8da285739bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0f175618d350058a2547fd1a3023dbe826eb3d (the previous follow-up fix)

Last push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=27509f16a0f2a011998b4dff8bba1fff6b48240a

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: dynamically lookup the atk_table_cell_get_type function

We need to deal with versions of the atk library that don't provide this
function, so we need to dynamically look it up instead of statically refering
to it.

Comment 9

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

Comment on attachment 8801691 [details] [diff] [review]
dynamically lookup the atk_table_cell_get_type function

Review of attachment 8801691 [details] [diff] [review]:
-----------------------------------------------------------------

r=me (with the tweak to previous patch)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5938913dcd
import atktablecell.h r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcabd365e0de
add new IPC messages needed for AtkTableCell interface r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5f6ceea72b
dynamically lookup the atk_table_cell_get_type function r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e25969288c
implement the AtkTablecell interface r=davidb

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c5938913dcd
https://hg.mozilla.org/mozilla-central/rev/bcabd365e0de
https://hg.mozilla.org/mozilla-central/rev/4e5f6ceea72b
https://hg.mozilla.org/mozilla-central/rev/25e25969288c