If focus is in a text input, and you minimize the Firefox window, the following events are fired: onblur onblur onfocus onblur onblur onfocus However, switching tabs correctly only fires onblur once. Restoring that same window fires the following: onblur onfocus onfocus onblur onfocus When you click into a text input that does not currently have focus, the following events are fired for that element: onblur onfocus This behavior is literally insane. It makes writing DOM compliant JavaScripts nearly impossible since Firefox hoses up the events.

moving somewhere a little closer to where the bug will get fixed.

It is my understanding that Firefox and Mozilla bugs are to remain separate. Is this not the case? If so, this is filed against Firefox and should remain so.

Please attach a testcase that can be used to reproduce the problem.

Test case proves another thing is happening. This may be a dupe.

Place focus in the text input and then minimize and restore Firefox.

When I minimize I get a blur and a focus, when unminimize the same (on Linux; I bet it's different on Windows). This is probably a duplicate of the bug about global window focus affecting DOM focus events....

On Windows it gets just a blur when minized, but two focus events when restored.

*** Bug 260996 has been marked as a duplicate of this bug. ***

Was this fixed by the checkin for bug 258285?

This is still broken: 20051105 trunk.

The problem here is the Windows OS. Windows dispatches focus and activation events in reverse order when a window is minimized and also when a minimized window is restored. This doesn't play nice with the event state manager. Looks like Bug 82534 tried to address this quirk in Windows. The fix that was checked in is not 100% correct. Though it solved the focus lock ups in that bug, it still doesn't play nice with the event state manager.

So, it appears that we are not detecting the Windows activation/deactivation sequences correctly. I did some poking with MS Spy++ and these are the window messages received by Firefox during activation/deactivation sequences. 1) Deactivation, switching from Firefox to some other window WM_ACTIVATE WM_KILLFOCUS 2) Activation, switching back to Firefox WM_ACTIVATE WM_SETFOCUS 3) Deactivation, minimizing Firefox WM_KILLFOCUS WM_ACTIVATE 4) Activation, restoring Firefox WM_ACTIVATE WM_SETFOCUS WM_ACTIVATE It seems that strange things also happen when virtual desktop managers are running. See Bug 357215. 5) Deactivation, switching to another virtual desktop WM_ACTIVATE WM_KILLFOCUS 6) Activation, switching back to Firefox's virtual desktop WM_ACTIVATE WM_ACTIVATE WM_SETFOCUS

Comment on attachment 253726 [details] [diff] [review] Patch Why did you remove kWClassNameDialog from the list of possible Moz windows?

Oops... I didn't mean to remove kWClassNameDialog.

Comment on attachment 254006 [details] [diff] [review] Patch Prepend GetAncestor with :: as the prevailing style mandates. As far as I can see the RewindFocusState and supporting stuff wouldn't be needed in focus controller anymore, so could you remove them too to not leave them dangling there? Other than those, this looks good to go for me.

Revised as per Ere's review.

Comment on attachment 254817 [details] [diff] [review] Patch As far as I can see mPreviousElement and mPreviousWindow can also be removed from focus controller. With that done, r=emaijala. Good work!

I need help checking this in. Ere, would you be able to help?

Sure. Patch checked in to trunk.

please add a mochitest for this. should be easy.

Comment on attachment 255791 [details] [diff] [review] patch for checkin Since this patch seems to have fixed bug 357215, one of the disastrous "focus broken" bugs, is there any chance we could take this on branch? I'd love to finally put all those "ctrl-c doesn't work" and "the quick find bar popped up unexpectedly" complaints to rest.

The patch from this bug probably caused bug 372177.

Comment on attachment 255791 [details] [diff] [review] patch for checkin not approved, need a branch-merged patch that includes the regression fix mentioned in comment 23

this probably caused https://bugzilla.mozilla.org/show_bug.cgi?id=379148 to

Since this has been OK on trunk "baking" for 3 months, can it now go onto branch?

no, not 1.8.1.4: - No branch-merged patch (comment 24) - probable cause of two regressions (comment 23 and comment 25) that aren't fixed by the aforementioned missing branch patch - way past the cut-off for nominations for this release. We've got release candidate builds already

OK. Didn't realize it was that close. Sorry, and thanks. Will shoot for next version in that case.

We wouldn't hold a release for this so it's not a "blocker" for 1.8.1.5. If you want to get it in anyway you can ask for approval for a patch that meets the requirements of comment 27 (but note even on trunk not all the regressions have been fixed). A large patch for a non-critical problem with a questionable history of regressions isn't going to be accepted late in the 1.8.1.5 process. The fact that no branch patch has been attached in the six weeks since comment 27 is not a hopeful sign.

OK, in that case, postponing requested block until the _3rd_ next version.

Reopening this due to unfixed regression (bug 379148). I'm going to back this out until a better patch is available.

Comment on attachment 254817 [details] [diff] [review] Patch The patch has been backed out.

Dogfooding Minefield now is annoying with bug 385478 which I think is worse than bug 379148. Is the dogfood keyword appropriate here? Bugzilla says don't set if you're not a dev.

Not a blocker.

ditto comment 29 still, not a 1.8.1 branch blocker. Unfixed regressions, questionable gain to fix old old bugs on a stability branch.

I suppose that something that annoys and wastes my time on a daily basis but doesn't actually prevent my getting things done is reasonably "non-critical", but the gain of fixing this (at least insofar as it fixes bug 357215) is not questionable.

This also confuses screen-readers when alt+tabbing between windows. I'm assuming screen magnifiers will have the same problem. I'd like to get this fixed for 1.9. It'd be nice if we could get it in for m9 so I can do appropriate testing.

Renominating, seems more important in light of a11y comments. /be

Taking..

This is definitely complicated. One thing is that NS_ACTIVATE and NS_DEACTIVATE must be sent from the same window as NS_GOTFOCUS and NS_LOSTFOCUS. Otherwise EventStateManager gets confused. The other thing is that activation and focus events must happen in correct order. All this is quite difficult to make happen on Windows where minimize and restore do some nasty stuff. And one undocumented thing the previous patch relied on doesn't work in all occasions, which caused the regression. Without that trick the whole thing collapses down. Regarding comment #6, why do you get both blur and focus when minimizing in Linux? Does it also happen when clicking another window? I was thinking the correct behavior would be only blur on minimize and only focus on restore.

I see it's not marked regression, but are we sure it never worked correctly? If it once worked it's a good idea to see what changed.

I've no idea if it has worked any time in the past. Anyway, the "nasty hack" in WebShellWindow/FocusController shows that it's not easy to get working as expected. I used a few too many hours trying to get the cleaned-up implementation to work properly, but I've now resorted to trying to get old implementation to work properly. Didn't think it would be this complicated...

Still working on it..

It's actually eventStateManager's fault, I think. It's suppressing a blur but not the related focus during activation. Still need to unwind some local changes, check what's missing since the backout that's causing trouble and do some serious testing.

Found out the cause for the new regression after the backout. A small part of code was not properly backed out from nsWindow.cpp. I'll fix that when the tree is green. It will fix bug 392148 and bug 393554 as reported, but the testcase for bug 392148 is showing another fairly serious problem that will need to be investigated (though it's more probably related to mouse trailer). After this I'll test and post a patch for EventStateManager.

The backout is now fixed.

So it all boils down to this: the forementioned reasons prevent a cleaner approach that the original patch was aiming for, therefore here's just a patch to fix the current behavior. The only problem was the extra focus event with no matching blur event. This was caused because nsEventStateManager suppressed the blur when it noticed that the activation sequence was in progress, but didn't suppress the focus. This patch simply makes it suppress them both. Now this seems to work perfectly on Windows, but I think this should be tested on Linux too. Anyone feel free to do that while I'm putting up my build environment..

Comment on attachment 278809 [details] [diff] [review] Possible patch Ok, doesn't affect Linux builds and now Linux and Windows behave identically.

Does anyone have other steps to reproduce this bug? With the test case, I lose occasional blur events but apart from that I've seen no other issues.

> Does anyone have other steps to reproduce this bug? Load a virtual desktop manager and try switching between virtual desktops then doing something like copying text from Firefox. That's where it bites me. See bug 357215.

Neil, I'm not aware of any other problems unless losing a blur different from getting an extra focus. I tried the PowerToys virtual desktop thingy but it misbehaved so badly that I wouldn't use it as any reference.

try http://virtuawin.sourceforge.net/ - it has some of the same "mozilla" problems as windows' piece of junk (I think it also goes by the name vdm) but it's a stable tool. Can't swear it has problem described in this bug - I haven't used it in several months.

Comment on attachment 278809 [details] [diff] [review] Possible patch OK, so I compared two builds, one with this patch, and I was able to trigger consecutive focus events in the one without the patch fairly readily.

Fix checked in to trunk. I also tested briefly with VirtuaWin without finding any problems with Firefox (VirtuaWin on the other hand gave me a hard time).

This bug caused all the popup tests to fail because they expect a focus event to be fired on the window.

Backed out again due to failing tests. Apparently the patch broke popup window activation somehow. *sigh*

we probably need to create another branch and clean this up make a focus refractor branch and rewrite 99 percent of the code like what was done on the reflow branch

This stuff is feeling scary. If we can't get to this before beta2 we probably don't want to take it at all for 1.9

Agreed. I've used quite a few hours to get a hold on this and other focus things, are they are truly scary.

b2 freeze is tomorrow. :(

And I'm still having trouble with this. Looking from the nsWindow side of this I've been unable to find a way to make it work properly.

Given Comment 58 moving to wanted list...

hi folks, I am having troubles related to this, perhaps worth a new ticket. lets see if somebody here has an idea: I have a page that registers an on blur handler to the window (the attached demo page simplifies this). When I open two tabs. lets say mozilla.org and my demo page, the onBlur handler will trigger always. If I lets say open a Word or Explorer the blur event triggers correctly ONLY if the text box has no focus. If the textfield does have the blinking cursor inside and I switch to explorer, word etc. the blur will NOT trigger. So I guess this is related to Windows events and how they are mapped, because the internal tab events work fine. (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13)

Any possibility of this and bug 357215 getting some dev cycles now that 3.0 is out?

I've been working on some related issues in bug 112294. It's getting there so yes, there's hope.

I think I'm getting there thanks to the inspirational atmosphere of Whistler. Still need to test some things, but looking good so far.

This is a new patch just for the record. I'll have to test some more and see if there's more cleanup that could be done.

Possibly actually working patch. Still work in progress.

Comment on attachment 331901 [details] [diff] [review] Better patch I'm pretty sure this works properly. The removed code in WM_WINDOWPOSCHANGED isn't needed now that the wrong WM_ACTIVATE isn't processed. Activation handling is needed in WM_KILLFOCUS handling too, because during activation WM_KILLFOCUS can follow WM_ACTIVATE before any WM_SETFOCUS.

Comment on attachment 331901 [details] [diff] [review] Better patch > if (gJustGotDeactivate) { >+ gJustGotActivate = PR_FALSE; > gJustGotDeactivate = PR_FALSE; > result = DispatchFocus(NS_DEACTIVATE, isMozWindowTakingFocus); >+ } >+ else if (gJustGotActivate) { >+ gJustGotActivate = PR_FALSE; >+ gJustGotDeactivate = PR_FALSE; Nit: gJustGotDeactivate must already be false here.

Patch for checkin, only change is a fix for Neil's nit.

Are we comfortable landing this so close to an alpha release? There has been many iterations of the patch, which is understandable given the state of our focus code, which makes me worried that things might not be perfect still. Should we hold off until until tomorrow so that we can get lots of bake time before "shipping" this?

Shaver sez get it in for testing goodness. Assuming Ere thinks that's a good idea of course.

I'm quite comfortable with the latest patch, which is quite simple and completely different from the previous attempts. So I support getting it in if possible.

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/c0f5a0af84dd

Windows unit tests have been dying for focus-related changes since this patch landed, backed out: http://hg.mozilla.org/mozilla-central/index.cgi/rev/c0f5a0af84dd

Weird, I was able to run the tests with this patch. I guess it's related to the removed code. Need to dig further..

It turns out that this patch caused me two really serious regressions: 1. When focus was supposed to be in the body of a web page, I couldn't scroll with the keyboard (although strangely enough I could tab to a link and scroll). 2. When opening a dialog whose first control was a textbox focus was not correctly set on the textbox. (Other dialogs were OK.)

Thanks for the info, that'll probably help me track it down although I blame myself for not noticing such blaring problems.

Has there been any progress on this?

No, sorry.

Any idea if Enn's focus rewrite will fix this along the way (or at least make it easier to fix)?

Yes, it does.

Bug number for that?

https://bugzilla.mozilla.org/show_bug.cgi?id=178324 if im not mistaken

Fixed by focus refactoring (bug 178324).