Last Comment Bug 760880 - decomtaminate Is Column / Row / Cell Selected() on accessible tables
: decomtaminate Is Column / Row / Cell Selected() on accessible tables
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
:
Mentors:
: 739885 (view as bug list)
Depends on:
Blocks: dexpcoma11y 760878
  Show dependency treegraph
 
Reported: 2012-06-02 18:55 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-06-16 21:57 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (22.47 KB, patch)
2012-06-11 15:46 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (22.53 KB, patch)
2012-06-12 05:21 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (22.54 KB, patch)
2012-06-14 14:15 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-06-02 18:55:15 PDT
similar approach to bug 757504
Comment 1 Mark Capella [:capella] 2012-06-11 15:46:12 PDT
Created attachment 632045 [details] [diff] [review]
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) 2012-06-12 01:40:59 PDT
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] 2012-06-12 03:52:15 PDT
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] 2012-06-12 05:21:27 PDT
Created attachment 632215 [details] [diff] [review]
Patch (v2)

With nits addressed .... for further review.
Comment 5 Mark Capella [:capella] 2012-06-14 03:49:06 PDT
Comment on attachment 632215 [details] [diff] [review]
Patch (v2)

(forgot to set the review flag)
Comment 6 Trevor Saunders (:tbsaunde) 2012-06-14 13:59:23 PDT
(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] 2012-06-14 14:15:37 PDT
Created attachment 633271 [details] [diff] [review]
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) 2012-06-14 14:19:32 PDT
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) 2012-06-14 14:27:21 PDT
Comment on attachment 633271 [details] [diff] [review]
Patch (v3)

r=me with comment 8 fixed
Comment 10 Mark Capella [:capella] 2012-06-14 14:35:45 PDT
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] 2012-06-14 19:19:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77f704ba95f8
Comment 12 Mark Capella [:capella] 2012-06-15 01:41:59 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb67e4c9f7df
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:53:36 PDT
https://hg.mozilla.org/mozilla-central/rev/21ab04d13974
Comment 14 alexander :surkov 2012-06-16 21:57:36 PDT
*** Bug 739885 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.