493552 - implement nsIAccessibleTable selection methods for ARIA grids

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wip (not tested)

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

forgot to add test file

Comment 4

[[e-mail]]

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

>+  nsCOMPtr<nsIAccessible> cell;
>+  CellRefAt(aRow, aColumn, getter_AddRefs(cell));
I don't like this a) because it's a virtual method but b) because you might need the row again anyway.

>+    if (nsAccUtils::Role(row) != nsIAccessibleRole::ROLE_ROW)
>+      return NS_OK; // Wrong markup
And then this will never happen.

>+  *aCellsCount = selCells.Length();
>+  *aCells = (PRInt32 *)nsMemory::Alloc((*aCellsCount) * sizeof(PRInt32));
>+  NS_ENSURE_TRUE(*aCells, NS_ERROR_OUT_OF_MEMORY);
Nit: shouldn't write *aCellsCount until after the allocation succeeds.

>+  for (PRUint32 i = 0; i < *aCellsCount; i++)
>+    (*aCells)[i] = selCells[i];
You can use nsMemory::Clone in places like this to avoid the manual copy.

> NS_IMETHODIMP
> nsARIAGridAccessible::GetSelectedColumns(PRUint32 *aColumnsCount,
>                                          PRInt32 **aColumns)
> {
>   NS_ENSURE_ARG_POINTER(aColumnsCount);
>   *aColumnsCount = 0;
>   NS_ENSURE_ARG_POINTER(aColumns);
>   *aColumns = nsnull;
GetSelectedColumns and GetSelectedColumnCount are very similar methods.
Two suggestions (which may apply to rows and cells, I haven't checked):
1. Remove the check for *aColumns, and only fill *aColumns if it is not null
2. As 1, but rename this, and implement GetSelectedColumns that null-checks
Then, implement GetSelectedColumnCount by calling GetSelectedColumns[Helper]

>+  PRInt32 rowIdx = aRow + 1;
>+
>+  nsCOMPtr<nsIAccessible> row;
Rather than adding one (and again for cells) I think I'd prefer
nsCOMPTr<nsIAccessible> row = GetNextRow();

>+  NS_IF_ADDREF(row);
Bah, NS_IF_ADDREF used to be prohibited on nsCOMPtr but nsDerivedSafe got removed :-(

>+  return row.get();
But I think you should be using forget() instead anyway (same for cell).

>+  nsIAccessible *nextRow = nsnull;
>+  if (!aRow)
>+    GetFirstChild(&nextRow);
>+  else
>+    aRow->GetNextSibling(&nextRow);
>+
>+  nsCOMPtr<nsIAccessible> tmpAcc;
>+  while (nextRow) {
>+    if (nsAccUtils::Role(nextRow) == nsIAccessibleRole::ROLE_ROW)
>+      return nextRow;
>+
>+    nextRow->GetNextSibling(getter_AddRefs(tmpAcc));
>+    tmpAcc.swap(nextRow);
>+  }
It took me some time to work out that this actually refcounts correctly...

>+    nsCOMPtr<nsIContent> content(do_QueryInterface(node));
Can we be sure this succeeds (i.e. some invariant that I'm not aware of)

>+  if (nsAccUtils::Role(aAccessible) == nsIAccessibleRole::ROLE_ROW) {
Why bother doing this if the row is being selected?

>+  PRUint32 role = nsAccUtils::Role(aAccessible);
Nit: move this variable up and reuse it.

Comment 5

[Marco Zehe (:MarcoZ)]

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

r=me with one nit:
>+  // getSelectedColumns test
>+  var actualSelRowsCountObj = { value: null };

In the comment should be "GetSelectedRows...".

Comment 6

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

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

r=me with neil's comments addressed (I'd like to see the next patch). Also a question below:

> nsARIAGridAccessible::GetColumns(PRInt32 *aColumns)
> {
>   NS_ENSURE_ARG_POINTER(aColumns);
>   *aColumns = 0;
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>+  nsCOMPtr<nsIAccessible> row = GetNextRow();
>   nsCOMPtr<nsIAccessible> cell;
>+  while ((cell = GetNextCellInRow(row, cell)))
>+    (*aColumns)++;

I didn't catch this before, but is it really sufficient to return the number of columns of the first row? Do all rows have the same number of cols?

Comment 7

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

Note I removed the old lines from the patch of GetColumns in my last comment.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 378589 [details] [diff] [review])
> r=me with neil's comments addressed (I'd like to see the next patch).

it will be.

> I didn't catch this before, but is it really sufficient to return the number of
> columns of the first row? Do all rows have the same number of cols?

That's we assume. ARIA hasn't a way to specify row/colspan so we can assume table has equal numbers of rows in each column and columns in each row. If this rule is broken then our table interface is broken as well. I think it's ok for the first approach. In future we can develop new rules to fix markup bad.

Comment 9

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

(In reply to comment #8)
> (In reply to comment #6)
> > I didn't catch this before, but is it really sufficient to return the number of
> > columns of the first row? Do all rows have the same number of cols?
> 
> That's we assume. ARIA hasn't a way to specify row/colspan so we can assume
> table has equal numbers of rows in each column and columns in each row. If this
> rule is broken then our table interface is broken as well. I think it's ok for
> the first approach. In future we can develop new rules to fix markup bad.

OK, hopefully if we hit inconsistent numbers of gridcells per row or col we fail gracefully, where gracefully == not crashing :)  I *think* we are ok, but might be worth testing.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #4)

Neil, thank you for review! That's is the case when superreview is necessary :)

> >+  CellRefAt(aRow, aColumn, getter_AddRefs(cell));
> I don't like this a) because it's a virtual method but b) because you might
> need the row again anyway.

fixed

> >+  *aCellsCount = selCells.Length();
> >+  *aCells = (PRInt32 *)nsMemory::Alloc((*aCellsCount) * sizeof(PRInt32));
> >+  NS_ENSURE_TRUE(*aCells, NS_ERROR_OUT_OF_MEMORY);
> Nit: shouldn't write *aCellsCount until after the allocation succeeds.
> 
> >+  for (PRUint32 i = 0; i < *aCellsCount; i++)
> >+    (*aCells)[i] = selCells[i];
> You can use nsMemory::Clone in places like this to avoid the manual copy.

fixed

> GetSelectedColumns and GetSelectedColumnCount are very similar methods.
> Two suggestions (which may apply to rows and cells, I haven't checked):
> 1. Remove the check for *aColumns, and only fill *aColumns if it is not null
> 2. As 1, but rename this, and implement GetSelectedColumns that null-checks
> Then, implement GetSelectedColumnCount by calling GetSelectedColumns[Helper]

I chose 2.

> >+  PRInt32 rowIdx = aRow + 1;
> >+
> >+  nsCOMPtr<nsIAccessible> row;
> Rather than adding one (and again for cells) I think I'd prefer
> nsCOMPTr<nsIAccessible> row = GetNextRow();

done

> >+  return row.get();
> But I think you should be using forget() instead anyway (same for cell).

done

> It took me some time to work out that this actually refcounts correctly...

me either, is there idea how to improve this?

> 
> >+    nsCOMPtr<nsIContent> content(do_QueryInterface(node));
> Can we be sure this succeeds (i.e. some invariant that I'm not aware of)

It won't be called on document node, so I can be sure, right?

> >+  if (nsAccUtils::Role(aAccessible) == nsIAccessibleRole::ROLE_ROW) {
> Why bother doing this if the row is being selected?

To be consistent i.e. cells and rows provides the same information. Afaik the algorithm is not defined yet by w3c, so I think it's good to have.

> 
> >+  PRUint32 role = nsAccUtils::Role(aAccessible);
> Nit: move this variable up and reuse it.

done


New patch is coming up.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (In reply to comment #8)

> OK, hopefully if we hit inconsistent numbers of gridcells per row or col we
> fail gracefully, where gracefully == not crashing :)  I *think* we are ok, but
> might be worth testing.

I tried to protect against this. We definitely need mochitests for this. So if you insist then I'll add them :)

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3

with Neil's and Marco's comments.

Comment 13

[[e-mail]]

Comment on attachment 379804 [details] [diff] [review]
patch3

>+  nsIAccessible *nextRow = nsnull;
>+  if (!aRow)
>+    GetFirstChild(&nextRow);
>+  else
>+    aRow->GetNextSibling(&nextRow);
>+
>+  nsCOMPtr<nsIAccessible> tmpAcc;
>+  while (nextRow) {
>+    if (nsAccUtils::Role(nextRow) == nsIAccessibleRole::ROLE_ROW)
>+      return nextRow;
>+
>+    nextRow->GetNextSibling(getter_AddRefs(tmpAcc));
>+    tmpAcc.swap(nextRow);
>+  }
(In reply to comment #10)
>(In reply to comment #4)
>> It took me some time to work out that this actually refcounts correctly...
>me either, is there idea how to improve this?
To make the refcounting more obviously correct,
a) change nsIAccessible *nextRow = nsnull; to nsCOMPtr<nsIAccessible> nextRow;
b) change &nextRow to getter_AddRefs(nextRow);
c) change return nextRow; to return nextRow.forget();
None of these changes affects the refcount so the original code was correct.

>+  // If the given accessible is row then remove aria-selected from cell
>+  // accessible.
(In reply to comment #10)
>(In reply to comment #4)
>>Why bother doing this if the row is being selected?
>To be consistent i.e. cells and rows provides the same information. Afaik the
>algorithm is not defined yet by w3c, so I think it's good to have.
I obviously understand that when you deselect a row you also have to deselect all of its cells. However I'm not convinced of the utility of deselecting the cells when you select a row. (There is also a case for selecting the cells as well as simply leaving their current state.)

I'll take your word for it that the method will be called on the right node.

Comment 14

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

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> 
> > OK, hopefully if we hit inconsistent numbers of gridcells per row or col we
> > fail gracefully, where gracefully == not crashing :)  I *think* we are ok, but
> > might be worth testing.
> 
> I tried to protect against this. We definitely need mochitests for this. So if
> you insist then I'll add them :)

Sure, here in in a follow up is fine :)

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> (In reply to comment #10)
> >(In reply to comment #4)
> >>Why bother doing this if the row is being selected?
> >To be consistent i.e. cells and rows provides the same information. Afaik the
> >algorithm is not defined yet by w3c, so I think it's good to have.
> I obviously understand that when you deselect a row you also have to deselect
> all of its cells. However I'm not convinced of the utility of deselecting the
> cells when you select a row. (There is also a case for selecting the cells as
> well as simply leaving their current state.)
> 

Yes, we have three cases:
1. leave them as is
2. set aria-selected="true"
3. remove aria-selected

Possibly the more right way is set aria-selected="true" so that cells and rows provides the same information. Leave them as is also should work. And the third case won't hit us. I think either case will work for us. But I don't insist to keep behaviour I originally provided. What option do you like more, second one?

Comment 16

[alexander :surkov (:asurkov)]

Neil, your opinion for comment #15 please?

Comment 17

[[e-mail]]

Sorry I forgot to answer, but the suggestions are already in preferred order!

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch4

Neil comment fixed

Comment 19

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/9ae23eb019fb

Comment 20

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre (.NET CLR 3.5.30729)