787308 - convert table cell accessible implementations to new interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 657141 [details] [diff] [review]
patch

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

thank you for nit clenaning up

::: accessible/src/generic/TableAccessible.h
@@ +176,5 @@
>     */
>    virtual bool IsProbablyLayoutTable() { return false; }
> +
> +  /**
> +   * Return this as an Accessible*.

maybe: Convert the table to Accessible*

::: accessible/src/generic/TableCellAccessible.cpp
@@ +15,5 @@
> +void
> +TableCellAccessible::RowHeaderCells(nsTArray<Accessible*>* aCells)
> +{
> +  uint32_t rowIdx = RowIdx(), colIdx = ColIdx();
> +  TableAccessible* table = Table();

no null check? (same below) iirc not every table guarantees the correct structure

@@ +23,5 @@
> +    Accessible* cell = table->CellAt(rowIdx, curColIdx);
> +    if (!cell)
> +      continue;
> +
> +    // GetCellAt should always return an nsIAccessibleTableCell (XXX Bug 587529)

update the comment please (and you might want to comment into mentioned bug) (and below)

@@ +32,5 @@
> +
> +    // Avoid addding cells multiple times, if this cells spans more columns
> +    // we'll get it later.
> +    if (tableCell->ColIdx() == curColIdx && cell->Role() == roles::ROWHEADER)
> +      aCells->AppendElement(cell);

it's not optimal because some implemntation of ColIdx() traverses to the start, file a follow up?

@@ +54,5 @@
> +    NS_ASSERTION(tableCell, "cell should be a table cell!");
> +    if (!tableCell)
> +      continue;
> +
> +    // Avoid addding cells multiple times, if this cells spans more columns

more rows I guess

::: accessible/src/generic/TableCellAccessible.h
@@ +31,3 @@
>  
>    /**
> +   * Return the column of the table this cell is in.

nit: column index

@@ +36,5 @@
>  
>    /**
>     * Return the the row of the table this cell is in.
>     */
> +  virtual uint32_t RowIdx() const = 0;

the the row -> the row index

@@ +61,5 @@
>  
>    /**
>     * Returns true if this cell is selected.
>     */
> +  virtual bool Selected() = 0;

is it nicer if we had IsSelected()? name conflicts? if so then it might be nice to comment

::: accessible/src/html/HTMLTableAccessible.cpp
@@ +153,4 @@
>    while ((parent = parent->Parent())) {
>      roles::Role role = parent->Role();
> +    if (role == roles::TABLE || role == roles::TREE_TABLE)
> +      return parent->AsTable();

AsTable in cycle should be faster I think (we don't respect the table structure but iirc we take care about this at tree creation stage)

@@ +174,5 @@
> +HTMLTableCellAccessible::RowIdx() const
> +{
> +  nsITableCellLayout* cellLayout = GetCellLayout();
> +  NS_ENSURE_TRUE(cellLayout, 0);
> + 

nit: extra space

::: accessible/src/html/HTMLTableAccessible.h
@@ +38,3 @@
>  
>    // Accessible
> +virtual TableCellAccessible* AsTableCell() { return this; }

nit: wrong indentation

@@ +134,5 @@
>    virtual void SelectRow(uint32_t aRowIdx);
>    virtual void UnselectCol(uint32_t aColIdx);
>    virtual void UnselectRow(uint32_t aRowIdx);
>    virtual bool IsProbablyLayoutTable();
> +  virtual Accessible* AsAccessible() { return this; }

it should inherit it from HTMLTableCellAccessible

::: accessible/src/xpcom/xpcAccessibleTableCell.cpp
@@ +60,5 @@
> +  if (mTableCell)
> +    return NS_ERROR_FAILURE;
> +
> +  *aRowIdx = mTableCell->RowIdx();
> +  

nit: extra space

@@ +101,5 @@
> +
> +  if (!mTableCell)
> +    return NS_ERROR_FAILURE;
> +
> +  nsAutoTArray<Accessible*, 10> headerCells;

10 doesn't seem be a typical headers number, how did you choose it?

@@ +110,5 @@
> +
> +  for (uint32_t idx = 0; idx < headerCells.Length(); idx++)
> +    cells->
> +      AppendElement(static_cast<nsIAccessible*>(headerCells.ElementAt(idx)),
> +                    false);

should be nice to use { } about this if since it takes several lines

::: accessible/src/xpcom/xpcAccessibleTableCell.h
@@ +14,3 @@
>  
>  namespace mozilla {
>  namespace a11y {

nit: new line between pls

@@ +19,2 @@
>  
>  class xpcAccessibleTableCell

a short comment while you are here please?

::: accessible/src/xul/XULListboxAccessible.cpp
@@ +748,3 @@
>    Accessible* thisRow = Parent();
>    if (!thisRow || thisRow->Role() != roles::ROW)
> +    return nullptr;

makes sense to move inline ARIAGridCellAccessible::Row into TableCellAccessible

@@ +804,5 @@
>    nsCOMPtr<nsIAccessibleTable> table;
>    GetTable(getter_AddRefs(table));
> +  NS_ASSERTION(table, "cell not in a table!");
> +  if (!table)
> +    return;

use Table() and then fix query below

::: accessible/src/xul/XULListboxAccessible.h
@@ +164,5 @@
>    // nsISupports
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    // nsIAccessibleTableCell
> +    NS_FORWARD_NSIACCESSIBLETABLECELL(xpcAccessibleTableCell::)

nit: fix indent

::: accessible/src/xul/XULTreeGridAccessible.cpp
@@ +669,2 @@
>  {
> +  return GetColumnIndex();

having two methods for the same things looks strange

@@ +687,2 @@
>    if (headerCell)
> +    aHeaderCells->AppendElement(headerCell);

does it make sense to clean array before putting anything there

Comment 3

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/generic/TableCellAccessible.cpp
> @@ +15,5 @@
> > +void
> > +TableCellAccessible::RowHeaderCells(nsTArray<Accessible*>* aCells)
> > +{
> > +  uint32_t rowIdx = RowIdx(), colIdx = ColIdx();
> > +  TableAccessible* table = Table();
> 
> no null check? (same below) iirc not every table guarantees the correct
> structure

yeah :(

> > +
> > +    // Avoid addding cells multiple times, if this cells spans more columns
> > +    // we'll get it later.
> > +    if (tableCell->ColIdx() == curColIdx && cell->Role() == roles::ROWHEADER)
> > +      aCells->AppendElement(cell);
> 
> it's not optimal because some implemntation of ColIdx() traverses to the
> start, file a follow up?

what is the better algorithm you can see?

> ::: accessible/src/generic/TableCellAccessible.h
> @@ +31,3 @@
> >  
> >    /**
> > +   * Return the column of the table this cell is in.
> 
> nit: column index
> 
> @@ +36,5 @@
> >  
> >    /**
> >     * Return the the row of the table this cell is in.
> >     */
> > +  virtual uint32_t RowIdx() const = 0;
> 
> the the row -> the row index

The row of the table this table cell is in feels funny to me.

> 
> >  
> >    /**
> >     * Returns true if this cell is selected.
> >     */
> > +  virtual bool Selected() = 0;
> 
> is it nicer if we had IsSelected()? name conflicts? if so then it might be
> nice to comment

personally I think I prefer thing->Selected() to thing->IsSelected()

We could do IsSelected() but it would require binary name stuffs for nsIAccessibleTable::IsSelected()

> 
> @@ +153,4 @@
> >    while ((parent = parent->Parent())) {
> >      roles::Role role = parent->Role();
> > +    if (role == roles::TABLE || role == roles::TREE_TABLE)
> > +      return parent->AsTable();
> 
> AsTable in cycle should be faster I think (we don't respect the table
> structure but iirc we take care about this at tree creation stage)

what about html tables with funny roles like <table role=menu> ?

> @@ +134,5 @@
> >    virtual void SelectRow(uint32_t aRowIdx);
> >    virtual void UnselectCol(uint32_t aColIdx);
> >    virtual void UnselectRow(uint32_t aRowIdx);
> >    virtual bool IsProbablyLayoutTable();
> > +  virtual Accessible* AsAccessible() { return this; }
> 
> it should inherit it from HTMLTableCellAccessible

I'm not sure I understand, its a method of TableAccessible, so what does a cell class have to do with it?

> > +
> > +  if (!mTableCell)
> > +    return NS_ERROR_FAILURE;
> > +
> > +  nsAutoTArray<Accessible*, 10> headerCells;
> 
> 10 doesn't seem be a typical headers number, how did you choose it?

basically just pulled something that seemed reasonable out of air.  Its not too big that coppying will take long if the array gets bigger than it, and should in all reasonable cases hold all the headers.

> ::: accessible/src/xpcom/xpcAccessibleTableCell.h
> @@ +14,3 @@
> >  
> >  namespace mozilla {
> >  namespace a11y {
> 
> nit: new line between pls

why? normally we don't put a blank line between namespace foo { s

> ::: accessible/src/xul/XULListboxAccessible.cpp
> @@ +748,3 @@
> >    Accessible* thisRow = Parent();
> >    if (!thisRow || thisRow->Role() != roles::ROW)
> > +    return nullptr;
> 
> makes sense to move inline ARIAGridCellAccessible::Row into
> TableCellAccessible

you mean have a virtual Accessible*  TableCellAccessible::Row() method?

> Accessible.cpp
> @@ +669,2 @@
> >  {
> > +  return GetColumnIndex();
> 
> having two methods for the same things looks strange

true, but iirc the GetColumn() one is on a super class that isn't necessarily a table or some such.

> @@ +687,2 @@
> >    if (headerCell)
> > +    aHeaderCells->AppendElement(headerCell);
> 
> does it make sense to clean array before putting anything there

unless we have a use case, I'd say its better for internal things to just expect the array is in good shape.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > it's not optimal because some implemntation of ColIdx() traverses to the
> > start, file a follow up?
> 
> what is the better algorithm you can see?

walking by children, if crazy HTML tables can do crazy things then cell could provide methods like prev/nextCell which goes with other methods (like cell by index and index by cell).

> > the the row -> the row index
> 
> The row of the table this table cell is in feels funny to me.

maybe: The index of the table row this cell is in?

> > is it nicer if we had IsSelected()? name conflicts? if so then it might be
> > nice to comment
> 
> personally I think I prefer thing->Selected() to thing->IsSelected()

mm, ok but we need to figure out the style, previously it seems we used IsSomethinged() instead Somethinged().

> We could do IsSelected() but it would require binary name stuffs for
> nsIAccessibleTable::IsSelected()

shouldn't be ok since they take different arguments?


> > AsTable in cycle should be faster I think (we don't respect the table
> > structure but iirc we take care about this at tree creation stage)
> 
> what about html tables with funny roles like <table role=menu> ?

I didn't try but I guessed that accessibility service takes care about this: no table structure. Anyway, it's not your patch code then I don't mind if you skip it.

> > > +  virtual Accessible* AsAccessible() { return this; }
> > 
> > it should inherit it from HTMLTableCellAccessible
> 
> I'm not sure I understand, its a method of TableAccessible, so what does a
> cell class have to do with it?

splinter review confused me, it shown that it was a HTMLTableHeaderCellAccessible.

> > 10 doesn't seem be a typical headers number, how did you choose it?
> 
> basically just pulled something that seemed reasonable out of air.  Its not
> too big that coppying will take long if the array gets bigger than it, and
> should in all reasonable cases hold all the headers.

ok

> > nit: new line between pls
> 
> why? normally we don't put a blank line between namespace foo { s

checked several files and it seems we do

> > ::: accessible/src/xul/XULListboxAccessible.cpp
> > @@ +748,3 @@
> > >    Accessible* thisRow = Parent();
> > >    if (!thisRow || thisRow->Role() != roles::ROW)
> > > +    return nullptr;
> > 
> > makes sense to move inline ARIAGridCellAccessible::Row into
> > TableCellAccessible
> 
> you mean have a virtual Accessible*  TableCellAccessible::Row() method?

Yes but not virtual. can we do that?

> > > +  return GetColumnIndex();
> > 
> > having two methods for the same things looks strange
> 
> true, but iirc the GetColumn() one is on a super class that isn't
> necessarily a table or some such.

GetColumnIndex() is on XUL tree grid cell (XULTreeGridCellAccessible) which is a part of table.

> > @@ +687,2 @@
> > >    if (headerCell)
> > > +    aHeaderCells->AppendElement(headerCell);
> > 
> > does it make sense to clean array before putting anything there
> 
> unless we have a use case, I'd say its better for internal things to just
> expect the array is in good shape.

ok, maybe an assertion if it's not empty?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > > it's not optimal because some implemntation of ColIdx() traverses to the
> > > start, file a follow up?
> > 
> > what is the better algorithm you can see?
> 
> walking by children, if crazy HTML tables can do crazy things then cell
> could provide methods like prev/nextCell which goes with other methods (like
> cell by index and index by cell).

ok, that might work and make sense.

> > > the the row -> the row index
> > 
> > The row of the table this table cell is in feels funny to me.
> 
> maybe: The index of the table row this cell is in?

I guess that works, although personally I think just saying the row like I did makes it pretty clear what you get is the index of the row.

> > > is it nicer if we had IsSelected()? name conflicts? if so then it might be
> > > nice to comment
> > 
> > personally I think I prefer thing->Selected() to thing->IsSelected()
> 
> mm, ok but we need to figure out the style, previously it seems we used
> IsSomethinged() instead Somethinged().

hm, that a good point, but I'm not sure what to do about it.  IsFoo() is fine with me, but I do prefer just Foo() I think.

> > We could do IsSelected() but it would require binary name stuffs for
> > nsIAccessibleTable::IsSelected()
> 
> shouldn't be ok since they take different arguments?

if you have overloads of a virtual method and you want to override one of the over loads then you are supposed to override them all, or gcc gives you really big and anoying woverloaded-virtual warnings.

> 
> > > AsTable in cycle should be faster I think (we don't respect the table
> > > structure but iirc we take care about this at tree creation stage)
> > 
> > what about html tables with funny roles like <table role=menu> ?
> 
> I didn't try but I guessed that accessibility service takes care about this:
> no table structure. Anyway, it's not your patch code then I don't mind if
> you skip it.

ok seems something to consider for future then.

> > > nit: new line between pls
> > 
> > why? normally we don't put a blank line between namespace foo { s
> 
> checked several files and it seems we do

oh, did you mean it should be

namespace mozilla {
namespace a11y {

class TableAccessible
?

it looked like from the comment you wanted
namespace mozilla {

namespace a11y {

which I can't seem to find in any file, and just looks funny.

> > > ::: accessible/src/xul/XULListboxAccessible.cpp
> > > @@ +748,3 @@
> > > >    Accessible* thisRow = Parent();
> > > >    if (!thisRow || thisRow->Role() != roles::ROW)
> > > > +    return nullptr;
> > > 
> > > makes sense to move inline ARIAGridCellAccessible::Row into
> > > TableCellAccessible
> > 
> > you mean have a virtual Accessible*  TableCellAccessible::Row() method?
> 
> Yes but not virtual. can we do that?

maybe?  I *believe* for tall tables we have now a cells row is its direct parent.   Although it might be faster to allow treegrid cells to over ride since they keep a pointer to the row, or maybe in future all cells should do that I'm not really sure.

> 
> > > 
> > > having two methods for the same things looks strange
> > 
> > true, but iirc the GetColumn() one is on a super class that isn't
> > necessarily a table or some such.
> 
> GetColumnIndex() is on XUL tree grid cell (XULTreeGridCellAccessible) which
> is a part of table.

yeah, guess I missed where it was when I looked, I'll nuke GetColumnIndex() then.

> > > @@ +687,2 @@
> > > >    if (headerCell)
> > > > +    aHeaderCells->AppendElement(headerCell);
> > > 
> > > does it make sense to clean array before putting anything there
> > 
> > unless we have a use case, I'd say its better for internal things to just
> > expect the array is in good shape.
> 
> ok, maybe an assertion if it's not empty?

I guess that's fine, although I could believe there might be a caller intentionally wants to add its own things to the array first.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> > walking by children, if crazy HTML tables can do crazy things then cell
> > could provide methods like prev/nextCell which goes with other methods (like
> > cell by index and index by cell).
> 
> ok, that might work and make sense.

file a followup?

> > > > the the row -> the row index
> > > 
> > > The row of the table this table cell is in feels funny to me.
> > 
> > maybe: The index of the table row this cell is in?
> 
> I guess that works, although personally I think just saying the row like I
> did makes it pretty clear what you get is the index of the row.

I like having 'row index' rather than 'index'. Up to you.

> hm, that a good point, but I'm not sure what to do about it.  IsFoo() is
> fine with me, but I do prefer just Foo() I think.

I'm fine with either way but we was used 'Is' before.

> > > We could do IsSelected() but it would require binary name stuffs for
> > > nsIAccessibleTable::IsSelected()
> > 
> > shouldn't be ok since they take different arguments?
> 
> if you have overloads of a virtual method and you want to override one of
> the over loads then you are supposed to override them all, or gcc gives you
> really big and anoying woverloaded-virtual warnings.

iirc using keyword helps, and it's temporary until we split trees.

> oh, did you mean it should be
> 
> namespace mozilla {
> namespace a11y {
> 
> class TableAccessible
> ?

yes

> it looked like from the comment you wanted
> namespace mozilla {
> 
> namespace a11y {
> 
> which I can't seem to find in any file, and just looks funny.

ah, no of course, splinter review confused me I guess.

> > > you mean have a virtual Accessible*  TableCellAccessible::Row() method?
> > 
> > Yes but not virtual. can we do that?
> 
> maybe?  I *believe* for tall tables we have now a cells row is its direct
> parent.   Although it might be faster to allow treegrid cells to over ride
> since they keep a pointer to the row, or maybe in future all cells should do
> that I'm not really sure.

ok, we can deal with that later. row is always a parent, extra ref doesn't make big sense

> > > > does it make sense to clean array before putting anything there
> > > 
> > > unless we have a use case, I'd say its better for internal things to just
> > > expect the array is in good shape.
> > 
> > ok, maybe an assertion if it's not empty?
> 
> I guess that's fine, although I could believe there might be a caller
> intentionally wants to add its own things to the array first.

then we have bad naming :) and we should use Append instead Get.

Comment 7

[Trevor Saunders (:tbsaunde)]

https://tbpl.mozilla.org/?tree=Try&rev=02db7ecb0673  seems to be green for m-a11y so landed
https://hg.mozilla.org/integration/mozilla-inbound/rev/67a582c0daf7

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67a582c0daf7