Closed Bug 261074 Opened 15 years ago Closed 11 years ago

OnFocus fires twice when window restored

Categories

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

x86
Windows 2000
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: emaijala+moz)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(3 files, 7 obsolete files)

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.
Assignee: firefox → events
Component: General → DOM: Events
Product: Firefox → Browser
QA Contact: firefox.general → ian
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.
Summary: OnFocus, OnBlur Fire Wildly for One Event → OnFocus fires twice when window restored
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....
Whiteboard: DUPEME
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.

Blocks: focusnav
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
   
Attached patch Patch (obsolete) — Splinter Review
Assignee: events → oliver_yeoh
Status: NEW → ASSIGNED
Attachment #253726 - Flags: superreview?(roc)
Attachment #253726 - Flags: review?(emaijala)
Comment on attachment 253726 [details] [diff] [review]
Patch

Why did you remove kWClassNameDialog from the list of possible Moz windows?
Attached patch Patch (obsolete) — Splinter Review
Oops... I didn't mean to remove kWClassNameDialog.
Attachment #253726 - Attachment is obsolete: true
Attachment #254006 - Flags: superreview?(roc)
Attachment #254006 - Flags: review?(emaijala)
Attachment #253726 - Flags: superreview?(roc)
Attachment #253726 - Flags: review?(emaijala)
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.
Attachment #254006 - Flags: superreview?(roc)
Attachment #254006 - Flags: review?(emaijala)
Attachment #254006 - Flags: review-
Attached patch Patch (obsolete) — Splinter Review
Revised as per Ere's review.
Attachment #254006 - Attachment is obsolete: true
Attachment #254817 - Attachment is obsolete: true
Attachment #254817 - Attachment is obsolete: false
Attachment #254817 - Flags: superreview?(roc)
Attachment #254817 - Flags: review?(emaijala)
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!
Attachment #254817 - Flags: review?(emaijala) → review+
Attachment #254817 - Flags: superreview?(roc) → superreview+
Attached patch patch for checkin (obsolete) — Splinter Review
I need help checking this in. Ere, would you be able to help?
Sure. Patch checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
please add a mochitest for this. should be easy.
Flags: in-testsuite?
Whiteboard: DUPEME
Depends on: 371564
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.
Attachment #255791 - Flags: approval1.8.1.3?
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
Attachment #255791 - Flags: approval1.8.1.4? → approval1.8.1.4-
Since this has been OK on trunk "baking" for 3 months, can it now go onto branch?
Flags: blocking1.8.1.4?
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
Flags: blocking1.8.1.4? → blocking1.8.1.4-
OK. Didn't realize it was that close. Sorry, and thanks. Will shoot for next version in that case.
Flags: blocking1.8.1.5?
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.
Flags: blocking1.8.1.5? → blocking1.8.1.5-
OK, in that case, postponing requested block until the _3rd_ next version.
Flags: blocking1.8.1.6?
Reopening this due to unfixed regression (bug 379148). I'm going to back this out until a better patch is available.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #255791 - Attachment is obsolete: true
Comment on attachment 254817 [details] [diff] [review]
Patch

The patch has been backed out.
Attachment #254817 - Attachment is obsolete: true
Depends on: 385478
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.  
Flags: blocking1.9?
Not a blocker.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
ditto comment 29 still, not a 1.8.1 branch blocker. Unfixed regressions, questionable gain to fix old old bugs on a stability branch.
Flags: blocking1.8.1.7? → blocking1.8.1.7-
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.
Keywords: access
Renominating, seems more important in light of a11y comments.

/be
Flags: blocking1.9- → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Taking..
Assignee: oliver_yeoh → emaijala
Status: REOPENED → NEW
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.
Status: NEW → ASSIGNED
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.
Blocks: 392148
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...
Blocks: 393554
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.
Whiteboard: [wanted-1.9]
The backout is now fixed.
Attached patch Possible patch (obsolete) — Splinter Review
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.
Attachment #278809 - Flags: superreview?(roc)
Attachment #278809 - Flags: review?(neil)
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.
Attachment #278809 - Flags: superreview?(roc) → superreview+
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.
Attachment #278809 - Flags: review?(neil) → review+
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). 
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
This bug caused all the popup tests to fail because they expect a focus event to be fired on the window.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out again due to failing tests. Apparently the patch broke popup window activation somehow. *sigh*
Attachment #278809 - Attachment is obsolete: true
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
Priority: -- → P5
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...
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
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.
Depends on: 112294
I think I'm getting there thanks to the inspirational atmosphere of Whistler. Still need to test some things, but looking good so far.
Status: REOPENED → ASSIGNED
Depends on: 82534
Attached patch New patch, work in progress (obsolete) — Splinter Review
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.
Attached patch Better patch (obsolete) — Splinter Review
Possibly actually working patch. Still work in progress.
Attachment #331899 - Attachment is obsolete: true
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.
Attachment #331901 - Flags: superreview?(roc)
Attachment #331901 - Flags: review?(neil)
Attachment #331901 - Flags: superreview?(roc) → superreview+
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.
Attachment #331901 - Flags: review?(neil) → review+
Attached patch Patch for checkSplinter Review
Patch for checkin, only change is a fix for Neil's nit.
Attachment #331901 - Attachment is obsolete: true
Attachment #334530 - Flags: superreview+
Attachment #334530 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Weird, I was able to run the tests with this patch. I guess it's related to the removed code. Need to dig further..
Status: REOPENED → ASSIGNED
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?
Depends on: 178324
Fixed by focus refactoring (bug 178324).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.