217012 - browser window won't come to foreground with help window open

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Dan M]

This bug is a piece broken out of multi-bug 204761. neil@parkwaycc notes in bug
204761 comment 30 that if you do this:

1. Open Mozilla browser
2. Open Mozilla help (Help -> Help Contents) (help is alwaysRaised)
3. Open another app
4. Try to raise Mozilla browser (by clicking in the browser window)

the browser window is activated but it won't raise. The other app remains on top.

Comment 1

[Dan M]

Attachment: when raising a window to the top, first raise all windows in higher layers to the top

Windows are constrained to remain below windows in higher layers. So if the OS
tries to raise such a window to the top, we force it to remain below other,
higher windows. Oops. This patch raises those windows in higher layers to the
top. Now when a user clicks in a window it still remains below windows in
higher layers but at least Mozilla can be brought to the foreground.

This patch handles only the case where a window is being brought to the top.
Other cases are conceivable but they would be considerably more trouble to
handle and as far as I know they never happen in real life. (I'm thinking of
the OS asking us to move the help window to the back of the stack. We would
refuse since we want to keep it above the browser. As far as I know that
request is always couched the other way: other windows are asked to move to the
front.)

I'm able to test only on Windows XP. I'd appreciate people trying the patch on
other platforms.

The printf of course is unnecessary and wouldn't be checked in, but I'm
concerned some platforms may get into a loop with windows fighting each other
to be on top. I expect that to not happen but it's worth checking for. You want
to see that printf once or twice for each Mozilla window you have open (or just
a subset of the windows) when clicking to raise a background window.

Comment 2

[Matthias Versen [:Matti]]

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

Comment 3

[mhonline]

hey Dan

sorry, I did not got what you want to say.
Is it possible to have this in short,
esp why the help-task is behaving in this non-windows-compliant way ?
Thanks

Martin



P.S. - or hint:
same problem but other solution: You may want to have a look at old way, the
help-task was handled in OOo (before 1.0) or SO 5.2

Comment 4

[Dan M]

mhonline: You're asking for clarification on the cause of the bug? Because it
wants something more capable than most OSes support, Mozilla implements its own
window layering. Mozilla's window layering code didn't account for interactions
with other programs. This patch should fix it. Seems to on Windows, anyway.

Comment 5

[[e-mail]]

Could this be the cause of bug 184890?

Comment 6

[R.J. Keller]

alwaysRaised is probably going to be removed. It's causing too many problems and
it doesn't seem like the appropriate action.

Comment 7

[Dan M]

Whoa! You gave me a start there. alwaysRaised won't be removed. It's part of DOM
0. You're talking about no longer using it by default on the Help window, which
is fine.

Comment 8

[R.J. Keller]

yes, that's what I meant. Sorry for the confusion.

Comment 9

[Dan M]

Oh, and neil (comment 5): it's conceivable this patch could fix bug 184890. It'd
be nice to have someone with a Win98 system give this patch a test run. In two
days it'll have been a month I've been waiting for a reviewer to let me check
this damn thing in.

Comment 10

[R.J. Keller]

I'll do some QA on if this will help bug 184890. I have a Win98 box.

Comment 11

[R.J. Keller]

I'll do some QA to see if this will help bug 184890. I have a Win98 box.

Comment 12

[Dan M]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

alright. Having heard from neither of my first two, I'm switching to my third
reviewer candidate. C'mon down, Neil!

As I've mentioned above, I'm concerned about infinite loops on the Mac. It
wouldn't hurt to have someone try this patch on the Mac before it goes in but
at this point I'm willing to check it in to the alpha trunk and wait for
fall-out.

Comment 13

[R.J. Keller]

Yes, this bug 184890 on Windows 98. I compiled a build without the patch then
one with the patch, and noticed that the freeze did go away.

We might want to try and get this for 1.5.

Comment 14

[[e-mail]]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

>+printf("************ Z-Constrain window %8lX\n",this);
I know, this is only a debugging printf and not going to get checked in, but we
use %p for pointers :-P

>+    if (altered &&
>+        (position == nsIWindowMediator::zLevelTop ||
>+          position == nsIWindowMediator::zLevelBelow && aReqBelow == 0))
>+
>+      PlaceWindowLayersBehind(mZlevel+1, nsIXULWindow::highestZ, 0);
Well the first nit is that the 3rd parameter is a pointer and therefore should
be nsnull (as possibly aReqBelow should be). Then I wondered, why aren't you
passing aReqBelow anyway?

>+  nsCOMPtr<nsISimpleEnumerator> windowEnumerator;
>+  nsCOMPtr<nsIWindowMediator> mediator(do_GetService(kWindowMediatorCID));
>+  if(!mediator)
>+    return;
Possibly a slight gain by moving the declaration of windowEnumerator after this
early return.

>+    if (nextZ < aLowLevel)
>+      break;
Possibly neater to use <= here to save you adding 1 to aLowLevel earlier.

Comment 15

[[e-mail]]

Requesting blocking 1.5 as it also fixes the dependent hang bug.

Comment 16

[Dan M]

I despise the use of nsnull for 0, when null pointers are 0 by definition of the
C++ language. nsnull is one of the leftovers from that priesthood of
self-proclaimed geniuses who gave us gems like the eye-twitching "if (CONSTANT
== variable)" construction because they thought it would protect them from
themselves. I'd like to see all these things fixed. And I find 0 more readable
than nsnull; I *know* what 0 is, so I'm gonna kick about this one.

Arguably aBehind is an extraneous parameter anyway. As you noticed its value is
always just -- ahem -- 0. Which is a special, overloaded value and that actually
does kind of stink but there are precedents and you weren't complaining about
that so I'll move on now.

But anyway, it's not the same thing as aReqBelow. Regardless of whether we were
asked to place some window X behind another of our own windows or in front of
everything, the purpose of this new method is to force all the windows which
must remain in front of window X to the front of everything possible, including
other applications' windows. So, used in this context (the only context it's
being used in so far) aBehind is always 0. To be sure aReqBelow is probably also
0 in this case but it may not be.

I've moved the declaration of windowEnumerator down. I agree that's better.

About (mZlevel+1 and nextZ < aLowLevel) vs (mZlevel and nextZ <= aLowLevel) I
agree we could save a bit of machine code by using the latter forms but
conceptually the former is clearer. I wanted the interface to this method to use
an inclusive range because I think that's easier to follow and, on the off-hand
chance that someday this method might find itself being used to move a group of
lower-Z windows back behind some other window, since z-levels are unsigned ints
there's no way to specify all windows through 0 if the range is exclusive.

Comment 17

[Dan M]

Oof. I'm a little bit afraid of this patch and I'd like to see it spend some
time on the trunk before it goes into 1.5.

Comment 18

[[e-mail]]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

OK, so having read the comments as well as the code, r=me once you move the
declaration of windowEnumerator.

Comment 19

[jag (Peter Annema)]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

+    nsCOMPtr<nsIXULWindow> nextXULWindow(do_QueryInterface(nextWindow));
+    if (!nextWindow)
+      break; // unexpected error

Either you wanted to check for !nextXULWindow there, or you could move the QI
down below the break. Though should this really be a break, or an assertion?
Put differently, how unexpected is this error?

Comment 20

[Dan M]

Quite right, I should be checking nextXULWindow. As for how unexpected it is,
frankly it'll never happen because the method by which windows are added to the
list I'm enumerating allows only nsIXULWindows. And the enumerator I'm using, I
got using the method GetZOrderXULWindowEnumerator. Really, it seems like I could
just trust that I'm going to get an nsIXULWindow. I'll remove the check.

Comment 21

[Asa Dotzler [:asa]]

not the kind of change I think we want to be taking this late in the game. 

Comment 22

[jag (Peter Annema)]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

sr=jag

Comment 23

[Dan M]

Patch is in, yay! Man, that was hard. Now to hope I haven't screwed any of the
other platforms.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

>+    if (altered &&
>+        (position == nsIWindowMediator::zLevelTop ||
>+          position == nsIWindowMediator::zLevelBelow && aReqBelow == 0))
>+

Gcc whines about the && expression not being over-parenthesized when it is a
term in a || expression.  Worth a quick review-less followup patch checkin?

>+  // get next lower window
>+  PRBool more = PR_FALSE;
>+  windowEnumerator->HasMoreElements(&more);
>+  while (more) {
>+    PRUint32 nextZ; // z-level of nextWindow
>+    nsCOMPtr<nsISupports> nextWindow;
>+    windowEnumerator->GetNext(getter_AddRefs(nextWindow));
>+    nsCOMPtr<nsIXULWindow> nextXULWindow(do_QueryInterface(nextWindow));
>+    if (!nextWindow)
>+      break; // unexpected error
>+
>+    nextXULWindow->GetZlevel(&nextZ);
>+    if (nextZ < aLowLevel)
>+      break;
>+    
>+    // move it just above its next higher window
>+    nsCOMPtr<nsIBaseWindow> nextBase(do_QueryInterface(nextXULWindow));
>+    if (nextBase) {
>+      nsCOMPtr<nsIWidget> nextWidget;
>+      nextBase->GetMainWidget(getter_AddRefs(nextWidget));
>+      if (nextZ <= aHighLevel)
>+        nextWidget->PlaceBehind(previousHighWidget, PR_FALSE);
>+      previousHighWidget = nextWidget;
>+    }
>+
>+    windowEnumerator->HasMoreElements(&more);
>+  }

This while loop morphed into an incomplete (one or more empty parts) for-loop
in the checked in version, to wit:

+  PRBool more = PR_FALSE;
+  windowEnumerator->HasMoreElements(&more);
+  for ( ; more; windowEnumerator->HasMoreElements(&more)) {
+    . . .
+      previousHighWidget = nextWidget;
+    }
+  }

Which makes me want to point out the utility and felicity of the
nearly-forgotten comma operator:

+  PRBool more = PR_FALSE;
+  while (windowEnumerator->HasMoreElements(&more), more) {
+    . . .
+  }

Sorry for the late comments -- I know how danm loves my nit-picking, but will
he when it's not timely? ;-)

/be

Comment 25

[Dan M]

According to t'box brad, gcc is happy with the if as is. Maybe gcc is less dumb
now. But the elegant comma operator while loop makes me happy. I'll check that
in as part of an upcoming patch in related bug 42557.

Comment 26

[Hermann Schwab]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

requesting fix for 1.4.2, was fixed in trunk starting BuildID 2003092504 

Requesting approval for 1.4.2

This bug crashes every Win9x, if a user wants to do something step for step and
reading help, like going over the preferences.

This has been crashing for a year, and is fixed now on the trunk for a month,
would be nice to have it in 1.4.2
A one-line patch only checking out the patch that introduced the crash had been
successfully tested in another bug, see bug 184890 comment 46, bug 184890
comment 23

Comment 27

[Mike Kaply [:mkaply]]

Comment on attachment 130237 [details] [diff] [review]
when raising a window to the top, first raise all windows in higher layers to the top

a=mkaply for 1.4.2

Comment 28

[Dan M]

1.what now? Oy, haven't you guys put a stake in that thing yet? If you need one,
I'm sure I have one around here somewhere. If you decide not to take me up on my
stake offer, I'll be able to take care of this for you in a week or so. If
you're in more of a rush...

Comment 29

[Dan M]

checked in to the 1.4 branch for 1.4.2