706067 - Make takeFocus work on widget items

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Currently takeFocus works on tree items only. We need to support other widgets.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 577557 [details] [diff] [review]
patch

>+void
>+nsAccessible::SetCurrentItem(nsAccessible* aItem)
>+{
>+  nsAutoString id;
>+  if (nsCoreUtils::GetID(aItem->GetContent(), id)) {

Are we absolutely sure that aItem->GetContent() will always succeed and not return NULL? If not, I'd prefer to move this call up and put its result into a local variable that we can test for validity.

>+    gA11yEventDumpToConsole = true; // debug stuff

Nit: Please comment before landing.

>+  <select id="listbox" size="5">
>+    <option id="lb_item1">item1</option>
>+    <option id="lb_item2">item1</option>
>+  </select>

Nit: please make the text of lb_item2 read "Item2" for consistency.

r=me with the above fixed and answered.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> Comment on attachment 577557 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >+void
> >+nsAccessible::SetCurrentItem(nsAccessible* aItem)
> >+{
> >+  nsAutoString id;
> >+  if (nsCoreUtils::GetID(aItem->GetContent(), id)) {
> 
> Are we absolutely sure that aItem->GetContent() will always succeed and not
> return NULL? If not, I'd prefer to move this call up and put its result into
> a local variable that we can test for validity.

I can be absolutely sure iff GetContent() never returns a null. But it shouldn't return null in this case because we don't generally process defunct accessibles.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 577557 [details] [diff] [review]
patch

>+  if (nsCoreUtils::GetID(aItem->GetContent(), id)) {

It seems like we might be able to replace nsCoreUtils::GetId() with nsIContent::GetId() atleast from reading the comment, haven't dug through the impls yet.

>+void
>+nsHTMLComboboxAccessible::SetCurrentItem(nsAccessible* aItem)
>+{
>+  return AreItemsOperable() ? mListAccessible->SetCurrentItem(aItem) : nsnull;

! returning a value in a function that returns void

>+  if (!mSelectControl)
>+    return;

This is to deal with anoying addons etc that decide to implement xul selects themselves?

>+nsXULMenubarAccessible::SetCurrentItem(nsAccessible* aItem)
>+{
>+  // XXX: not implemented

I tend to think if this actually should have an implementation that does something we should have a warning here.

>+nsXULTreeAccessible::SetCurrentItem(nsAccessible* aItem)
>+{
>+  // XXX: not implemented

same

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> >+  if (!mSelectControl)
> >+    return;
> 
> This is to deal with anoying addons etc that decide to implement xul selects
> themselves?

yes

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a2ed4e0b009