404283 - Can't toggle Help windows z-index

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Stefan [:stefanh]]

Mac OS 10.4.10

STR:

1) Open the Help viewer in Firefox or SeaMonkey

2) Right-click (Ctrl-click) in the content area

3) Look at the context menu, make sure the "Always on Top" menuitem is checked

4) Click on the browser window

Expected result: Help window should stay in front of browser window - should jump back in front once you release the mouse

Actual result: Browser window is now the frontmost window

Tested on SeaMonkey & Firefox trunk.

NOTE: This is obviously an old cocoa regression:

- Works with Firefox 2006092808
- Does not work with Firefox 2006092904



From toolkit/components/help/content/help.js:

{
775   window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
776   var webnav = window.getInterface(Components.interfaces.nsIWebNavigation);
777   var dsti = webnav.QueryInterface(Components.interfaces.nsIDocShellTreeItem);
778   var treeowner = dsti.treeOwner;
779   var ifreq = treeowner.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
780 
781   return ifreq.getInterface(Components.interfaces.nsIXULWindow);
782 }
783 
784 # toggleZLevel - Toggles whether or not the window will always appear on top. Because
785 #   alwaysRaised is not supported on an OS other than Windows and Mac OS X, this code
786 #   will not appear in those builds.
787 #
788 #   element - The DOM node that persists the checked state.
789 #ifdef XP_MACOSX
790 #define HELP_ALWAYS_RAISED_TOGGLE
791 #endif
792 #ifdef XP_WIN
793 #define HELP_ALWAYS_RAISED_TOGGLE
794 #endif
795 #ifdef HELP_ALWAYS_RAISED_TOGGLE
796 function toggleZLevel(element)
797 {
798   var xulwin = getXulWin();
799   
800   // Now we can flip the zLevel, and set the attribute so that it persists correctly
801   if (xulwin.zLevel > xulwin.normalZ) {
802     xulwin.zLevel = xulwin.normalZ;
803     element.setAttribute("checked", "false");
804   } else {
805     xulwin.zLevel = xulwin.raisedZ;
806     element.setAttribute("checked", "true");
807   }
808 }
809 #endif

I don't really know how the widget code works, but looking at nsCocoaWindow, I can't really find any implemention of this (I could be wrong, though).

Comment 1

[Håkan Waara]

This should be very simple to fix. All that is needed is to implement nsCocoaWindow::PlaceBehind() which today is a stub.

Comment 2

[Stefan [:stefanh]]

Putting this on the blocker radar, since this is a feature that existed before the cocoa switch.

Comment 3

[Mike Schroepfer]

moving to blocking

Comment 4

[Josh Aas]

Attachment: PlaceBehind impl v1.0

Here is a PlaceBehind impl, but it doesn't completely resolve this bug. Probably because gecko's window z-order list isn't accurate. Working on that now.

Comment 5

[Josh Aas]

Attachment: fix v0.8

Comment 6

[Josh Aas]

Note to self - whatever we land to fix this shouldn't regress bug 355091.

Comment 7

[Josh Aas]

Attachment: disable always raised help v1.0

The reason fix v0.8 doesn't work is that when you divert a call to orderWindow:relativeTo: (don't allow it to succeed by calling super, maybe move the window somewhere else instead) Cocoa gets into a state where the next time the window moves it moves silently. That is, orderWindow:relativeTo: doesn't get called, nothing does. This is because something thinks the window is already in the right place. Because nothing gets called, we can't keep a window below the alwaysRaised window.

This silent window move is a Cocoa bug, I spent a long time trying to work around it with no luck. I'll file a bug with Apple. I think for now we should consider alwaysRaised to be unavailable on Mac OS X. It doesn't work in Gecko 1.8 either, it is super buggy, so this isn't even really a regression. The patch I am attaching now disables that "feature" of our help system on Mac OS X. I doubt anyone is going to miss it.

We still need to do zlevel handling for other things, apple event handling in particular. Fix v0.8 is a much better zlevel impl than we have now and if Apple fixes the silent window move bug then alwaysRaised will just work. Fix v0.8 does not regress Apple event handling. I think we should take fix v0.8 and this alwaysRaised disable patch.

Comment 8

[Stefan [:stefanh]]

Josh,

What do you mean by "It doesn't work in Gecko 1.8 either"? I can ensure you that I get the expected results in comment #0 with Gecko 1.8...

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Josh, I think I should test fix v0.8, so my review will be delayed by
a bit (though I hope to have it finished by later today).

Among other things, I'm worried that [GeckoBaseWindow
orderWindow:relativeTo:] might get reentered when you call orderBack:
or orderFrontRegardless from it.

Comment 10

[Josh Aas]

Stefan - on Mac OS X 10.5 if you open the help window with alwaysRaised on, the window always appears to be active even when another window has key and main. Secondly, if you drag the browser window under the help window somewhere else on the screen really weird (bad) things happen. alwaysRaised only "works" in a very superficial way.

Comment 11

[Josh Aas]

Steven - whenever we modify a z-order change we return immediately afterwards. We never call super. Modified requests are always logically different than the original (there are multiple ways of expressing the same move, like "go to bottom" or "go under the current bottom window"), either the window would continuously move one direction or it would jump to the final position. Either way we're fine. Please do make sure I'm right though!

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 301522 [details] [diff] [review]
fix v0.8

Josh, you misunderstood what I was saying.

If orderBack: and/or orderFrontRegardless call
orderWindow:relativeTo:, [GeckoBaseWindow orderWindow:relativeTo:]
will get reentered when these methods are called from within it, and
bad things will happen -- notably, a second Z-level event will get
sent to Gecko, and an infinite loop is possible.

I did some tests and found out that, while orderFrontRegardless
doesn't call orderWindow:relativeTo: (on either Tiger or Leopard),
orderBack: does call it (on both Tiger and Leopard).  So, at the very
least, you'll need to do something to avoid the bad consequences of
[GeckoBaseWindow orderWindow:relativeTo:] being reentered when
orderBack: is called from it.

On general principles I think it's best to also do this for calls from
[GeckoBaseWindow orderWindow:relativeTo:] to orderFrontRegardless --
you never know when Apple might change its implementation of [NSWindow
orderFrontRegardless], and make it call orderWindow:relativeTo:].

So I'm r-ing until this problem is fixed.

But my other tests of this patch went fine, on both Tiger and Leopard.
And I didn't notice any other problems with it.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 301908 [details] [diff] [review]
disable always raised help v1.0

r=whatever.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 301908 [details] [diff] [review]
disable always raised help v1.0

Yeah, I'm fine with this.

Comment 15

[Josh Aas]

"disable always raised help v1.0" checked in

Will create a new bug with the z-level patch I created here for followup on that system.

Comment 16

[Josh Aas]

Filed bug 417142 on a more robust z-level impl.

Comment 17

[Stefan [:stefanh]]

So, I'm not questioning the decision to not fixing this now - but am I supposed to file a new bug if I want to track the issue that I reported (since the issue wasn't fixed)?

Comment 18

[Josh Aas]

Please file a new bug on re-enabling the always raised option if you want it. It would depend on the bug I filed on the Cocoa z-level impl.