739884 - decomtaminate impl of UnselectRow() and UnselectCol() on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

same procedure as bug 739882 but with UnselectRow() and UnselectColumn()

Comment 1

[Max Li [:maxli]]

Attachment: Patch v1

Comment 2

[Trevor Saunders (:tbsaunde)]

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)]

Comment on attachment 617152 [details] [diff] [review]
Patch v1

please fix comment 2 then rerequest review

Comment 4

[Max Li [:maxli]]

Attachment: Patch v2

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 618494 [details] [diff] [review]
Patch v2

Alex, I'm busy till friday afternoon atleast mind taking this?

Comment 6

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch v3

fixed nits

Comment 8

[Max Li [:maxli]]

Attachment: Patch v3.1

sorry, missed one

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 618617 [details] [diff] [review]
Patch v3.1

thank you! r=me

Comment 10

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c659e6b5a0a

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2c659e6b5a0a