189085 - focus wrong after clicking cancel in "edit" filepicker & dialog

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

steps to reproduce:
1. open preferences
2. go to helper apps panel
3. choose a type
4. click edit (new probably works too)
5. click "choose"
6. click cancel
7. click cancel, expect the preference window to have focus
8. watch the browser window you opened preferences from get focus

Comment 1

[Ere Maijala (slow)]

This also happens on other Windows versions (at least Win2K and WinXP). I
believe the problem might be that the parent of the dialog is still disabled
when the dialog is closed. This will cause Windows to set focus to the next
top-most window.

Comment 2

[Ere Maijala (slow)]

Then again, here this shouldn't be the case if it follows the normal modal
window logic. I'm going to dig into it a bit deeper.

Comment 3

[Ere Maijala (slow)]

I think this is the infamous Windows problem with stacked modal dialogs. We have
code to focus the correct window in this situation, but I think the filepicker
doesn't count as a modal dialog.

I'll just try to find out if we have any better way to enforce the correct focus
(and not cause focus grabbing or other problems). 

Comment 4

[Ere Maijala (slow)]

Bug 54936 is the problem we need to avoid. Actually, the filepicker is already
noted in http://bugzilla.mozilla.org/show_bug.cgi?id=54936#c33.

Comment 5

[Ere Maijala (slow)]

I'll take this.

Comment 6

[Ere Maijala (slow)]

Attachment: v1 Patch to alter the mechanism used to ensure the focus

This patch will ensure that the correct window gets focus when a parented
window is destroyed. Also helps for File -> Open Web Location -> Choose File ->
Cancel -> Cancel. Might also fix bug 155002.

Comment 7

[Ere Maijala (slow)]

Comment on attachment 112547 [details] [diff] [review]
v1 Patch to alter the mechanism used to ensure the focus 

danm, could you review this? I've tried to make sure focus is not forced
unnecessarily to avoid problems such as bug 54936.

Comment 8

[Dan M]

Comment on attachment 112547 [details] [diff] [review]
v1 Patch to alter the mechanism used to ensure the focus 

  Alright you. If you check this code in, please keep your brace style
convention the same as the surrounding code (which is already the One True Way
in this case :) ).
  Summary and recap: bug 22658 notes a problem with the wrong window sometimes
being activated after a window has been closed. So while closing a XUL window
the current (pre-patch) code (1) determines that it is the topmost window and
takes this to mean its parent should be topmost once it's gone. If true, it (2)
activates its parent. It takes both steps before the window to be closed is
deactivated. For some reason, this works. However this also causes focus
regressions so (a) it's restricted to windows we know will be a problem
(dialogs stacked at least two deep, bug 54936). Since we (b) can't detect
stacks containing native/non-XUL windows, we just let the activation bug go
unaddressed in that case.
  The proposed code still does (1) unchanged but puts off (2) until after the
window has been deactivated and Windows has made a decision about the next
topmost window, so the decision can be corrected afterward, if necessary.
Apparently because of this timing the proposed code can run in all cases, so it
omits restriction (a).

  OK I'm remembering. Sorry to say, I don't like the proposed fix. This is a
very likely solution and I considered it when working on bug 22658 but I
discarded it because it makes the windows flash. An incorrect window is brought
to the front but it's immediately replaced by the correct window. It looks
cheesy and IMO isn't really an improvement over doing nothing at all.
  We revisit this problem once a year or so (bug 87504 for example). In the
past we've decided to let it be. Other Windows applications have the same
problem, after all. I'm inclined to close this one as a duplicate, too.
  However if you strongly feel that flashing the correct window to the front is
better than leaving it be, I'd take a patch that keeps the current behaviour
but does an additional check to be sure it took. That is, keep all the current
code but additionally do your check (2).

Comment 9

[Ere Maijala (slow)]

Oh, the flashing. I find it perfectly acceptable as long as the result is
correct (yeah, looks cheesy but anyway). Without this I'll need to change the
focus myself (as Windows will typically choose a window I definitely didn't
want), which is much worse. Ok, I'll make a new patch.

BTW, Composer flashes all the time when inserting images etc. 

Comment 10

[Ere Maijala (slow)]

Attachment: v1.1 Patch

How about this? Like v1, but eliminates the flash.

Comment 11

[Ere Maijala (slow)]

Comment on attachment 112796 [details] [diff] [review]
v1.1 Patch

Btw, this also fixes flashing in Composer when inserting images etc. I suppose
there's a hack or two in Composer to regain focus. They would probably be
unnecessary if this patch or its derivative is accepted.

Comment 12

[Dan M]

Comment on attachment 112796 [details] [diff] [review]
v1.1 Patch

Sorry. I really am. It looked like you were on to something there. This patch
pretty much works, but it causes a focus regression (a variety of the famous
bug 54936, in fact.)

Try it. Launch browser, make sure focus is in the URL bar, Edit -> Preferences
-> Helper Apps, New Type, Choose, Cancel, Cancel, Cancel. Focus is off the URL
bar and in fact the URL bar refuses to take focus until after you first give
focus to something else.

That's the real worry when patching this code. You seem to want this bug fixed
very much. I await your next patch. Please be very careful about focus
regressions.

Once again I suggest a hybrid of your first patch with the existing code (last
paragraph of comment 8). If it doesn't cause focus regressions!

Comment 13

[Dan M]

(I forgot to mention in my last comment...) responding to comment 9: To my eye,
windows flashing down then up looks unprofessional. My objection to the first
patch is that while it fixes the less common case (double-deep native dialog) it
"regresses polish," if I can call it that, in the more common case (double-deep
XUL dialogs).

My interest in the hybrid patch (last paragraph, comment 12) is that it should
address your objection to the (currently broken) less common case, though in an
unpolished way, and leave the already shiny parts still polished. Assuming there
are no focus regressions :) . I haven't tried it.

Comment 14

[Ere Maijala (slow)]

No worries, I'm aware of the multitude of problems focus related things may
cause. I didn't notice this case, thanks for pointing it out. I'll explore it
some more..

Comment 15

[Ere Maijala (slow)]

Attachment: v1.2 Patch

Here we go again :) WM_SETFOCUS was deferred when another window was being
destroyed (I didn't dare question the need for this (bug 134437)) but it was
re-posted to wrong window. Now all the cases I tested work perfectly.

Comment 16

[Ere Maijala (slow)]

Comment on attachment 112904 [details] [diff] [review]
v1.2 Patch

I'm once again ready for the verdict :)

Comment 17

[Dan M]

  Focus sucks. I apologize for whatever role I may have had in the flaky
behaviour of focus in Mozilla. I haven't done a complete battery of tests with
patch 1.2, but I did turn up a problem pretty quickly. Text edit form fields in
web pages aren't properly blurred.
  Launch browser.
  Load bugzilla.mozila.org/query.cgi.
  Click in the "Summary" text edit box, see the flashing cursor.
  Open the Edit -> Preferences dialog (I used the mouse).
  The dialog is open and on top of the browser window, but the text edit cursor
is still flashing in the Summary edit field. The good news is, keystrokes are
correctly routed to the Preferences dialog, not the browser form field. So it's
not as bad as it could be. But I expect (haven't tested) that DOM events are
affected and it seems like a problem, probably more than cosmetic.
  I'm beginning to wonder how this all works as well as it does.
  (PS when you get this patch beaten into shape, and I'm expecting you will, I'm
going to be all happy because you're close to fixing an old, annoying bug, but
I'm also going to whine about formatting. Please stick to two-space indentation
(some of that file has three-space indendation as you've done but we're trying
to squelch that) and this stuff where you're adding spaces

@@ -415,7 +468,7 @@
       delete shellInfo;
    }
    mContentShells.Clear();
-
+   
    if(mContentTreeOwner) {
       mContentTreeOwner->XULWindow(nsnull);
       NS_RELEASE(mContentTreeOwner);

... Damn now I'm being picky. I'm more concerned about other things of course
but I do have an anal side.

Comment 18

[Ere Maijala (slow)]

Ok, I'll just continue my quest :)

Well, I hated the three-space indents, but didn't dare change them. Now I will.
And honestly, I've no idea how I always manage to do some weird whitespace
changes. Maybe I'll just blame VC++ :)

Comment 19

[Ere Maijala (slow)]

CC'ing Alex Savulov and Chris Waterson because of bug 134437.

The patch for that bug introduced a mechanism to defer WM_SETFOCUS when a window
was being destroyed. It looks to me that the deferred message was posted to
wrong window, which is why it didn't seem to cause problems. Now if I change it
to really work, it ruins focus. For example the window might receive WM_SETFOCUS
immediately followed by WM_KILLFOCUS. If we now defer WM_SETFOCUS, it will be
handled after WM_KILLFOCUS. This is of course not desirable. 

I'm inclined to remove this mechanism altogether as it's also a part of the
focus problem of modal windows. Removing it doesn't seem to break the test case
of bug 134437. 

Alex, Chris, what do you think?

Comment 20

[Alexandru Savulov]

CC-ing Kevin.

Comment 21

[Ere Maijala (slow)]

Attachment: v2 Patch

This is (finally) a revised, a bit radical patch aimed to simplify the focus
stuff and make it work better. To sum it all up, at least the following fixes
are provided:
- correctly focus parent when closing double modals where the other one is a
filepicker 
- also no flashing of focus when inserting an image in Composer
- correctly focus parent when closing a dialog that was raised when another
window was active
- no more messing the order of window messages

I've tested it a fair bit and didn't find any problems, which of course doesn't
mean there aren't any :)

Comment 22

[Dan M]

Comment on attachment 113735 [details] [diff] [review]
v2 Patch

Huh. I can find no focus problems with patch v2. And specifically the problem I
mentioned in comment 17 seems to have evaporated. (However there was another
bug in that scenario: close the prefs dialog and, like in comment 12, the
browser edit field would refuse to re-take focus. v2 fixes that problem.)

v2 backs out the fix for bug 134437. However it does look like Ere is correct;
the 134437 fix seems to focus the wrong window and backing it out doesn't bring
its bug back. I've already talked with those guys about that patch. I know
they're concerned.

However as far as I can tell this is a good fix. I'm giving it the +. Alex,
Kevin: feel free to jump in here.

Comment 23

[Ere Maijala (slow)]

Oh :) 

Just for the record and those concerned: I realize this is a somewhat high-risk
change. I've beaten it a lot and for what I've done it works well. Anyway, there
might be something I've overlooked, but I hope we can get this in soon after 1.3
so there would be time to do any needed polishing. And if there are some issues,
I'm ready to tackle them.

Comment 24

[Ere Maijala (slow)]

Correcting component. I've seen no objections... good :)

Comment 25

[jag (Peter Annema)]

Comment on attachment 113735 [details] [diff] [review]
v2 Patch

>Index: xpfe/appshell/src/nsXULWindow.cpp
>===================================================================

>+#ifdef XP_PC
>+  // We need to explicitly set the focus on Windows
>+  nsCOMPtr<nsIWindowMediator> windowMediator(do_GetService(kWindowMediatorCID));
>+  if (windowMediator) {
>+    nsCOMPtr<nsIBaseWindow> parent(do_QueryReferent(mParentWindow));
>+    if (parent) {
>+      nsCOMPtr<nsIWidget> parentWidget;
>+      parent->GetMainWidget(getter_AddRefs(parentWidget));
>+      if (parentWidget)
>+        parentWidget->PlaceBehind(0, PR_TRUE);
>+    }
>+  }
>+#endif

You don't seem to be using windowMediator at all here.

Comment 26

[Ere Maijala (slow)]

Oh, you're right :) Corrected patch coming soon..

Comment 27

[Ere Maijala (slow)]

Attachment: v2.1 Patch

Removed windowMediator, otherwise the same.

Comment 28

[Ere Maijala (slow)]

Attachment: v2.1 Patch

Removed windowMediator, otherwise the same.

Comment 29

[Ere Maijala (slow)]

Comment on attachment 114707 [details] [diff] [review]
v2.1 Patch

Crap.

Comment 30

[Ere Maijala (slow)]

Comment on attachment 114708 [details] [diff] [review]
v2.1 Patch

Transferring r=, requesting sr=.

Comment 31

[jag (Peter Annema)]

Comment on attachment 114708 [details] [diff] [review]
v2.1 Patch

sr=jag

Comment 32

[Ere Maijala (slow)]

Fix checked in to trunk. Let's keep an eye on any regressions.

Comment 33

[Bernard Alleysson]

Today (build 2003022402) mozilla windows won't stay minimized
I don't have steps to reproduce yet but this looks like an old familiar bug
this checkin may have regressed this

Comment 34

[Ere Maijala (slow)]

Interesting. I haven't seen that happen, so steps to reproduce along with
information about operating system etc. would be appreciated.

Comment 35

[Ere Maijala (slow)]

Walk84 reported seeing also last week an issue where the window would restore
immediately when trying to minimize it, so if this is the same issue, the patch
of this bug might not have caused it.

Comment 36

[Phil Randal]

That regression is bug 194738.

Same effects as the old bug 120155.

Comment 37

[Dan M]

Bad news, everyone! This patch seems to have caused (part of) bug 195798. That
bug is really two bugs: the Mail Compose Window sometimes has carets in two
separate edit fields, and sometimes no caret. Bryner has traced the no-caret
case to a missing window deactivate message. (The textbox doesn't do anything
when reactivated because it caches its focused state, and so thinks it's still
focused because of the missing window deactivate message.)

That bug is cross-platform. However on Windows we've noticed a
WM_ACTIVATE(WA_INACTIVE) message recently gone missing. That is, the Message
Compose window no longer gets that message when "closed" (that window is only
hidden when closed, not destroyed). It does get that message if this part of the
patch for this bug is reverted:

Index: widget/src/windows/nsWindow.cpp
===================================================================
@@ -1816,7 +1811,8 @@
         }
       }
     } else
-      ::ShowWindow(mWnd, SW_HIDE);
+      ::SetWindowPos(mWnd, 0, 0, 0, 0, 0, SWP_HIDEWINDOW | SWP_NOSIZE |
SWP_NOMOVE | 
+        SWP_NOZORDER | SWP_NOACTIVATE);
   }
   mIsVisible = bState;
   return NS_OK;

At the moment I'm just cross-referencing these two bugs.

Comment 38

[Ere Maijala (slow)]

Dan, in my tests the compose window does get the WM_ACTIVATE with WA_INACTIVE
but it doesn't happen immediately because nobody else takes the focus. It does
anyway happen when I click another window. I guess we could change
SetWindowPos() to ShowWindow() for non-modal windows, but that shouldn't really
affect the real problem. Will test it a bit more though.

Comment 39

[Ere Maijala (slow)]

Reopening to fix Show().

Comment 40

[Ere Maijala (slow)]

Attachment: Patch to fix Show()

This patch fixes nsWindow::Show() so that SetWindowPos() is only used for
dialogs. It seems to fix the caret problem of bug 195798 and I don't see any
bad effects on it. It looks to me some messages happen in unexpected order
otherwise.

Comment 41

[Dan M]

Comment on attachment 118681 [details] [diff] [review]
Patch to fix Show() 

Heh. Yeah, this fixes the bug 195798 problem and I see no regressions from it.
Seems a safe bet we'll never make a dialog disappear the same way Composer does
:) r=danm

Comment 42

[Ere Maijala (slow)]

Comment on attachment 118681 [details] [diff] [review]
Patch to fix Show() 

I'm optimistic about getting sr= and I believe we want this fix for 1.4a :)

Comment 43

[jag (Peter Annema)]

Comment on attachment 118681 [details] [diff] [review]
Patch to fix Show() 

sr=jag

Comment 44

[Asa Dotzler [:asa]]

Comment on attachment 118681 [details] [diff] [review]
Patch to fix Show() 

please land this change first thing in 1.4beta. thanks.

Comment 45

[Ere Maijala (slow)]

Checked in.