Note: There are a few cases of duplicates in user autocompletion which are being worked on.

decomtaminate GetCellAt() on accessible tables

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: capella)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
same as bug 739882, bug 739883, and bug 739885
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 624143 [details] [diff] [review]
Patch (v1)

Asking mentor for initial feedback ... this builds and passes all mochitest-a11y's locally ... let me know what you find, and I'll fix it -- mark
Attachment #624143 - Flags: feedback?(trev.saunders)

Comment 2

5 years ago
Comment on attachment 624143 [details] [diff] [review]
Patch (v1)

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

stealing feedback request from Trevor, asking him for review

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +882,3 @@
>    nsCOMPtr<nsIDOMElement> cellElement;
> +  GetCellAt(aRowIndex, aColumnIndex, *getter_AddRefs(cellElement));
> +  NS_ENSURE_TRUE(cellElement, nsnull);

no warning, just return null

@@ +886,5 @@
>    nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
> +  if (!cellContent)
> +    return nsnull;
> +
> +  NS_ENSURE_TRUE(mDoc, nsnull);

it shouldn't be null, so do not add it

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +60,5 @@
> +  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
> +      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
> +    return NS_ERROR_INVALID_ARG;
> +
> +  NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIndex, aColumnIndex));

I think I'd check *aCell instead of row and column indices and it's null then return invalid_arg

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +289,4 @@
>  
>    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
> +  control->GetItemAtIndex(aRowIndex, getter_AddRefs(item));
> +  NS_ENSURE_TRUE(item, nsnull);

no warning pls

@@ +297,2 @@
>  
> +  NS_ENSURE_TRUE(mDoc, nsnull);

no check

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +333,5 @@
>  
> +nsAccessible*
> +nsXULTreeGridAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex)
> +{ 
> +  nsAccessible* rowAccessible = GetTreeItemAccessible(aRowIndex);

rowAccessible -> row please

@@ +345,4 @@
>  
>    nsRefPtr<nsXULTreeItemAccessibleBase> rowAcc = do_QueryObject(rowAccessible);
> +  if (!rowAcc)
> +    return nsnull;

would you mind to file a bug to add a downcasting to nsXULTreeItemAccessibleBase (or alternatively to make GetTreeItemAccessible to return nsXULTreeItemAccessibleBase*)
Attachment #624143 - Flags: review?(trev.saunders)
Attachment #624143 - Flags: feedback?(trev.saunders)
Attachment #624143 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
FYI, I had to leave the row and column indices check in the xpcAccessibleTable.cpp member function GetCellAt(), that I'd copied from the existing GetCellIndexAt() member. 

Without the check, we fail getCellAt() at line number 79 with NS_ERROR_ILLEGAL_VALUE ...
http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_table_2.html?force=1#77

Comment 4

5 years ago
(In reply to Mark Capella [:capella] from comment #3)

> Without the check, we fail getCellAt() at line number 79 with
> NS_ERROR_ILLEGAL_VALUE ...
> http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/
> table/test_table_2.html?force=1#77

I'm curious what returns this value?
(Assignee)

Comment 5

5 years ago
I had changed all the nits above as requested, and found that one item to fail out. Putting it back causes the test to pass. 

Now there is an additional issue where test_general.html hangs ... I'm putting things back one at a time and testing to see what's going on.

Comment 6

5 years ago
(In reply to Mark Capella [:capella] from comment #5)
> I had changed all the nits above as requested, and found that one item to
> fail out. Putting it back causes the test to pass. 

Yes, I get this but I don't understand the cause. I didn't see where you can return this error value.

> Now there is an additional issue where test_general.html hangs ... I'm
> putting things back one at a time and testing to see what's going on.

it shouldn't be related, btw, which test_general.html?
(Assignee)

Comment 7

5 years ago
My mistake, the hanging test is treeupdate/test_imagemap.html ... if I pull all my patches out of the queue and re-build and re-test, the problem is still there so my patch can't be responsible.

fwiw, if while the test is hanging, I click on a background window (shift the focus?) the test then continues, and runs to completion successfully.
(Assignee)

Comment 8

5 years ago
Created attachment 624336 [details] [diff] [review]
Patch (v2)

Next version of the patch with nits addressed, asking tbsaunde for review?.

FYI, a fresh pull, and a non-debug / clobber build resolved all test issues :)
Attachment #624143 - Attachment is obsolete: true
Attachment #624143 - Flags: review?(trev.saunders)
Attachment #624336 - Flags: review?(trev.saunders)
(Reporter)

Comment 9

5 years ago
Comment on attachment 624336 [details] [diff] [review]
Patch (v2)

>+nsHTMLTableAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex)
>+{ 
>   nsCOMPtr<nsIDOMElement> cellElement;
>-  nsresult rv = GetCellAt(aRow, aColumn, *getter_AddRefs(cellElement));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  GetCellAt(aRowIndex, aColumnIndex, *getter_AddRefs(cellElement));
>+  if (!cellElement)
>+    return nsnull;
> 
>   nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
>+  if (!cellContent)
>+    return nsnull;
>+
>   nsAccessible* cell = mDoc->GetAccessible(cellContent);
>+  if (!cell)
>+    return nsnull;

you never derefrence it after this so no need for a check.

> 
>-  if (!cell) {
>-    return NS_ERROR_INVALID_ARG;
>+  if (cell == this) {
>+    // XXX bug 576838: crazy tables (like table6 in tables/test_table2.html) may
>+    // return itself as a cell what makes Orca hang.
>+    return nsnull;
>   }

nit, be nice to do

// XXX blah blah
// blahh
if (cell == this)
  return nsnull;

return cell;

>+  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
>+      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())

you need to case aRowIdx / aColIdx to PRUint32 before comparing to RowCount() / ColCount() to keep gcc happy

>+nsXULListboxAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex)
>+{ 
>   nsCOMPtr<nsIDOMXULSelectControlElement> control =
>     do_QueryInterface(mContent);
>+  if (!control)
>+    return nsnull;

you probably should warn here since mContent should have the right type given we created this type of accessible.
Attachment #624336 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 10

5 years ago
Re: 
>+  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
>+      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
you need to case aRowIdx / aColIdx to PRUint32 before comparing to RowCount() / ColCount() to keep gcc happy

This code was cloned from existing member function GetCellIndexAt()
http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/xpcAccessibleTable.cpp#51

I don't see why gcc would complain about one and not the other, but I'm a WIN guy so it may not be obvious to me.

Can you elaborate?
(Assignee)

Comment 11

5 years ago
Created attachment 625119 [details] [diff] [review]
Patch (v3)

FYI, we've got feedback+ and review+... this would be the final version unless Trev has further details for the gcc comments.
Attachment #624336 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=eea7a9606f4e
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c
Target Milestone: --- → mozilla15

Comment 14

5 years ago
(In reply to Mark Capella [:capella] from comment #13)
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6feee6c13c
(Reporter)

Comment 15

5 years ago
(In reply to Mark Capella [:capella] from comment #10)
> Re: 
> >+  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
> >+      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
> you need to case aRowIdx / aColIdx to PRUint32 before comparing to
> RowCount() / ColCount() to keep gcc happy
> 
> This code was cloned from existing member function GetCellIndexAt()
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/
> xpcAccessibleTable.cpp#51
> 
> I don't see why gcc would complain about one and not the other, but I'm a
> WIN guy so it may not be obvious to me.

it does complain about the other, but its only a warning.
https://hg.mozilla.org/mozilla-central/rev/7a6feee6c13c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.