739885 - decomtaminate impl of IsRowSelected() and IsColumnSelected() on accessible tables

Status: RESOLVED DUPLICATE of bug 760880

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

same procedure as bug 739882 eith IsRowSelected() and IsColumnSelected()

Comment 1

[Andrei Touzik]

Could you assingn it to me, please?

Comment 2

[Andrei Touzik]

Attachment: Patch v.1.0

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 618073 [details] [diff] [review]
Patch v.1.0

let's get a review from the mentor and then I'll do feedback

Comment 4

[Hubert Figuiere [:hub]]

Comment on attachment 618073 [details] [diff] [review]
Patch v.1.0

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

::: accessible/src/generic/ARIAGridAccessible.cpp
@@ +315,4 @@
>  
>    do {
>      if (!nsAccUtils::IsARIASelected(row)) {
> +      nsAccessible *cell = GetCellInRowAt(row, aColIdx);

nit: nsAccessible*

@@ +319,1 @@
>        if (!cell) // Do not fail due to wrong markup

comment: this is no longer a failure.

@@ +322,2 @@
>        if (!nsAccUtils::IsARIASelected(cell))
> +        return false;

we should be able to have one if () statement

something like 
if (!cell || !nsAccUtils::IsARIASelected(cell))
  return false;

@@ +332,2 @@
>  {
> +  nsAccessible *row = GetRowAt(aRowIdx);

nsAccessible*

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1055,2 @@
>      if (NS_SUCCEEDED(rv)) {
> +      return isSelected;

this does not work. the logic does not return isSelected. It just break the loop if !isSelected.

Line 1062 is where you should return isSelected.

@@ +1082,2 @@
>      if (NS_SUCCEEDED(rv)) {
> +      return isSelected;

same as above (line 1056)

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +47,5 @@
>    NS_SCRIPTABLE NS_IMETHOD GetColumnExtentAt(PRInt32 row, PRInt32 column, PRInt32 *_retval NS_OUTPARAM); \
>    NS_SCRIPTABLE NS_IMETHOD GetRowExtentAt(PRInt32 row, PRInt32 column, PRInt32 *_retval NS_OUTPARAM); \
>    NS_SCRIPTABLE NS_IMETHOD GetColumnDescription(PRInt32 columnIndex, nsAString & _retval NS_OUTPARAM); \
>    NS_SCRIPTABLE NS_IMETHOD GetRowDescription(PRInt32 rowIndex, nsAString & _retval NS_OUTPARAM); \
> +  NS_SCRIPTABLE NS_IMETHOD IsColumnSelected(PRInt32 columnIndex, bool *_retval NS_OUTPARAM) \

nit: bool*

see also below, line 53.

Comment 5

[Andrei Touzik]

Attachment: Patch v.2.0

Comment 6

[Hubert Figuiere [:hub]]

Comment on attachment 619193 [details] [diff] [review]
Patch v.2.0

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1041,3 @@
>    for (PRInt32 rowIdx = 0; rowIdx < rowCount; rowIdx++) {
> +    rv = IsCellSelected(rowIdx, aColIdx, &isSelected);
> +    if (NS_FAILED(rv)) {

In the old we don't break out of the loop on failure.

Here you should do 
if (NS_SUCCEEDED(rv)) {
  if (!isSelected)
    break;
}

@@ +1071,3 @@
>    for (PRInt32 colIdx = 0; colIdx < colCount; colIdx++) {
> +    rv = IsCellSelected(aRowIdx, colIdx, &isSelected);
> +    if (NS_FAILED(rv)) {

same as above.

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 619193 [details] [diff] [review]
Patch v.2.0

>+ARIAGridAccessible::IsColSelected(PRUint32 aColIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  NS_ENSURE_ARG(IsValidColumn(aColumn));
>+  NS_ENSURE_ARG(IsValidColumn(aColIdx));

that's wrong, you'll return an nsresult in case of failure, and you shouldn't need to check this at all.

>+bool
>+ARIAGridAccessible::IsRowSelected(PRUint32 aRowIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  nsAccessible *row = GetRowAt(aRow);
>+  nsAccessible* row = GetRowAt(aRowIdx);
>   NS_ENSURE_ARG(row);

no, return false and assert if you like.

>   // TableAccessible
>   virtual PRUint32 ColCount();
>   virtual PRUint32 RowCount();
>   virtual void UnselectCol(PRUint32 aColIdx);
>   virtual void UnselectRow(PRUint32 aRowIdx);
>+  virtual bool IsColSelected(PRUint32 aColIdx);
>+  virtual bool IsRowSelected(PRUint32 aRowIdx);

keep in order of TableAccessible.h

>-
>   PRInt32 colCount = 0;
>   nsresult rv = GetColumnCount(&colCount);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (aColumn < 0 || aColumn >= colCount)
>-    return NS_ERROR_INVALID_ARG;
>+  if (aColIdx < 0 || aColIdx >= colCount)
>+    return false;
> 
>   PRInt32 rowCount = 0;
>   rv = GetRowCount(&rowCount);
>   NS_ENSURE_SUCCESS(rv, rv);

I think you should remove these checks and require method is only called with valid indexs.

you could add asserts with RowCount() and ColCount() if you like.

>+nsHTMLTableAccessible::IsRowSelected(PRUint32 aRowIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>   PRInt32 rowCount = 0;
>   nsresult rv = GetRowCount(&rowCount);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (aRow < 0 || aRow >= rowCount)
>-    return NS_ERROR_INVALID_ARG;
>+  if (aRowIdx < 0 || aRowIdx >= rowCount)
>+    return false;
> 
>   PRInt32 colCount = 0;
>   rv = GetColumnCount(&colCount);
>   NS_ENSURE_SUCCESS(rv, rv);

same here

> 
>+  // TableAccessible
>+  virtual bool IsColSelected(PRUint32 aColIdx);
>+  virtual bool IsRowSelected(PRUint32 aRowIdx);
>+

keep in existing section.

>+xpcAccessibleTable::IsColumnSelected(PRInt32 aColumnIndex, bool* aIsSelected)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsSelected);
>+  *aIsSelected = false;
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
> 
>+  *aIsSelected = mTable->IsColSelected(aColumnIndex);

you should check aColIdx is a valid index to return NS_ERROR_INVALID_ARG if not.

>+xpcAccessibleTable::IsRowSelected(PRInt32 aRowIndex, bool* aIsSelected)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsSelected);
>+  *aIsSelected = false;
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+
>+  *aIsSelected = mTable->IsRowSelected(aRowIndex);

same

>   nsresult GetSummary(nsAString& aSummary);
>   nsresult GetColumnCount(PRInt32* aColumnCount);
>   nsresult GetRowCount(PRInt32* aRowCount);
>   nsresult UnselectColumn(PRInt32 aColIdx);
>   nsresult UnselectRow(PRInt32 aRowIdx);
>   nsresult IsProbablyForLayout(bool* aIsForLayout);
>+  nsresult IsColumnSelected(PRInt32 aColumnIndex, bool* aIsSelected);
>+  nsresult IsRowSelected(PRInt32 aRowIndex, bool* aIsSelected);

nit, use same order as TableAccessible.h

>   NS_SCRIPTABLE NS_IMETHOD GetRowCount(PRInt32* aRowCount) \
>     { return xpcAccessibleTable::GetRowCount(aRowCount); } \
>-  NS_SCRIPTABLE NS_IMETHOD GetCellAt(PRInt32 rowIndex, PRInt32 columnIndex, nsIAccessible * *_retval NS_OUTPARAM); \
>-  NS_SCRIPTABLE NS_IMETHOD GetCellIndexAt(PRInt32 rowIndex, PRInt32 columnIndex, PRInt32 *_retval NS_OUTPARAM); \
>-  NS_SCRIPTABLE NS_IMETHOD GetColumnIndexAt(PRInt32 cellIndex, PRInt32 *_retval NS_OUTPARAM); \
>-  NS_SCRIPTABLE NS_IMETHOD GetRowIndexAt(PRInt32 cellIndex, PRInt32 *_retval NS_OUTPARAM); \
>-  NS_SCRIPTABLE NS_IMETHOD GetRowAndColumnIndicesAt(PRInt32 cellIndex, PRInt32 *rowIndex NS_OUTPARAM, PRInt32 *columnIndex NS_OUTPARAM); \
>-  NS_SCRIPTABLE NS_IMETHOD GetColumnExtentAt(PRInt32 row, PRInt32 column, PRInt32 *_retval NS_OUTPARAM); \
>-  NS_SCRIPTABLE NS_IMETHOD GetRowExtentAt(PRInt32 row, PRInt32 column, PRInt32 *_retval NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetCellAt(PRInt32 rowIndex, PRInt32 columnIndex, nsIAccessible**  _retval NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetCellIndexAt(PRInt32 rowIndex, PRInt32 columnIndex, PRInt32* _retval NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetColumnIndexAt(PRInt32 cellIndex, PRInt32* _retval NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetRowIndexAt(PRInt32 cellIndex, PRInt32* _retval NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetRowAndColumnIndicesAt(PRInt32 cellIndex, PRInt32* rowIndex NS_OUTPARAM, PRInt32* columnIndex NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetColumnExtentAt(PRInt32 row, PRInt32 column, PRInt32* _retval NS_OUTPARAM); \
>+  NS_SCRIPTABLE NS_IMETHOD GetRowExtentAt(PRInt32 row, PRInt32 column, PRInt32* _retval NS_OUTPARAM); \

it'd be easier to read if you left these as they are  especially since we'll change them soon anyway.

>+bool
>+nsXULListboxAccessible::IsColSelected(PRUint32 aColIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
>     do_QueryInterface(mContent);
>   NS_ASSERTION(control,
>                "Doesn't implement nsIDOMXULMultiSelectControlElement.");
> 
>   PRInt32 selectedrowCount = 0;
>   nsresult rv = control->GetSelectedCount(&selectedrowCount);
>   NS_ENSURE_SUCCESS(rv, rv);

don't return an nsresult from a function returning a bool.

> 
>   PRInt32 rowCount = 0;
>   rv = GetRowCount(&rowCount);
>   NS_ENSURE_SUCCESS(rv, rv);

use RowCount() here

>+nsXULListboxAccessible::IsRowSelected(PRUint32 aRowIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   nsCOMPtr<nsIDOMXULSelectControlElement> control =
>     do_QueryInterface(mContent);
>   NS_ASSERTION(control,
>                "Doesn't implement nsIDOMXULSelectControlElement.");
>-  
>+
>   nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>-  control->GetItemAtIndex(aRow, getter_AddRefs(item));
>+  control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
>   NS_ENSURE_TRUE(item, NS_ERROR_INVALID_ARG);

no, returning a nsresult from a function returning bool, return false instead.

>+bool
>+nsXULTreeGridAccessible::IsColSelected(PRUint32 aColIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>+  /*
>+     If all the row has been selected, then all the columns are selected.
>+     Because we can't select a column alone.
>+   */
> 
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  // If all the row has been selected, then all the columns are selected.
>-  // Because we can't select a column alone.

no leave the comment as is.

>-  
>   PRInt32 rowCount = 0;
>   nsresult rv = GetRowCount(&rowCount);
>   NS_ENSURE_SUCCESS(rv, rv);

use RowCount()

>+bool
>+nsXULTreeGridAccessible::IsRowSelected(PRUint32 aRowIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   if (!mTreeView)
>     return NS_ERROR_INVALID_ARG;

wrong, this case should never happen since it shouldn't be called with 0 rows.  You should probably return false in this case.

> 
>   nsCOMPtr<nsITreeSelection> selection;
>   nsresult rv = mTreeView->GetSelection(getter_AddRefs(selection));
>   NS_ENSURE_SUCCESS(rv, rv);

returning nsresult as bool again.

>+  // TableAccessible
>+  virtual bool IsColSelected(PRUint32 aColIdx);
>+  virtual bool IsRowSelected(PRUint32 aRowIdx);

keep in existing section.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 619193 [details] [diff] [review]
Patch v.2.0

cancelling review until Hub and Trevor comments are addressed

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 619193 [details] [diff] [review]
Patch v.2.0

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

I missed Trevor did review, so stopping reviewing.

::: accessible/src/generic/ARIAGridAccessible.cpp
@@ +298,2 @@
>  {
> +  NS_ENSURE_ARG(IsValidColumn(aColIdx));

this returns nsresult, you should return bool but I don't think you need to check it since it's internal method, the caller should take care to pass valid index

@@ +321,1 @@
>    NS_ENSURE_ARG(row);

no row means false

::: accessible/src/generic/ARIAGridAccessible.h
@@ +76,5 @@
>    virtual PRUint32 RowCount();
>    virtual void UnselectCol(PRUint32 aColIdx);
>    virtual void UnselectRow(PRUint32 aRowIdx);
> +  virtual bool IsColSelected(PRUint32 aColIdx);
> +  virtual bool IsRowSelected(PRUint32 aRowIdx);

keep the order like in TableAccessible.h

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1029,3 @@
>    PRInt32 colCount = 0;
>    nsresult rv = GetColumnCount(&colCount);
>    NS_ENSURE_SUCCESS(rv, rv);

use ColCount() instead

Comment 10

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #9)
> I missed Trevor did review, so stopping reviewing.

I need to use F5 more often :)

Comment 11

[Andrei Touzik]

Attachment: Patch v.3.0

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 622168 [details] [diff] [review]
Patch v.3.0

>   do {
>-    if (!nsAccUtils::IsARIASelected(row)) {
>-      nsAccessible *cell = GetCellInRowAt(row, aColumn);
>-      if (!cell) // Do not fail due to wrong markup
>-        return NS_OK;
>-      
>-      if (!nsAccUtils::IsARIASelected(cell))
>-        return NS_OK;
>+    isSelected = nsAccUtils::IsARIASelected(row);
>+    if(!isSelected) {
>+      break;
>     }
>   } while ((row = rowIter.Next()));
> 
>-  *aIsSelected = true;
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-ARIAGridAccessible::IsRowSelected(PRInt32 aRow, bool* aIsSelected)
>-{
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  nsAccessible *row = GetRowAt(aRow);
>-  NS_ENSURE_ARG(row);
>-
>-  if (!nsAccUtils::IsARIASelected(row)) {
>-    AccIterator cellIter(row, filters::GetCell);
>-    nsAccessible *cell = nsnull;
>-    while ((cell = cellIter.Next())) {
>-      if (!nsAccUtils::IsARIASelected(cell))
>-        return NS_OK;
>+  if(!isSelected) {
>+    for (PRInt32 rowIdx = 0; rowIdx < RowCount(); rowIdx++) {
>+      isSelected = IsCellSelected(rowIdx, aColIdx);
>+      if (!isSelected)
>+        break;
>     }
>   }

keep the old logic, it should be much faster and there's no reason to change.

>+  nsAccessible* row = GetRowAt(aRowIdx);
>+  if(!row)
>+    return false;
>+
>+  isSelected = nsAccUtils::IsARIASelected(row);
>+  if(!isSelected) {

return early please.

>+    for (PRInt32 colIdx = 0; colIdx < ColCount(); colIdx++) {
>+      isSelected = IsCellSelected(aRowIdx, colIdx);
>+      if (!isSelected)
>+        break;

just use return, and no need for the variable.

>+  bool isSelected = false;
>+  for (PRInt32 rowIdx = 0; rowIdx < RowCount(); rowIdx++) {

PRUint32 would make match the return type of ColCount()

>+    isSelected = IsCellSelected(rowIdx, aColIdx);
>+    if (!isSelected)
>+      break;

please prefer return in cases like this. (here and later)

>+  bool isSelected = false;
>+  for (PRInt32 colIdx = 0; colIdx < ColCount(); colIdx++) {

don't call functions in a loop like this unlesswhat it returns may change, or you are very sure the compiler will know it can  call the function only once neither of which is true here.

> nsHTMLTableAccessible::IsCellSelected(PRInt32 aRow, PRInt32 aColumn,
>                                       bool *aIsSelected)

not actually changed here and in ARIAGridAccessible too.

>+  virtual bool IsCellSelected(PRUint32 aRowIdx, PRUint32 aColIdx);

how does that compile since you never define it?

> 
>   nsCOMPtr<nsIDOMXULSelectControlElement> control =
>     do_QueryInterface(mContent);
>-  NS_ASSERTION(control,
>-               "Doesn't implement nsIDOMXULSelectControlElement.");

leave the assert please

>+  if(!control)
>+    return false;
>   
>   nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>-  control->GetItemAtIndex(aRow, getter_AddRefs(item));
>-  NS_ENSURE_TRUE(item, NS_ERROR_INVALID_ARG);
>+  control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
>+  if(!control)
>+    return false;

NS_ENSURE_TRUE(item, false);

> 
>-  nsresult rv = mTreeView->GetSelection(getter_AddRefs(selection));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  mTreeView->GetSelection(getter_AddRefs(selection));
>+  if (!selection)

same


> 
>   // nsIAccessibleTable
>   NS_DECL_OR_FORWARD_NSIACCESSIBLETABLE_WITH_XPCACCESSIBLETABLE
> 
>   // TableAccessible
>   virtual PRUint32 ColCount();
>   virtual PRUint32 RowCount();
>+  virtual bool IsColSelected(PRUint32 aColIdx);
>+  virtual bool IsRowSelected(PRUint32 aRowIdx);
>   virtual void UnselectRow(PRUint32 aRowIdx);
> 
>   // nsAccessNode
>   virtual void Shutdown();
> 
>   // nsAccessible
>   virtual mozilla::a11y::TableAccessible* AsTable() { return this; }
>   virtual mozilla::a11y::role NativeRole();

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 622168 [details] [diff] [review]
Patch v.3.0

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

Andrei, could you please address Trevor's comments and then please ask him for review? (canceling feedback request for now)

Comment 14

[alexander :surkov (:asurkov)]

Andrei, do you have time to finish this bug in foreseeable future?

Comment 15

[Andrei Touzik]

(In reply to alexander :surkov from comment #14)
> Andrei, do you have time to finish this bug in foreseeable future?

I am sorry, the next two weeks are quite busy

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Andrei Touzik from comment #15)
> (In reply to alexander :surkov from comment #14)
> > Andrei, do you have time to finish this bug in foreseeable future?
> 
> I am sorry, the next two weeks are quite busy

it's ok, please take your time. I just needed to ensure you're on the bug still.

Comment 17

[Mark Capella [:capella]]

Oh...... I think I opened a different bug and completed this work: bug 760880 ...