Closed Bug 1305402 Opened 8 years ago Closed 8 years ago

Implement AtkTableCell interface

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jdiggs, Assigned: tbsaunde)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

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!
Blocks: tablea11y
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.
Attachment #8795732 - Flags: review?(dbolter)
To implement the interface with 1 IPC message per method we need these new
messages.
Attachment #8795734 - Flags: review?(dbolter)
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.
Attachment #8795735 - Flags: review?(dbolter)
Assignee: nobody → tbsaunde+mozbugs
Attachment #8795732 - Flags: review?(dbolter) → review+
Attachment #8795734 - Flags: review?(dbolter) → review+
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?
Attachment #8795735 - Flags: review?(dbolter) → review+
> ::: 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.
Pushed by tsaunders@mozilla.com:
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
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.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8801691 - Flags: review?(dbolter)
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)
Attachment #8801691 - Flags: review?(dbolter) → review+
Pushed by tsaunders@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.