Closed Bug 493552 Opened 11 years ago Closed 11 years ago

implement nsIAccessibleTable selection methods for ARIA grids

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch wip (not tested) (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #378379 - Attachment is obsolete: true
Attachment #378587 - Flags: superreview?(neil)
Attachment #378587 - Flags: review?(marco.zehe)
Attachment #378587 - Flags: review?(david.bolter)
Attached patch patch2 (obsolete) — Splinter Review
forgot to add test file
Attachment #378587 - Attachment is obsolete: true
Attachment #378589 - Flags: superreview?(neil)
Attachment #378589 - Flags: review?(marco.zehe)
Attachment #378589 - Flags: review?(david.bolter)
Attachment #378587 - Flags: superreview?(neil)
Attachment #378587 - Flags: review?(marco.zehe)
Attachment #378587 - Flags: review?(david.bolter)
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.
Attachment #378589 - Flags: superreview?(neil) → superreview+
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...".
Attachment #378589 - Flags: review?(marco.zehe) → review+
Attachment #378589 - Flags: review?(david.bolter) → review+
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?
Note I removed the old lines from the patch of GetColumns in my last comment.
(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.
(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.
(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.
(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 :)
Attached patch patch3 (obsolete) — Splinter Review
with Neil's and Marco's comments.
Attachment #378589 - Attachment is obsolete: true
Attachment #379804 - Flags: superreview?(neil)
Attachment #379804 - Flags: superreview?(neil) → superreview+
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.
(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 :)
(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?
Neil, your opinion for comment #15 please?
Sorry I forgot to answer, but the suggestions are already in preferred order!
Attached patch patch4Splinter Review
Neil comment fixed
Attachment #379804 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/9ae23eb019fb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 498277
No longer depends on: 498277
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.