Last Comment Bug 706067 - Make takeFocus work on widget items
: Make takeFocus work on widget items
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: alexander :surkov
:
Mentors:
Depends on: 718237
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-29 05:09 PST by alexander :surkov
Modified: 2012-01-15 10:51 PST (History)
1 user (show)
mzehe: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.86 KB, patch)
2011-11-29 05:17 PST, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review
patch2 (17.98 KB, patch)
2011-11-29 23:57 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-11-29 05:09:13 PST
Currently takeFocus works on tree items only. We need to support other widgets.
Comment 1 alexander :surkov 2011-11-29 05:17:20 PST
Created attachment 577557 [details] [diff] [review]
patch
Comment 2 Marco Zehe (:MarcoZ) on PTO until August 15 2011-11-29 06:16:58 PST
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 2011-11-29 06:21:55 PST
(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) 2011-11-29 18:49:47 PST
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 2011-11-29 23:55:56 PST
(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 2011-11-29 23:57:00 PST
Created attachment 577875 [details] [diff] [review]
patch2
Comment 7 Ed Morley [:emorley] 2011-12-09 07:09:00 PST
https://hg.mozilla.org/mozilla-central/rev/1a2ed4e0b009

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