261074 - OnFocus fires twice when window restored

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P5)

Description

[Jerry Baker]

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.

Comment 1

[Mike Connor [:mconnor]]

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

Comment 2

[Jerry Baker]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Jerry Baker]

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

Comment 5

[Jerry Baker]

Attachment: This will display the problem.

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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....

Comment 7

[Jerry Baker]

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

Comment 8

[Jerry Baker]

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

Comment 9

[Aaron Leventhal]

Was this fixed by the checkin for bug 258285?

Comment 10

[Jerry Baker]

This is still broken: 20051105 trunk.

Comment 11

[Oliver Yeoh]

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.

Comment 12

[Oliver Yeoh]

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 13

[Oliver Yeoh]

Attachment: Patch

Comment 14

[Ere Maijala (slow)]

Comment on attachment 253726 [details] [diff] [review]
Patch

Why did you remove kWClassNameDialog from the list of possible Moz windows?

Comment 15

[Oliver Yeoh]

Attachment: Patch

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

Comment 16

[Ere Maijala (slow)]

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.

Comment 17

[Oliver Yeoh]

Attachment: Patch

Revised as per Ere's review.

Comment 18

[Ere Maijala (slow)]

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!

Comment 19

[Oliver Yeoh]

Attachment: patch for checkin

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

Comment 20

[Ere Maijala (slow)]

Sure. Patch checked in to trunk.

Comment 21

[Robert Sayre]

please add a mochitest for this. should be easy.

Comment 22

[Peter Kasting]

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.

Comment 23

[Thomas Bertels]

The patch from this bug probably caused bug 372177.

Comment 24

[Daniel Veditz [:dveditz]]

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

Comment 25

[gabe]

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

Comment 26

[Worcester12345]

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

Comment 27

[Daniel Veditz [:dveditz]]

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

Comment 28

[Worcester12345]

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

Comment 29

[Daniel Veditz [:dveditz]]

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.

Comment 30

[Worcester12345]

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

Comment 31

[Ere Maijala (slow)]

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

Comment 32

[Ere Maijala (slow)]

Comment on attachment 254817 [details] [diff] [review]
Patch

The patch has been backed out.

Comment 33

[Brian Polidoro]

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.  

Comment 34

[Johnny Stenback (:jst)]

Not a blocker.

Comment 35

[Daniel Veditz [:dveditz]]

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

Comment 36

[John Hardin]

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.

Comment 37

[Tim Keenan]

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.

Comment 38

[Brendan Eich [:brendan]]

Renominating, seems more important in light of a11y comments.

/be

Comment 39

[Ere Maijala (slow)]

Taking..

Comment 40

[Ere Maijala (slow)]

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.

Comment 41

[Aaron Leventhal]

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.

Comment 42

[Ere Maijala (slow)]

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...

Comment 43

[Ere Maijala (slow)]

Still working on it..

Comment 44

[Ere Maijala (slow)]

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.

Comment 45

[Ere Maijala (slow)]

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.

Comment 46

[Ere Maijala (slow)]

The backout is now fixed.

Comment 47

[Ere Maijala (slow)]

Attachment: Possible patch

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 48

[Ere Maijala (slow)]

Comment on attachment 278809 [details] [diff] [review]
Possible patch

Ok, doesn't affect Linux builds and now Linux and Windows behave identically.

Comment 49

[[e-mail]]

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.

Comment 50

[John Hardin]

> 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.

Comment 51

[Ere Maijala (slow)]

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.

Comment 52

[Wayne Mery (:wsmwk)]

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 53

[[e-mail]]

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.

Comment 54

[Ere Maijala (slow)]

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). 

Comment 55

[Neil Deakin]

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

Comment 56

[Ere Maijala (slow)]

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

Comment 57

[gabe]

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

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 59

[Ere Maijala (slow)]

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

Comment 60

[Reed Loden [:reed]]

b2 freeze is tomorrow. :(

Comment 61

[Ere Maijala (slow)]

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.

Comment 62

[Mike Schroepfer]

Given Comment 58 moving to wanted list...

Comment 63

[Fabian Lange]

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)

Comment 64

[Fabian Lange]

Attachment: Demonstrates onblur handler not firing on "window" change when input field is focused.

Comment 65

[John Hardin]

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

Comment 66

[Ere Maijala (slow)]

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

Comment 67

[Ere Maijala (slow)]

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

Comment 68

[Ere Maijala (slow)]

Attachment: New patch, work in progress

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.

Comment 69

[Ere Maijala (slow)]

Attachment: Better patch

Possibly actually working patch. Still work in progress.

Comment 70

[Ere Maijala (slow)]

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 71

[[e-mail]]

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.

Comment 72

[Ere Maijala (slow)]

Attachment: Patch for check

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

Comment 73

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 74

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 75

[Ere Maijala (slow)]

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.

Comment 76

[Benjamin Smedberg]

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

Comment 77

[Dave Camp (:dcamp)]

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

Comment 78

[Ere Maijala (slow)]

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

Comment 79

[[e-mail]]

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.)

Comment 80

[Ere Maijala (slow)]

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

Comment 81

[John Hardin]

Has there been any progress on this?

Comment 82

[Ere Maijala (slow)]

No, sorry.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

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

Comment 84

[Neil Deakin]

Yes, it does.

Comment 85

[Ere Maijala (slow)]

Bug number for that?

Comment 86

[gabe]

https://bugzilla.mozilla.org/show_bug.cgi?id=178324

if im not mistaken

Comment 87

[Masatoshi Kimura [:emk]]

Fixed by focus refactoring (bug 178324).