Closed Bug 388187 Opened 13 years ago Closed 13 years ago
Incorrect initial focus event when dialogs are opened
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.
Joanie, does this occur for all XUL radio buttons -- e.g. in Firefox as well?
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!
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
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?
OS: Linux → All
Hardware: PC → All
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).
This is fixed in the Firefox page setup dialog as well as the import settings dialog. Please reopen if you still have issues.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
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.
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.
Might want to try and do the mFireFocusTimer->InitWithFuncCallback() in a "pageshow" handler in nsRootAccessible. But, only for root accessibles that are shown.
Assignee: aaronleventhal → ginn.chen
Status: REOPENED → NEW
Yes, the focus event is fired before the dialog window is displayed. comment #11 sounds good. I'll try.
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.
It's a good idea Ginn. Marco, can you test with this patch and see if it breaks anything on Windows?
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)
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.
(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.
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 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
Attachment #282867 - Flags: review?(ginn.chen) → review-
Attachment #283162 - Flags: review?(aaronleventhal) → review+
Summary: Focused radio button should issue focus events when dialogs appear → Incorrect initial focus event when dialogs are opened
Attachment #283162 - Flags: approval1.9? → approval1.9+
Ginn, we seem to think this is fixed? Should it be marked RESOLVED FIXED?
(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. >
Status: NEW → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Fix checked in, sorry for the extra noise.
You need to log in before you can comment on or make changes to this bug.