388187 - Incorrect initial focus event when dialogs are opened

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Launch Accerciser and turn event monitoring on.
2. Launch Thunderbird
3. Get into any of the following dialog boxes:
  a. Account wizard
  b. Import wizard in address book
  c. Page setup dialog

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.

Notes: This only seems to be the case when the dialog initially appears.  If the dialog box loses focus, it will issue the correct events for the focused radio button when it regains focus.  Also, when a radio button is the focused item in subsequent steps in the "wizards" (i.e. after you press the Next button) it issues the correct events.

Comment 1

[Aaron Leventhal]

Joanie, does this occur for all XUL radio buttons -- e.g. in Firefox as well?

Comment 2

[Joanmarie Diggs]

It does in the Page Setup dialog -- but in Firefox that is identical to the dialog in Thunderbird, so it's not all that surprising. :-)

I cannot find any other dialogs in Firefox where a radio button has focus as soon as the dialog appears, so it's hard to say.  Sorry!

Comment 3

[Aaron Leventhal]

Note to Mozilla developers:
It's probably because nsRootAccessible::FireCurrentFocusEvent() uses nsAccessNode::GetCurrentFocus(), and it's the radiogroup which Mozilla believes has focus. In the normal case we fire a RadioStateChange in radio.xml's selectedItem setter, and that gets turned into an accessible focus event. In this case, the radiogroup already starts out with that radio item selected, so this method is not called.

I can think of 3 ways of fixing this:
1) Set aaa:activedescendant on radiogroup, but I think that would require an anonid on each radio item
2) In the constructor for radio group search for a selected item and fire RadioStateChange for it. Hopefully that wouldn't cause any regressions with radiogroups that dynamically change the selected item when the radiogroup is first displayed
3) Change FireCurrentFocusEvent(), GetCurrentFocus() or FireAccessibleFocusEvent() to take XUL radiogroups into account, but that seems like a hack

Comment 4

[Aaron Leventhal]

Actually looks like I'm wrong, the problem is probably elsewhere.

This is broken in Firefox 3 on Windows as well, but it used to work in Firefox 2.

What I'm seeing is:
1) Focus event on radio button
2) Focus event on the HTML document that was previously focused

Joanie, are you seeing that on Linux?

Comment 5

[Aaron Leventhal]

Joanie, if that's what you're seeing, then the question is why the extra focus event is fired (not why the radio button isn't firing focus events).

Comment 6

[Aaron Leventhal]

This is fixed in the Firefox page setup dialog as well as the import settings dialog.

Please reopen if you still have issues.

Comment 7

[Joanmarie Diggs]

Oops, I hadn't seen your comments in July.  I guess I need to modify my filters to look for my name in bugzilla mail.  Sorry!!

I just tried it with the latest trunk and the FF Page Setup dialog (opened via Alt+F, U).  When doing so I get a focus: event for the File menu, one for the New Window menu item, and then one for the Document Frame that was previous focused. I am still not getting a focus: event for the Portrait radio button which has focus.  I see parallel object:state-changed:focused events (i.e. I get them for the menu, menu item and document frame, but not the radio button).

Reopening.

Comment 8

[Aaron Leventhal]

Hmm, I'm not seeing this bug on Windows anymore as I used to, either with a debug or a standard release build. I'll have to look on Linux when I get to the office next week.

Comment 9

[Aaron Leventhal]

I think this is broken (on Linux only) for the initial focus in any dialog.

If I increase the timeout to 1000 for FireFocusCallback() in nsRootAccessible then the bug is fixed on my system.

But, that is arbirtrary, I'd like to base things off of a better notification when the dialog is really ready.

Comment 10

[Aaron Leventhal]

Anyway, we fire that timeout whenever a new root accessible is created because a new window is being created. There's a timeout (normally length 0) because focus isn't ready yet. Anyway, the timeout isn't working.

Comment 11

[Aaron Leventhal]

Might want to try and do the 
mFireFocusTimer->InitWithFuncCallback() in a "pageshow" handler in nsRootAccessible. But, only for root accessibles that are shown.

Comment 12

[Ginn Chen]

Yes, the focus event is fired before the dialog window is displayed.

comment #11 sounds good. I'll try.

Comment 13

[Ginn Chen]

Attachment: patch

Aaron, is it correct?
Does it work on Windows?

On Linux, activate event is right after focus event for nsWindow.
See nsWindow::SetFocus()

BTW:
I didn't do much testing with this patch yet, and I may not have a chance to do it in next week.

Comment 14

[Aaron Leventhal]

It's a good idea Ginn.

Marco, can you test with this patch and see if it breaks anything on Windows?

Comment 15

[Aaron Leventhal]

Attachment: Ginn's fix, with some tweaks

Ginn, this was a good idea. Since you are not available as much next week, I hope you don't mind that I've posted what I think is an improvement.

Three changes:
1) Move to another place in code that already deals with focus returning to the root accessible, so that it can all be in one place
2) More specific comments
3) Don't recreate mFireFocusTimer if it already exists (this happens for example if you Alt+Tab to another window and Alt+Tab back)

Comment 16

[Aaron Leventhal]

Marco, please test with the new patch instead, and if you don't mind please test it on Linux as well to see if it still fixes the original bug.

Comment 17

[Marco Zehe (:MarcoZ)]

(In reply to comment #16)
> Marco, please test with the new patch instead, and if you don't mind please
> test it on Linux as well to see if it still fixes the original bug.

Unfortunately, it doesn't fix the original bug on Linux. On Windows, I don't notice any difference in behaviour. While Ginn's patch fixes the bug on Linux, yours doesn't.

Comment 18

[Ginn Chen]

Aaron, your patch looks good.
But I need to debug into it to figure out why it doesn't work.

I'll find some time to do that tomorrow.

Comment 19

[Ginn Chen]

Comment on attachment 282867 [details] [diff] [review]
Ginn's fix, with some tweaks

GetCurrentFocus() doesn't work here.
It doesn't give us what we want.

In nsRootAccessible::FireAccessibleFocusEvent(),
we can use (aNode == mDOMNode), or we can move the code back to nsRootAccessible::HandleEventWithTarget

Comment 20

[Ginn Chen]

Attachment: patch v2

Comment 21

[Aaron Leventhal]

Ginn, we seem to think this is fixed? Should it be marked RESOLVED FIXED?

Comment 22

[Aaron Leventhal]

(In reply to comment #0)
> Steps to reproduce:
> 
> 1. Launch Accerciser and turn event monitoring on.
> 2. Launch Thunderbird
> 3. Get into any of the following dialog boxes:
>   a. Account wizard
>   b. Import wizard in address book
>   c. Page setup dialog
> 
> 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.
> 
> Notes: This only seems to be the case when the dialog initially appears.  If
> the dialog box loses focus, it will issue the correct events for the focused
> radio button when it regains focus.  Also, when a radio button is the focused
> item in subsequent steps in the "wizards" (i.e. after you press the Next
> button) it issues the correct events.
> 

Comment 23

[Aaron Leventhal]

Fix checked in, sorry for the extra noise.