decomtaminate getting row and column count on accessible tables

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: tbsaunde, Assigned: maxli)

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
similar to what was done with  IsProbablyForLayout and Caption in bug 648265.
implement the methods RowCount() and ColCount() from TableAccessible on each of nsARIAGridAccessible nsHTMLTableAccessible nsXULListboxAccessible nsXULTreeGridAccessible
the same way GetRowCount() and GetColumnCount() currently are then remove GGetRowCount() and GetColumnCount()
implement GetRowCount() and and GetColumnCount() on xpcAccessibleTable using the TableAccessible pointer member
finally adjust the NS_DECL_OR_FORWARD_WITH_XPCACCESSIBLETABLE macro in accessible/src/xpcom/xpcAccessibleTable.h to forward GetRowCount() and GetColumnCount() to xpcAccessibleTable.
(Reporter)

Updated

5 years ago
Blocks: 574336
(Assignee)

Comment 1

5 years ago
Created attachment 615584 [details] [diff] [review]
Patch v1
Assignee: nobody → maxli
Attachment #615584 - Flags: feedback?(surkov.alexander)

Comment 2

5 years ago
Comment on attachment 615584 [details] [diff] [review]
Patch v1

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

please attach new patch and get review from Trevor

::: accessible/src/generic/ARIAGridAccessible.cpp
@@ +90,2 @@
>    if (IsDefunct())
> +    return 0;

you don't need IsDefunct check, now it's internal method

@@ +110,3 @@
>  {
> +  if (IsDefunct())
> +    return 0;

same

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +569,2 @@
>    if (IsDefunct())
> +    return 0;

same

@@ +573,2 @@
>  
> +  PRInt32 rows, columns;

initialize them please

@@ +573,4 @@
>  
> +  PRInt32 rows, columns;
> +  tableLayout->GetTableSize(rows, columns);
> +  return columns;

wonder if you get any warnings here. btw, I filed bug 746060 for the GetTableSize issue.

@@ +582,2 @@
>    if (IsDefunct())
> +    return 0;

no defuct pls

@@ +586,2 @@
>  
> +  PRInt32 rows, columns;

initialize

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +21,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +xpcAccessibleTable::GetColumnCount(PRInt32 *aColumnCount)

type* name

@@ +33,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +xpcAccessibleTable::GetRowCount(PRInt32 *aRowCount)

type* name

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +23,5 @@
>    xpcAccessibleTable(mozilla::a11y::TableAccessible* aTable) : mTable(aTable) { }
>  
>    nsresult GetCaption(nsIAccessible** aCaption);
> +  nsresult GetColumnCount(PRInt32 *aColumnCount);
> +  nsresult GetRowCount(PRInt32 *aRowCount);

same

@@ +39,5 @@
>      { return xpcAccessibleTable::GetSummary(aSummary); } \
> +  NS_SCRIPTABLE NS_IMETHOD GetColumnCount(PRInt32 *aColumnCount) \
> +    { return xpcAccessibleTable::GetColumnCount(aColumnCount); } \
> +  NS_SCRIPTABLE NS_IMETHOD GetRowCount(PRInt32 *aRowCount) \
> +    { return xpcAccessibleTable::GetRowCount(aRowCount); } \

same

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +247,2 @@
>    if (IsDefunct())
> +    return 0;

no defunct

@@ +276,2 @@
>    if (IsDefunct())
> +    return 0;

same

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +77,3 @@
>  {
> +  if (IsDefunct())
> +    return 0;

no defunct

@@ +86,3 @@
>  {
> +  if (IsDefunct() || !mTreeView)
> +    return 0;

no defunct check

@@ +89,2 @@
>  
> +  PRInt32 rowCount;

initialize it

@@ +89,4 @@
>  
> +  PRInt32 rowCount;
> +  mTreeView->GetRowCount(&rowCount);
> +  return rowCount;

I filed bug 746063 about nsITreeView issue, but probably you should make sure that rowCount >= 0 before you return it. Otherwise you end up with not desired results I think.
Attachment #615584 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 615664 [details] [diff] [review]
Patch v2
Attachment #615584 - Attachment is obsolete: true
Attachment #615664 - Flags: review?(trev.saunders)
(Reporter)

Comment 4

5 years ago
Comment on attachment 615664 [details] [diff] [review]
Patch v2

>   AccIterator cellIter(row, filters::GetCell);
>   nsAccessible *cell = nsnull;
> 
>+  PRUint32 columnCount = 0;

nit, colCount

>-  PRInt32 rows;
>-  return tableLayout->GetTableSize(rows, *acolumnCount);
>+  PRInt32 rows = 0, columns = 0;
>+  tableLayout->GetTableSize(rows, columns);

nit, rowCount colCount

>-  PRInt32 columns;
>-  return tableLayout->GetTableSize(*arowCount, columns);
>+  PRInt32 rows = 0, columns = 0;

same

>+xpcAccessibleTable::GetColumnCount(PRInt32* aColumnCount)
>+{
>+  NS_ENSURE_ARG_POINTER(aColumnCount);
>+  *aColumnCount = 0;
>+  if (!mTable)

nit, blank line before if

>+xpcAccessibleTable::GetRowCount(PRInt32* aRowCount)
>+{
>+  NS_ENSURE_ARG_POINTER(aRowCount);
>+  *aRowCount = 0;
>+  if (!mTable)

same

>   nsCOMPtr<nsIDOMXULSelectControlElement> element(do_QueryInterface(mContent));
>-  NS_ENSURE_STATE(element);
> 
>   PRUint32 itemCount = 0;
>-  nsresult rv = element->GetItemCount(&itemCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  element->GetItemCount(&itemCount);

you probably shouldn't rely on the QueryInterface succeding.

general comment you shtype* name not type *name.

r=me with those fixed

btw you should change the methods that call GetRowCount() / GetColumnCount() to call the new methods, but doing that as other methods are converted or in follow ups is probably fine.

sorry about the delay in reviewing this, was busy with school and life.
Attachment #615664 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 616750 [details] [diff] [review]
Patch v3
Attachment #615664 - Attachment is obsolete: true
Attachment #616750 - Flags: review?(trev.saunders)
(Reporter)

Updated

5 years ago
Attachment #616750 - Flags: review?(trev.saunders) → review+
(Reporter)

Updated

5 years ago
Blocks: 747219
(Reporter)

Updated

5 years ago
Blocks: 747227

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd794853712c
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/bd794853712c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 757440
You need to log in before you can comment on or make changes to this bug.