Closed Bug 406214 Opened 17 years ago Closed 17 years ago

Dialogs with checkbox have focus on checkbox rather than on the default button

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Version:
1.8 Branch
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: svl-bmo, Assigned: smichaud)

References

Details

(Keywords: access, regression, verified1.8.1.12)

Attachments

(4 files, 1 obsolete file)

workaround patch
1.08 KB, patch
Details | Diff | Splinter Review
workaround patch2
1.02 KB, patch
Details | Diff | Splinter Review
zipped testcase
8.19 KB, application/zip
Details
Event-handler patch, rev1
2.41 KB, patch
smaug
: review+
jst
: superreview+
dveditz
: approval1.8.1.12+
mtschrep
: 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?
Assignee: guifeatures → jag
Component: XP Apps: GUI Features → XP Toolkit/Widgets
Product: Mozilla Application Suite → Core
QA Contact: xptoolkit.widgets
Version: SeaMonkey 1.1 Branch → 1.8 Branch
Flags: blocking1.8.1.12?
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.
Keywords: access
(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 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
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.
Blocks: 397707
Attached patch workaround patch — — Splinter Review 
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'.

(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.
Attached file 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.
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.
Attached patch Event-handler patch (obsolete) — — Splinter Review 
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/
Assignee: jag → smichaud
Status: NEW → ASSIGNED
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.
Attachment #292429 - Flags: review?(Olli.Pettay)
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

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.
Attachment #292429 - Attachment is obsolete: true
Attachment #292715 - Flags: review?(Olli.Pettay)
Attachment #292429 - Flags: review?(Olli.Pettay)
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.
Attachment #292715 - Flags: review?(Olli.Pettay) → review+
> 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.
Attachment #292715 - Flags: superreview?(jst)
> 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 2.0.0.10 and
2.0.0.11).

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 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.
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])? :-)
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Whiteboard: need sr=jst
Attachment #292715 - Flags: superreview?(jst) → superreview+
Attachment #292715 - Flags: approval1.9?
Attachment #292715 - Flags: approval1.9? → approval1.9+
Landed on 1.8 branch.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: need sr=jst
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.
Attachment #292715 - Flags: approval1.8.1.12?
(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 1.8.1.12, a=dveditz for release-drivers
Attachment #292715 - Flags: approval1.8.1.12? → approval1.8.1.12+
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: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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Keywords: fixed1.8.1.12 → verified1.8.1.12
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: