1.08 KB, patch
|Details | Diff | Splinter Review|
1.02 KB, patch
|Details | Diff | Splinter Review|
8.19 KB, application/zip
2.41 KB, patch
Mike Schroepfer: approval1.9+
|Details | Diff | Splinter Review|
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.
Unfortunately, I don't see this bug on linux with 1.1.7 or current branch CVS. The OK button has focus.
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?
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.
(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.
I first noticed the check box having default focus rather than the default button on FF 18.104.22.168 (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:22.214.171.124) Gecko/20071127 Firefox/126.96.36.199
cc mats per aaronlev's suggestion
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?
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.
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
(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'.
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.
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.
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 "email@example.com" 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.
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 :-)
The same three-line change also makes Martijn's testcase (from comment #14) work properly.
(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.
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:firstname.lastname@example.org/
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.
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).
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
> 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?
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
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.
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:email@example.com/
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.
> 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.
> all platforms Windows, OS X, Linux.
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)
This bug only happens on the 1.8 branch (e.g. Firefox 188.8.131.52 and 184.108.40.206).
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...
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)
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!
> chime it chime in
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.
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.
> lands on the trunk Oops, should be "lands on the 1.8 branch".
(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?
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. :-)
> 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.
(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.)
If I don't do it now I will forget... So I tried it with SeaMonkey 1.1.7 and Firefox 220.127.116.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.
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])? :-)
Landed on 1.8 branch.
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.
(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 on attachment 292715 [details] [diff] [review] Event-handler patch, rev1 approved for 18.104.22.168, a=dveditz for release-drivers
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?
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:22.214.171.124pre) Gecko/20080122 BonEcho/126.96.36.199pre I guess the testcase of comment 14 could be converted in some automated testcase, but it needs to have chrome context.