Make takeFocus work on widget items

RESOLVED FIXED in mozilla11

Status

()

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

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Depends on: 1 bug, {access})

unspecified
mozilla11
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

17.98 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Currently takeFocus works on tree items only. We need to support other widgets.
(Assignee)

Comment 1

6 years ago
Created attachment 577557 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #577557 - Flags: review?(trev.saunders)
Attachment #577557 - Flags: review?(marco.zehe)

Comment 2

6 years ago
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.
Attachment #577557 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 3

6 years ago
(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 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
(Assignee)

Comment 5

6 years ago
(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
(Assignee)

Comment 6

6 years ago
Created attachment 577875 [details] [diff] [review]
patch2
Attachment #577557 - Attachment is obsolete: true
Attachment #577557 - Flags: review?(trev.saunders)
Attachment #577875 - Flags: review?(trev.saunders)
Attachment #577875 - Flags: review?(trev.saunders) → review+

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1a2ed4e0b009
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Flags: in-testsuite+
Depends on: 718237
You need to log in before you can comment on or make changes to this bug.