Last Comment Bug 406214 - Dialogs with checkbox have focus on checkbox rather than on the default button
: Dialogs with checkbox have focus on checkbox rather than on the default button
Status: VERIFIED FIXED
: access, regression, verified1.8.1.12
Product: Core
Classification: Components
Component: XUL (show other bugs)
: 1.8 Branch
: x86 Windows 2000
: -- normal with 3 votes (vote)
: ---
Assigned To: Steven Michaud [:smichaud] (Retired)
:
:
Mentors:
: 406309 (view as bug list)
Depends on:
Blocks: 397707
  Show dependency treegraph
 
Reported: 2007-11-30 12:51 PST by Sander
Modified: 2008-01-22 18:00 PST (History)
19 users (show)
dveditz: blocking1.8.1.12+
martijn.martijn: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
workaround patch (1.08 KB, patch)
2007-12-04 06:12 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
workaround patch2 (1.02 KB, patch)
2007-12-04 07:55 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
zipped testcase (8.19 KB, application/zip)
2007-12-04 08:44 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Event-handler patch (2.37 KB, patch)
2007-12-10 09:33 PST, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
Event-handler patch, rev1 (2.41 KB, patch)
2007-12-11 19:06 PST, Steven Michaud [:smichaud] (Retired)
bugs: review+
jst: superreview+
dveditz: approval1.8.1.12+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Sander 2007-11-30 12:51:25 PST
Any type of dialog which contains a checkbox (for example the "confirm close" box if you have more than one tab open in a window) has the focus set on the checkbox, rather than on the default button. Hitting the space bar, which would ordinarily submit the default button, now merely toggles the checkbox.
Seen with SeaMonkey 1.1.7 - 20071128. I'm pretty certain this is a regression since 1.1.6 (20071030). ajschult has offered to track down the exact regression range.
Comment 1 Andrew Schultz 2007-12-01 00:47:37 PST
Unfortunately, I don't see this bug on linux with 1.1.7 or current branch CVS.  The OK button has focus.
Comment 2 Sander 2007-12-01 03:32:22 PST
Ok, so this is windows only (maybe Mac too?). It's not present on trunk or the 1.8.0 branch. Regressed on the 1.8 branch between 2007110708 and 2007111309. Unfortunately there's no windows SeaMonkey build from within that time period, so I can't narrow it down any further. :( 

Ooh, but Firefox suffers from this, too! Cool, so that makes the regression range 2007110708 to 2007110803.

The only thing from that checkin range which looks like it might be vaguely related would be bug 397707 - which unfortunately is inaccessible. Steven: is this something you could've caused?
Comment 3 Jeremy Baron 2007-12-01 04:28:20 PST
different observation of same set of symptoms (I think) with same regression range.

2 focus rings on same dialog and the element selected by default has changed

STR:
Open mutliple tabs.
Hit ctrl-q or the window's X button in the upper right corner.

Actual:
2 focus rings appear and the default action of the spacebar is toggling the checkbox.

Expected:
1 focus ring
(and previously the default action of the spacebar was canceling the quit.  not sure if that was intended but it's probably best to be consistent post-release unless there's a good argument for changing it)

Don't have win, so I haven't seen it myself. (afaict the mac default was always checkbox and I only see one focus ring I think.  It's a little hard to tell with ghost focus rings from widget:mac)  grewy has seen it on both XP and vista and provided that regression range and Lucy saw it on vista.
Comment 4 Jesse Ruderman 2007-12-01 04:35:11 PST
*** Bug 406309 has been marked as a duplicate of this bug. ***
Comment 5 Steven Michaud [:smichaud] (Retired) 2007-12-01 08:15:03 PST
(In reply to comment #2)

> Steven: is this something you could've caused?

Yes, it's possible.

This doesn't happen on the Mac.  I'll test on Windows and Linux on
Monday (after I get home).  If I can reproduce it on either, I'll try
reversing my bug 397707 patch, to see if this makes any difference.
Comment 6 bugzilla_moz.20.ibyte 2007-12-02 07:47:00 PST
I first noticed the check box having default focus rather than the default button on FF 2.0.0.10 (Windows), and I hate it, as now you can't use e.g. a forum post preview button any more by tapping the space bar twice (assuming you prefer to have form submission confirmation on, which I do).

Still exists on:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Comment 7 Jeremy Baron 2007-12-03 15:50:42 PST
cc mats per aaronlev's suggestion
Comment 8 therube 2007-12-03 22:08:14 PST
Slightly different, or perhaps a bit broader in scope then originally described ...

In SeaMonkey, Bookmarks Manager (Ctrl+B) | Find (Ctrl+F)

In the Find dialog, focus is placed in the 'name' field rather then in the search criteria field.

Closest range I can come up with is 2007-11-07-08-mozilla1.8 to 2007-11-13-09-mozilla1.8

There is a check box, as described, but default focus appears not to be set there.  So this could be different again?
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-04 04:36:20 PST
Yes, this was caused by the fix for bug 397707.
I have a 1.8.1 Firefox build. Before I applied the patch from that bug, I didn't see this bug. After I applied the patch, I can see this bug.
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-04 06:12:19 PST
Created attachment 291425 [details] [diff] [review]
workaround patch

This seems to make it work again, but this is obviously a workaround (I really don't like the focus code in dialog.xml and commonDialog.js)

It seems that focusedElt is null here where it used to give the focused element:
http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/dialog.xml#164
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-04 07:07:03 PST
(In reply to comment #10)
> It seems that focusedElt is null here where it used to give the focused
> element:
> http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/widgets/dialog.xml#164

Sorry, never mind, this is incorrect.
When I put an alert(focusedElt.nodeName) in there, I get 'xul:button'.
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-04 07:55:11 PST
Created attachment 291438 [details] [diff] [review]
workaround patch2

(In reply to comment #11)
> Sorry, never mind, this is incorrect.
> When I put an alert(focusedElt.nodeName) in there, I get 'xul:button'.

Sorry, this is also incorrect, with a window.dump(focusedElt.nodeName), I get 'checkbox' as result. That makes more sense of why this is happening and why this workaround patch2 works.

Actually, it seems to me that the code in focusInit() shouldn't be called at all because document.commandDispatcher.focusedElement should not be null, because a button gets focused here:
http://lxr.mozilla.org/mozilla1.8/source/toolkit/content/commonDialog.js#174
which happens before focusInit(), afaict.
Comment 13 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-04 08:12:04 PST
Ok, I rechecked that. After backing out the patch from bug 397707 again, document.commandDispatcher.focusedElement is again not null, because the code in focusInit() isn't called.
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-12-04 08:44:39 PST
Created attachment 291443 [details]
zipped testcase

I need chrome privs to get a working testcase that reproduces the issue, so it's a bit complicated to get it set up, I'm afraid.

- Unzip the attached zipped testcase.
- Install the "testdirectory@testdirectory.xpi" extension and restart Firefox
- Create a directory in C:\, named "testfiles"
- Copy the files "opendialog.xul" and "hhhh.xul" to that directory.
- Open up "chrome://tests/content/opendialog.xul"
- Click on the "opendialog" button

Expected result:
Dialog opens, only one button is shown, no red block with text should be shown after 500ms.

Actual result:
After 500ms, a red block with the text 'You should not see this text' is shown in the dialog.
Comment 15 Steven Michaud [:smichaud] (Retired) 2007-12-04 10:23:01 PST
I've found the part of my patch for bug 397707 that triggers this bug
(I'll post a comment in bug 397707 that identifies it).

It's only three lines, and isn't an essential part of the patch.  But
it prevents a nasty problem in Camino, so I can't just get rid of it.

While Martijn tries to work around this problem from the Gecko side,
I'll try to work around it from the focus-event-handling side (by
fiddling with code in content/events/src/nsEventStateManager.cpp).

But this won't be easy, and I'm afraid it may take a while.

This bug isn't as serious as it first appeared -- in the "Confirm
Close" dialog, the focus seems (in some sense) to be in both the
checkbox and the "Close Tabs" button at the same time.  So, though the
spacebar does toggle the checkbox (when it should trigger the "Close
Tabs" button), hitting Return/Enter still triggers the "Close Tabs"
button (as it should).

I hope this means that this bug isn't drop-dead urgent :-)
Comment 16 Steven Michaud [:smichaud] (Retired) 2007-12-04 10:32:50 PST
The same three-line change also makes Martijn's testcase (from comment
#14) work properly.
Comment 17 Steven Michaud [:smichaud] (Retired) 2007-12-04 12:00:50 PST
(Following up comment #15)

> I've found the part of my patch for bug 397707 that triggers this
> bug (I'll post a comment in bug 397707 that identifies it).
>
> It's only three lines, and isn't an essential part of the patch.
> But it prevents a nasty problem in Camino, so I can't just get rid
> of it.

I was wrong.  The trigger is the four lines right above those three
lines, which _are_ an essential part of my patch.
Comment 18 Steven Michaud [:smichaud] (Retired) 2007-12-10 09:33:45 PST
Created attachment 292429 [details] [diff] [review]
Event-handler patch

After a couple of days spent in a futile search for something more
elegant, here's the obvious solution to this problem.

Yes, it's a bit ugly (with that OS-specific #ifdef).  But then so is a
lot of the rest of the focus code.  And both this patch and my patch
for bug 397707 are bandaids, intended just to get things working "well
enough".  A more through review of the focus code will take much
longer, and should (mostly) be targeted at Mozilla 1.9 (i.e. Firefox
3) and Mozilla 2.0.

As you'll see in the patch comment, the Windows problem is triggered
by something in another OS-specific #ifdef block (compiled only on
Windows and OS/2).  So in principle I might have made this patch's
#ifdef block cause the code to be used on everything _except_ Windows
and OS/2.

But I have no indication that my patch for bug 397707 does any good on
any OS except for OS X.  So I chose an #ifdef that makes part of it
(basically the part that triggers this bug (406214) on Windows)
compile only on OS X.

Here are tryserver builds, built with this patch, for OS X and Linux.
Of course I'd really like to also post a Windows build, but the
tryservers can't do Windows builds on the 1.8 branch (for more info
see bug 400881).  In the meantime I'm looking for a home for my own
Windows build (the one I tested with) -- I hope to find one in the
next couple of days.

https://build.mozilla.org/tryserver-builds/2007-12-07_16:26-smichaud@pobox.com-bugzilla406214/
Comment 19 Steven Michaud [:smichaud] (Retired) 2007-12-10 09:36:26 PST
Comment on attachment 292429 [details] [diff] [review]
Event-handler patch

Olli, I've asked you to review this patch because you reviewed my
patch for bug 397707.  Please let me know if you think someone else
should do the review.
Comment 20 Steven Michaud [:smichaud] (Retired) 2007-12-10 09:41:08 PST
Side note:  The Linux tryserver build depends on Pango, and won't run
without it (or on systems who's Pango version is too old).  It should
work fine on recent Linux distros (e.g. SuSE Linux 10.1).  But it
won't run on distros that are a few years old (e.g. SuSE Linux 9.2).
Comment 21 Olli Pettay [:smaug] 2007-12-11 16:30:08 PST
Note, I don't have Windows-dev-machine to test the patch.

> As you'll see in the patch comment, the Windows problem is triggered
> by something in another OS-specific #ifdef block (compiled only on
> Windows and OS/2).  So in principle I might have made this patch's
> #ifdef block cause the code to be used on everything _except_ Windows
> and OS/2.
Perhaps do that and explain properly why and somehow link to the other
#ifdef
Comment 22 Steven Michaud [:smichaud] (Retired) 2007-12-11 16:46:42 PST
> Perhaps do that and explain properly why

I don't understand.  Are you really asking me to make the following
change to my patch?

-#if defined (XP_MAC) || defined(XP_MACOSX)
+#if !defined(XP_WIN) && !defined(XP_OS2)

If so, please tell me why.

> and somehow link to the other #ifdef

I've already done that (in my patch comment).  Was what I said not
clear enough?
Comment 23 Olli Pettay [:smaug] 2007-12-11 16:53:55 PST
I understood your comment so that
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp&rev=1.595.2.29&750-830#740
is part of the problem, and that is XP_WIN/XP_OS2 only.

Btw, could you find someone with OS2 to test the patch ;)


> I've already done that (in my patch comment).  Was what I said not
> clear enough?
To me it wasn't clear that there is really some #ifdefed code in ::PreHandle
Comment 24 Steven Michaud [:smichaud] (Retired) 2007-12-11 19:06:05 PST
Created attachment 292715 [details] [diff] [review]
Event-handler patch, rev1

Alright, then.  Here's a revision that follows your suggestions, Olli.
Please let me know what you think.

Later I'll post links to new OS X and Linux tryserver builds (though
my revision will only make any difference on the Linux build).

I'm still working to find a home for my Windows tryserver builds.

> Btw, could you find someone with OS2 to test the patch ;)

There's a mozilla.dev.ports.os2 newsgroup that has current messages
(from this month!).  I suppose I could post a message there.
Comment 25 Steven Michaud [:smichaud] (Retired) 2007-12-11 19:47:49 PST
Here are OS X and Linux tryserver builds for rev1 of my patch
(attachment 292715 [details] [diff] [review]):

https://build.mozilla.org/tryserver-builds/2007-12-11_18:41-smichaud@pobox.com-bugzilla406214-rev1/
Comment 26 Olli Pettay [:smaug] 2007-12-13 06:36:08 PST
Comment on attachment 292715 [details] [diff] [review]
Event-handler patch, rev1

This looks ok, but must be tested well. Does this change any behavior described in Bug 397707.
That bug is for OSX, but does Windows (and OS2) properly. 
Though, at least Windows should behave like before Bug 397707.
Comment 27 Steven Michaud [:smichaud] (Retired) 2007-12-13 08:44:00 PST
> but must be tested well

Yes.  I've done what I can.  But (as before) most testing will only
happen after the patch has been landed.  Especially on the more
obscure platforms like OS/2.

(I'll keep trying to find a home for my Windows builds ... but at this
point I'm not terribly hopeful.)

> Does this change any behavior described in Bug 397707.

In my tests, aside from this bug (406214) getting fixed (at least on
Windows), the behavior is exactly the same on all platforms.
Comment 28 Steven Michaud [:smichaud] (Retired) 2007-12-13 08:48:49 PST
> all platforms

Windows, OS X, Linux.
Comment 29 bugzilla_moz.20.ibyte 2007-12-17 10:46:44 PST
I did not observe this bug on Firefox 3 Beta 1, WinXP Home SP2
(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1)
Comment 30 Steven Michaud [:smichaud] (Retired) 2007-12-17 11:06:39 PST
This bug only happens on the 1.8 branch (e.g. Firefox 2.0.0.10 and
2.0.0.11).
Comment 31 neil@parkwaycc.co.uk 2007-12-19 15:57:44 PST
Is this perhaps really a Windows widget bug? When nsXULWindow tries to show the dialog the OS blurs the previous window and the windows nsWindow seems to think that Gecko is losing focus and manages to dispatch an event that blurs the current element in the dialog, which sounds wrong...
Comment 32 neil@parkwaycc.co.uk 2007-12-19 16:38:00 PST
OK, so I won't bore you with a full stack trace, but this is what happens:
Ths dialog's document finishes loading. This triggers the XULWindow to show itself, which the widget code does by calling SetWindowPos to bring the window to the front and show and activate the window. This causes the OS to blur the parent window. This blur is detected by the widget code, but because it doesn't have the fix to bug 265383 it thinks that you're opening an embedding or native dialog and immediately blurs the current element. This confuses any code such as the common dialog that tries to set the focus during the load event. See:
nsWindow::ProcessMessage (case WM_KILLFOCUS)
nsEventStateManager::PreHandleEvent (case NS_LOSTFOCUS)
Comment 33 Steven Michaud [:smichaud] (Retired) 2007-12-19 17:21:42 PST
Neil, what you say may be right (though I don't know enough about
Windows widgets to be able to say).  But if my simple change fixes the
problem (even if it just papers it over), isn't that a better
solution?

Right now most development efforts are devoted to what will become
Firefox 3 (the trunk, aka Mozilla 1.9), or to what will come after
that (the so-called Mozilla 2.0).  I don't think you'll find too many
Windows developers eager to spend their time giving this bug a
comprehensive solution, unless doing so also resolves a lot of
critical problems.

If Windows developers (those who'd be doing this work) think I'm
wrong, please chime it!
Comment 34 Steven Michaud [:smichaud] (Retired) 2007-12-19 17:22:30 PST
> chime it

chime in
Comment 35 neil@parkwaycc.co.uk 2007-12-20 02:04:07 PST
I guess it depends on whether this bug actually manifests on OS/2 or not; if it does, which IMHO isn't clear from the comments, then your fix is better.
Comment 36 Steven Michaud [:smichaud] (Retired) 2007-12-20 14:49:25 PST
I'm not going to be able to build or test in OS/2.

My current plan is to wait until this patch lands on the trunk, then
post a message to the mozilla.dev.ports.os2.  I don't know if whoever
does the OS/2 port does nightly builds (or weekly or monthly builds).
If they do, landing the patch on the trunk would be the easiest way to
get it into one of these builds, and to get it tested.
Comment 37 Steven Michaud [:smichaud] (Retired) 2007-12-20 14:50:26 PST
> lands on the trunk

Oops, should be "lands on the 1.8 branch".
Comment 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-21 10:52:46 PST
(In reply to comment #36)
> I'm not going to be able to build or test in OS/2.

Perhaps Peter can help with that, without requiring you to land it on trunk beforehand?

Comment 39 Peter Weilbacher 2007-12-21 15:19:01 PST
Thanks for thinking of me, Gavin.

I tested SeaMonkey 1.1.6 and 1.1.7, just by pressing Ctrl-Q, and I saw that the focus was always on the "Close all tabs" button, never on the checkbox. So I mostly convinced that we don't have this specific problem on OS/2. Will ask in the newsgroup now to make sure.

Btw, Steven, for the next time: the earlier you post to the OS/2 newsgroup the earlier we can test. We like to hear about bugs/patches that could affect our platform. :-)
Comment 40 Steven Michaud [:smichaud] (Retired) 2007-12-21 16:44:07 PST
> Btw, Steven, for the next time: the earlier you post to the OS/2
> newsgroup the earlier we can test. We like to hear about
> bugs/patches that could affect our platform. :-)

I will.

Do you have nightly (or weekly or monthly) builds of the 1.8 branch?
(Or for that matter of the trunk?)

Can you do a build with this bug's current patch (attachment 292715 [details] [diff] [review]),
and make it available for people to download and test?

It's almost impossible to get people to test patches without something
they can download.  (It's pretty difficult even with downloads.)
Worries about things like this are part of the reason I haven't yet
posted to the mozilla.dev.ports.os2 newsgroup.
Comment 41 Steven Michaud [:smichaud] (Retired) 2007-12-21 16:47:57 PST
(By the way, my questions/requests aren't urgent.  I'm about to go on
Christmas vacation, and will be away from the Internet until early
January.)
Comment 42 Peter Weilbacher 2007-12-23 14:46:09 PST
If I don't do it now I will forget... So I tried it with SeaMonkey 1.1.7 and Firefox 2.0.0.11. Both act normally with and without the patch. I watched for the focus of the quit confirmation dialog and also tried Martijn's testcase from comment 14. I now also provide a DLL for testing with SM for others. In case you don't read any more from me here, nobody reported any problems with that.
Comment 43 Steven Michaud [:smichaud] (Retired) 2008-01-04 14:39:10 PST
Thanks, Peter.  So it appears that this bug isn't an issue on OS/2,
and that the patch for it doesn't cause problems, either.

(In reply to comment #32 and comment #35)

I still think the best way to fix this problem is to land my patch
(attachment 292715 [details] [diff] [review]) on the 1.8 branch.  It's certainly the simplest.

(If we were working on the trunk, I agree that it'd be better to try
to get closer to the problem's actual source.  But we're not ... at
least with regard to this bug.)

jst, are you back from your holidays?  Can you spare some time to sr
my patch (attachment 292715 [details] [diff] [review])? :-)
Comment 44 Steven Michaud [:smichaud] (Retired) 2008-01-15 08:26:59 PST
Landed on 1.8 branch.
Comment 45 Samuel Sidler (old account; do not CC) 2008-01-15 09:25:43 PST
Comment on attachment 292715 [details] [diff] [review]
Event-handler patch, rev1

Steven, patches on the 1.8 branch require explicit approval before being checked in. Though this will likely be approved, please keep this in mind in the future.
Comment 46 Steven Michaud [:smichaud] (Retired) 2008-01-15 09:34:20 PST
(In reply to comment #45)

Silly me.  I _did_ request approval, but for the "1.9 branch" :-(

(Needless to say, this bug is 1.8-branch only.)
Comment 47 Daniel Veditz [:dveditz] 2008-01-15 15:39:34 PST
Comment on attachment 292715 [details] [diff] [review]
Event-handler patch, rev1

approved for 1.8.1.12, a=dveditz for release-drivers
Comment 48 Al Billings [:abillings] 2008-01-22 17:19:13 PST
Looking through the comments, things seem to meander a bit. Do we have a clean testcase for this or repro steps to show it on Windows with Firefox?
Comment 49 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-01-22 18:00:52 PST
Verified fixed, with the "confirm close" dialog when multiple tabs open and with the testcase and steps to reproduce in comment 14, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080122 BonEcho/2.0.0.12pre

I guess the testcase of comment 14 could be converted in some automated testcase, but it needs to have chrome context.

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