The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: maxli)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=eitan@mozilla.com][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
same procedure as bug 739882 but with UnselectRow() and UnselectColumn()
(Assignee)

Comment 1

5 years ago
Created attachment 617152 [details] [diff] [review]
Patch v1
Assignee: nobody → maxli
Attachment #617152 - Flags: review?(eitan)
(Reporter)

Comment 2

5 years ago
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.
Attachment #617152 - Flags: review?(eitan) → review?(trev.saunders)
(Reporter)

Comment 3

5 years ago
Comment on attachment 617152 [details] [diff] [review]
Patch v1

please fix comment 2 then rerequest review
Attachment #617152 - Flags: review?(trev.saunders)
(Assignee)

Comment 4

5 years ago
Created attachment 618494 [details] [diff] [review]
Patch v2
Attachment #617152 - Attachment is obsolete: true
Attachment #618494 - Flags: review?(trev.saunders)
(Reporter)

Comment 5

5 years ago
Comment on attachment 618494 [details] [diff] [review]
Patch v2

Alex, I'm busy till friday afternoon atleast mind taking this?
Attachment #618494 - Flags: review?(trev.saunders) → review?(surkov.alexander)

Comment 6

5 years ago
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
Attachment #618494 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 618616 [details] [diff] [review]
Patch v3

fixed nits
Attachment #618494 - Attachment is obsolete: true
Attachment #618616 - Flags: review?(surkov.alexander)
(Assignee)

Comment 8

5 years ago
Created attachment 618617 [details] [diff] [review]
Patch v3.1

sorry, missed one
Attachment #618616 - Attachment is obsolete: true
Attachment #618616 - Flags: review?(surkov.alexander)
Attachment #618617 - Flags: review?(surkov.alexander)

Comment 9

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

thank you! r=me
Attachment #618617 - Flags: review?(surkov.alexander) → review+

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c659e6b5a0a
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/2c659e6b5a0a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.