Last Comment Bug 739884 - decomtaminate impl of UnselectRow() and UnselectCol() on accessible tables
: decomtaminate impl of UnselectRow() and UnselectCol() on accessible tables
Status: RESOLVED FIXED
[good first bug][mentor=eitan@mozilla...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-03-27 21:18 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-27 06:56 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.01 KB, patch)
2012-04-20 17:16 PDT, Max Li [:maxli]
no flags Details | Diff | Review
Patch v2 (12.25 KB, patch)
2012-04-25 17:55 PDT, Max Li [:maxli]
surkov.alexander: review+
Details | Diff | Review
Patch v3 (12.22 KB, patch)
2012-04-26 04:13 PDT, Max Li [:maxli]
no flags Details | Diff | Review
Patch v3.1 (12.22 KB, patch)
2012-04-26 04:20 PDT, Max Li [:maxli]
surkov.alexander: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-03-27 21:18:15 PDT
same procedure as bug 739882 but with UnselectRow() and UnselectColumn()
Comment 1 Max Li [:maxli] 2012-04-20 17:16:16 PDT
Created attachment 617152 [details] [diff] [review]
Patch v1
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-20 18:32:24 PDT
Comment on attachment 617152 [details] [diff] [review]
Patch v1

>+xpcAccessibleTable::UnselectColumn(PRInt32 aColIdx)
>+{
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+  
>+  mTable->UnselectCol(aColIdx);

you should check that the row index argument is valid as in >= 0 and < RowCount() and return NS_ERROR_INVALID_ARG if it isn't

>+  return NS_OK;
>+}
>+
>+nsresult
>+xpcAccessibleTable::UnselectRow(PRInt32 aRowIdx)
>+{
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+
>+  mTable->UnselectRow(aRowIdx);

same

>+void
>+nsXULListboxAccessible::UnselectRow(PRUint32 aRowIdx)
> {
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-  
>   nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
>     do_QueryInterface(mContent);
>-  NS_ASSERTION(control,
>-               "Doesn't implement nsIDOMXULMultiSelectControlElement.");

keep the assert

>   nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>-  control->GetItemAtIndex(aRow, getter_AddRefs(item));
>-  NS_ENSURE_TRUE(item, NS_ERROR_INVALID_ARG);
>-
>-  return control->RemoveItemFromSelection(item);
>-}
>-
>-NS_IMETHODIMP
>-nsXULListboxAccessible::UnselectColumn(PRInt32 aColumn)
>-{
>-  // xul:listbox and xul:richlistbox support row selection only.
>-  return NS_OK;
>+  
>+  if(control) {

I'm not sure you need to add the if, but if you are adding an if please keep nsCOMPtr<foo> item inside the if.
Comment 3 Trevor Saunders (:tbsaunde) 2012-04-22 13:58:53 PDT
Comment on attachment 617152 [details] [diff] [review]
Patch v1

please fix comment 2 then rerequest review
Comment 4 Max Li [:maxli] 2012-04-25 17:55:50 PDT
Created attachment 618494 [details] [diff] [review]
Patch v2
Comment 5 Trevor Saunders (:tbsaunde) 2012-04-25 18:41:42 PDT
Comment on attachment 618494 [details] [diff] [review]
Patch v2

Alex, I'm busy till friday afternoon atleast mind taking this?
Comment 6 alexander :surkov 2012-04-26 00:42:00 PDT
Comment on attachment 618494 [details] [diff] [review]
Patch v2

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

::: accessible/src/generic/ARIAGridAccessible.h
@@ +66,5 @@
>    NS_DECL_OR_FORWARD_NSIACCESSIBLETABLE_WITH_XPCACCESSIBLETABLE
>  
> +  // TableAccessible
> +  virtual void UnselectCol(PRUint32 aColIdx);
> +  virtual void UnselectRow(PRUint32 aRowIdx);

move them to existing TableAccessible section

::: accessible/src/html/nsHTMLTableAccessible.h
@@ +133,5 @@
>    virtual PRUint32 RowCount();
>    virtual void Summary(nsString& aSummary);
>    virtual bool IsProbablyLayoutTable();
> +  virtual void UnselectCol(PRUint32 aColIdx);
> +  virtual void UnselectRow(PRUint32 aRowIdx);

keep them in order of TableAccessible.h

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +27,5 @@
>    nsresult GetRowCount(PRInt32* aRowCount);
>    nsresult GetSummary(nsAString& aSummary);
>    nsresult IsProbablyForLayout(bool* aIsForLayout);
> +  nsresult UnselectColumn(PRInt32 aColIdx);
> +  nsresult UnselectRow(PRInt32 aRowIdx);

keep it in the order of TableAccessibe.h

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +782,5 @@
>      do_QueryInterface(mContent);
>    NS_ASSERTION(control,
>                 "Doesn't implement nsIDOMXULMultiSelectControlElement.");
>  
> +  if(control) {

space after if
Comment 7 Max Li [:maxli] 2012-04-26 04:13:33 PDT
Created attachment 618616 [details] [diff] [review]
Patch v3

fixed nits
Comment 8 Max Li [:maxli] 2012-04-26 04:20:47 PDT
Created attachment 618617 [details] [diff] [review]
Patch v3.1

sorry, missed one
Comment 9 alexander :surkov 2012-04-26 04:30:48 PDT
Comment on attachment 618617 [details] [diff] [review]
Patch v3.1

thank you! r=me
Comment 11 Ed Morley [:emorley] 2012-04-27 06:56:10 PDT
https://hg.mozilla.org/mozilla-central/rev/2c659e6b5a0a

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