Closed
Bug 739885
Opened 12 years ago
Closed 12 years ago
decomtaminate impl of IsRowSelected() and IsColumnSelected() on accessible tables
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
DUPLICATE
of bug 760880
People
(Reporter: tbsaunde, Assigned: xph9753)
References
Details
(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])
Attachments
(1 file, 2 obsolete files)
17.10 KB,
patch
|
Details | Diff | Splinter Review |
same procedure as bug 739882 eith IsRowSelected() and IsColumnSelected()
Assignee | ||
Comment 1•12 years ago
|
||
Could you assingn it to me, please?
Updated•12 years ago
|
Assignee: nobody → xph9753
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #618073 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Comment 3•12 years ago
|
||
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
Attachment #618073 -
Flags: review?(surkov.alexander)
Attachment #618073 -
Flags: review?(hub)
Attachment #618073 -
Flags: feedback?(surkov.alexander)
Comment 4•12 years ago
|
||
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.
Attachment #618073 -
Flags: review?(hub) → review-
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #618073 -
Attachment is obsolete: true
Attachment #618073 -
Flags: feedback?(surkov.alexander)
Attachment #619193 -
Flags: review?(hub)
Assignee | ||
Updated•12 years ago
|
Attachment #619193 -
Flags: feedback?(surkov.alexander)
Comment 6•12 years ago
|
||
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.
Attachment #619193 -
Flags: review?(hub) → review-
Reporter | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 619193 [details] [diff] [review] Patch v.2.0 cancelling review until Hub and Trevor comments are addressed
Attachment #619193 -
Flags: feedback?(surkov.alexander)
Comment 9•12 years ago
|
||
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•12 years ago
|
||
(In reply to alexander :surkov from comment #9) > I missed Trevor did review, so stopping reviewing. I need to use F5 more often :)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #619193 -
Attachment is obsolete: true
Attachment #622168 -
Flags: review?(trev.saunders)
Attachment #622168 -
Flags: feedback?(surkov.alexander)
Reporter | ||
Comment 12•12 years ago
|
||
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();
Attachment #622168 -
Flags: review?(trev.saunders)
Comment 13•12 years ago
|
||
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)
Attachment #622168 -
Flags: feedback?(surkov.alexander)
Comment 14•12 years ago
|
||
Andrei, do you have time to finish this bug in foreseeable future?
Assignee | ||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
Oh...... I think I opened a different bug and completed this work: bug 760880 ...
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•