760880 - decomtaminate Is Column / Row / Cell Selected() on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

similar approach to bug 757504

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Review request for the next one ... 

Also, I went ahead and fixed two alignment nits I forgot in bug 760881 re: HTMLTableAccessible::SelectRow() and UnselectRow().

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 632045 [details] [diff] [review]
Patch (v1)

>   do {
>     if (!nsAccUtils::IsARIASelected(row)) {
>-      Accessible* cell = GetCellInRowAt(row, aColumn);
>+      Accessible* cell = GetCellInRowAt(row, aColIdx);
>       if (!cell) // Do not fail due to wrong markup
>-        return NS_OK;
>+        return false;
>       
>       if (!nsAccUtils::IsARIASelected(cell))
>-        return NS_OK;
>+        return false;

nit, you could put these in one if.

>+  Accessible* row = GetRowAt(aRowIdx);
>   NS_ENSURE_ARG(row);

that'll return an nsresult which you shouldn't do.
It would also tend to indicate bad markup not bad argument so you should probably fail peacefully.


>   if (!nsAccUtils::IsARIASelected(row)) {
>-    Accessible* cell = GetCellInRowAt(row, aColumn);
>+    Accessible* cell = GetCellInRowAt(row, aColIdx);
>     NS_ENSURE_ARG(cell);

same

>-HTMLTableAccessible::IsColumnSelected(PRInt32 aColumn, bool* aIsSelected)
>+bool
>+HTMLTableAccessible::IsColSelected(PRUint32 aColIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>   PRInt32 colCount = 0;
>   nsresult rv = GetColumnCount(&colCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(rv, "GetColumnCount() Shouldn't fail!");

just use RowCount() / ColCount() here and below

> 
>-  if (aColumn < 0 || aColumn >= colCount)
>-    return NS_ERROR_INVALID_ARG;
>+  if (aColIdx < 0 || aColIdx >= colCount)
>+    return false;

caller should check this similar to all other methods on TableAccessible

>+  bool isSelected = false;
>   for (PRInt32 rowIdx = 0; rowIdx < rowCount; rowIdx++) {
>-    bool isSelected = false;
>-    rv = IsCellSelected(rowIdx, aColumn, &isSelected);
>+    bool isCellSelected = false;
>+    rv = IsCellSelected(rowIdx, aColIdx, &isCellSelected);

you could use the new version you add.

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

all three of these methods should check the indexs are valid.

>+XULListboxAccessible::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);
>+  NS_ASSERTION(rv, "GetSelectedCount() Shouldn't fail!");

you can just do NS_ENSURE_SUCCESS(rv, false)  which is better since it prints the error.

> 
>   PRInt32 rowCount = 0;
>   rv = GetRowCount(&rowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(rv, "GetRowCount() Shouldn't fail!");

same, use RowCount()

>   nsCOMPtr<nsIDOMXULSelectControlElement> control =
>     do_QueryInterface(mContent);
>   NS_ASSERTION(control,
>                "Doesn't implement nsIDOMXULSelectControlElement.");
> 
>   nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>-  control->GetItemAtIndex(aRow, getter_AddRefs(item));
>-  NS_ENSURE_TRUE(item, NS_ERROR_INVALID_ARG);
>+  control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
>+  NS_ASSERTION(item, "GetItemAtIndex() Shouldn't fail!");

you can use NS_ENSURE_SUCCESS here too

>+XULListboxAccessible::IsCellSelected(PRUint32 aRowIdx, PRUint32 aColIdx)
> {
>-  return IsRowSelected(aRowIndex, aIsSelected);
>+  bool isSelected = false;
>+  IsRowSelected(aRowIdx, &isSelected);
>+  return isSelected;

you could use the version you add

>   PRInt32 selectedrowCount = 0;
>   rv = GetSelectionCount(&selectedrowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(rv, "GetSelectionCount() Shouldn't fail!");

don't change from NS_ENSURE_SUCCESS

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

really this should never happen since a tree with no view has no rows, but we might have cahce issues related to updating mTreeView sync and the cache async, so I guess its ok.

>   nsCOMPtr<nsITreeSelection> selection;
>   nsresult rv = mTreeView->GetSelection(getter_AddRefs(selection));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(rv, "GetSelection() Shouldn't fail!");

leave just change second arg to false

Comment 3

[Mark Capella [:capella]]

FYI... in XULListboxAccessible::IsRowSelected()

>   nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>-  control->GetItemAtIndex(aRow, getter_AddRefs(item));
>-  NS_ENSURE_TRUE(item, NS_ERROR_INVALID_ARG);
>+  control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
>+  NS_ASSERTION(item, "GetItemAtIndex() Shouldn't fail!");
you can use NS_ENSURE_SUCCESS here too

Errors out, so I switched it to fail gracefully like in existing code in method XULListboxAccessible::CellAt().

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2)

With nits addressed .... for further review.

Comment 5

[Mark Capella [:capella]]

Comment on attachment 632215 [details] [diff] [review]
Patch (v2)

(forgot to set the review flag)

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #3)
> FYI... in XULListboxAccessible::IsRowSelected()
> 
> >   nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
> >-  control->GetItemAtIndex(aRow, getter_AddRefs(item));
> >-  NS_ENSURE_TRUE(item, NS_ERROR_INVALID_ARG);
> >+  control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
> >+  NS_ASSERTION(item, "GetItemAtIndex() Shouldn't fail!");
> you can use NS_ENSURE_SUCCESS here too
> 
> Errors out, so I switched it to fail gracefully like in existing code in
> method XULListboxAccessible::CellAt().

err, I should have said NS_ENSURE_TRUE

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v3)

Looking at it now, I must have done something wrong last night because using NS_ENSURE_SUCCESS(rv, false); does indeed work.

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 632215 [details] [diff] [review]
Patch (v2)

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

don't call RowCount() every time through the loop, use a local var instead.

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

the other cleanup seems to make the temporary bool pointless, it would be nice if you got rid of it.

btw also instead of breaking you could just return false.

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

same as RowCount()

>+    bool isCellSelected = IsCellSelected(aRowIdx, colIdx);
>+    isSelected = isCellSelected;
>+    if (!isCellSelected)
>+      break;

also the same, it seems you could get rid of the temporary and return false

>+HTMLTableAccessible::IsCellSelected(PRUint32 aRowIdx, PRUint32 aColIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aIsSelected);
>-  *aIsSelected = false;
>-
>   nsITableLayout *tableLayout = GetTableLayout();
>   NS_ENSURE_STATE(tableLayout);

just return false, NS_ENSURE_STATE doesn't return a bool.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 633271 [details] [diff] [review]
Patch (v3)

r=me with comment 8 fixed

Comment 10

[Mark Capella [:capella]]

Thanks! I think the local bool in the for loop is to ensure that at least one "selected == true" condition is met in addition to no "selected == false" conditions. Other wise, it could loop zero times, and report back a selected == true erroneously.

Comment 11

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=77f704ba95f8

Comment 12

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb67e4c9f7df

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21ab04d13974