441233 - Missing focus events in Thunderbird account wizard dialog

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Delete any existing ~/.thunderbird
2. Launch Accerciser and turn event monitoring on.
3. Launch Thunderbird causing the Account wizard to appear.

Expected results:  Because a radio button has focus, a focus: event and an
object:state-changed:focused event would each be issued for the focused radio
button.

Actual results: No focus events are issued for the focused radio button.

Note:  This was originally reported as bug #388187.  While the other dialogs mentioned were fixed, it seems that the Account Wizard dialog was not.

Comment 1

[Marco Zehe (:MarcoZ)]

Hi Ginn, since you were the author of the above quoted bug fix, could you take a look at this one? Thanks!

Comment 2

[Ginn Chen]

Macro, can you reproduce it on Windows?

Comment 3

[Marco Zehe (:MarcoZ)]

(In reply to comment #2)
> Macro, can you reproduce it on Windows?

No I can't. This really looks similar to the other bug Joanie mentioned, which was also Linux-specific.

Comment 4

[Damon Sicore (:damons)]

Since 1.9.1 is a time based release, I don't think we'd stop for this issue.  However, it is definitely wanted if someone were to provide a patch.  Please argue if you feel differently.
 
wanted1.9.1+ P3
blocking1.9.1-

Comment 5

[Ginn Chen]

nsWindow.cpp is platform specific, it's possible that the focus event comes through without activate the window. (I don't understand the code.)

 2208       DispatchGotFocusEvent();
 2209   
 2210       // send the activate event if it wasn't already sent via any
 2211       // SetFocus() calls that were the result of the GOTFOCUS event
 2212       // above.
 2213       if (mActivatePending) {
 2214           mActivatePending = PR_FALSE;
 2215           DispatchActivateEvent();
 2216       }

We create root accessible and add event listener when activate event is dispatched.
So if focus event is earlier, we will miss it.

Perhaps we can fire a focus event when we get activate event.

BTW: Press Alt+Tab to switch to another window, and switch back can be a workaround.

Comment 6

[Ginn Chen]

Attachment: patch

By the way, we won't get duplicate focus events because at-spi remembers last focused object.

Comment 7

[alexander :surkov (:asurkov)]

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


>         MAI_LOG_DEBUG(("\n\nReceived: EVENT_WINDOW_ACTIVATED\n"));
>-        nsDocAccessibleWrap *accDocWrap =
>-          static_cast<nsDocAccessibleWrap *>(accessible.get());
>-        accDocWrap->mActivated = PR_TRUE;
>+        nsRootAccessible *rootAcc =
>+          static_cast<nsRootAccessible *>(accessible.get());
>+        rootAcc->mActivated = PR_TRUE;
>         guint id = g_signal_lookup ("activate", MAI_TYPE_ATK_OBJECT);
>         g_signal_emit(atkObj, id, 0);
>+        rootAcc->FireCurrentFocusEvent();

would be nice to add comment to describe this change.
>       */
>     PRBool FireAccessibleFocusEvent(nsIAccessible *aFocusAccessible,
>                                     nsIDOMNode *aFocusNode,
>                                     nsIDOMEvent *aFocusEvent,
>                                     PRBool aForceEvent = PR_FALSE,
>                                     PRBool aIsAsynch = PR_FALSE);
> 
>     nsCaretAccessible *GetCaretAccessible();
>+    void FireCurrentFocusEvent();

nit: it makes sense to move up the method to group similar methods. Also please add java-style comment.

Comment 8

[Ginn Chen]

Pushed in with Surkov's comments addressed.

http://hg.mozilla.org/mozilla-central/index.cgi/rev/4641b7f25326190c4a12d3820c5d2f58d8c574bf

Comment 9

[Mats Palmgren (inactive)]

Hmm, firing DOM focus events from a11y code doesn't seem right to me...
Focus handling should work independently of a11y, otherwise there will be
bugs with --disable-accessibility

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, that sounds like a serious problem with this setup.  Shouldn't there be sr happening for a11y changes that affect the DOM like this one does?

Comment 11

[Brendan Eich [:brendan]]

accessible/... changes still need SR, as far as I know.

/be

Comment 12

[Ginn Chen]

I think nsRootAccessible::FireCurrentFocusEvent() would not send the DOM focus events outside accessible module.
It simulates a DOM focus event, call nsRootAccessible::HandleEventWithTarget() to send it as an a11y focus event.

Comment 13

[Mats Palmgren (inactive)]

You're right, I misread the code. My apologies.