216426 - Implement something to switch off and on the always on top behaviour of help window

Status: VERIFIED FIXED

(SeaMonkey :: Help Documentation, enhancement)

Description

[Gabriele Fava]

User-Agent:       Mozilla/4.0 (compatible; Opera7.0; Windows 98)
Build Identifier: 2003071814

Note that this bug is different from bug 195888.
Reports requesting a non-on-the-top behaviour are 194532, 211876 and 193348.
136647 is cause of the present on-the-top behaviour.

The current Help Window behaviour is to stay always on the top, but just of
other mozilla windows. That's often very useful (especially good the thing that
it doesn't go over normal external windows). But there are also situations when
it would be preferrable help window to behaves like normal windows, in order to
see the entire content of the windows and possibly work better, while not having
to close the help you were reading.

So, it would be very useful to all the users having an option to switch off or
on the always-on-top behaviour. It should not be an option placed in the
preference panel, because the need of one behaviour or the other depends just on
the situation, rather then the user. So it should be put as a selectable option
in the help browser's menu or (since the help doesn't have any menus at present)
as a big nice button on the help toolbar.

The default option at first boot should be always-on-top (which is more likely
to be the preferred if easily deactivatable).
I don't know though if the current option should be carried through different
sessions and neither if through different help starts in the same session.
Maybe a reset every time would be an acceptable choice.

Reproducible: Always

Steps to Reproduce:

Comment 1

[R.J. Keller]

confirming.

Comment 2

[Daniel Wang]

almost a patch:

/extensions/help/resources/content/contextHelp.js

 33     var params = Components.classes["@mozilla.org/embedcomp/dialogparam;1"]
 34                           
.createInstance(Components.interfaces.nsIDialogParamBlock);
 35     params.SetNumberStrings(2);
 36     params.SetString(0, helpFileURI);
 37     params.SetString(1, topic);
  +
  +     var AlwaysRaised = true;
  +     try {
  +       pref =
Components.classes["@mozilla.org/preferences;1"].getService(Components.interfaces.nsIPrefService);
  +       AlwaysRaised = pref.GetBoolPref("extension.help.alwaysraised");
  +     } catch (e) {
  +     }
  +
 38     var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
 39                        .getService(Components.interfaces.nsIWindowWatcher);
  +     winfeatures = "chrome,all,dialog=no"
  +     if (AlwaysRaised) winfeatures = winfeatures + ",alwaysRaised"
  -     ww.openWindow(null, MOZ_HELP_URI, "_blank",
"chrome,all,alwaysRaised,dialog=no", params);
  +     ww.openWindow(null, MOZ_HELP_URI, "_blank", winfeatures, params);
 41   }
 42 }

Comment 3

[Dan M]

Note that bug 42557, if it's ever implemented, is just what you're talking about
but more general (for all windows).

Comment 4

[R.J. Keller]

--> me

Comment 5

[R.J. Keller]

Attachment: Patch from Daniel Wang

Good job Daniel!
A couple of problems is that prefs wasn't declared (need to be var prefs), but
with that it looks great! Compiled it and it works fine.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

"because the need of one behaviour or the other depends just on
the situation"

This implies that a pref is not the solution, it implies the real solution is an
API to open the help window either alwaysRaised or not, depending on the context
in which the help window was opened. 

Try again. 

Comment 7

[Jason Kersey]

GAME ON.

Comment 8

[R.J. Keller]

Comment on attachment 130069 [details] [diff] [review]
Patch from Daniel Wang

Sorry all, misinterpreted the report.

Comment 9

[R.J. Keller]

Attachment: Patch

Comment 10

[Dan M]

Well, OK but it's still an incomplete patch. How does one set the second
parameter of openHelp? And Ben's objection to the first patch was that it's a
hidden global preference, while the reporter seemed to be saying that sometimes
you'd want it one way, sometimes another.

The first patch would work. But it's a bit of a hack, so it's not raising many
warm fuzzies. The second patch won't have any actual effect.

Comment 11

[R.J. Keller]

Filed mozdev bug 4445 to work on this issue with the new help menu.
http://mozdev.org/bugs/show_bug.cgi?id=4445

Comment 12

[Brendan Eich [:brendan]]

What's going on at mozdev.org?  Why not work here to fix this bug, or better
yet, to fix bug 42557 as damn suggested in comment 3?

/be

Comment 13

[R.J. Keller]

Brenden, I never said I was not going to fix this. The reason why I put it in
the mozdev database is because there are some people there that might be able to
help me fix this. Not only that, the FirebirdHelp project at mozdev is going to
be the new help window when Mozilla switches to Firebird/Thunderbird. This bug
isn't going to be fixed for 1.5b or 1.5, so I thought it would be a good idea to
note it there. I'm not saying that I'm moving the bug to there.

As for bug 42557, I'm not very experienced with stuff outside the help window. I
was hoping to get a function cooked up that would work for help only. I don't
know much about XP Apps.

Comment 14

[R.J. Keller]

Note to everyone: alwaysRaised is being removed in the help menu rewrite (that's
planning on being checked in for mozilla 1.6 alpha), so something like this will
be unlikely to be added.

If you have a problem with this, talk in newsgroups, not in bug. I monitor
mozilla-documentation newsgroup and I can talk to you there about it.

Comment 15

[R.J. Keller]

WONTFIX. Debate in mozilla-documentation newsgroup if you disagree, but don't
reopen.

Comment 16

[Daniel Wang]

v

Comment 17

[[e-mail]]

Attachment: Patch to dynamically set the zLevel

Based on the work in bug 42557, this provides for a menuitem on the context
menu to demonstrate how the zLevel could be dynamically and persistently
altered.

Comment 18

[R.J. Keller]

Reopening...

Comment 19

[R.J. Keller]

Wow! Great sutff! My comments are below:


+  var webnav = window.getInterface(Components.interfaces.nsIWebNavigation);

Shouldn't it be this?
var webnav = getWebNavigation();

I'm not very familiar with interfaces, so I could be wrong. Don't think we need
to get the interface if it's already gotten.

+  var xulwin = ifreq.getInterface(Components.interfaces.nsIXULWindow);
+  if (xulwin.zLevel > xulwin.normalZ) {

Can we have some spacing and documentation?

I'd like to have a function description for every help function. parameters
should also be explained. Lack of documentation has made it difficult for
contributors to help fix bugs.

Comment 20

[Dan M]

I of course prefer my own UI as mentioned in bug 42557. It's rather the same
thing but more developed. The problem with it of course; actually it's a good
thing, is that it requires window real estate for a little widget. And there
just isn't any space available in the Mozilla Help window. That makes Neil's
solution, which doesn't require real estate, lower impact. But I'd prefer to add
space for a leveling widget to the Help window and adapt the 42557 UI, rather
than add an item to the context menu. I hadn't thought about it much because
this bug had been closed "won't fix."

In Firebird, the 42557 UI is a toolbar button. Which I'm not sure I like. But it
suggests an obvious location in the Help window. The 42557 UI doesn't know about
Help's own toolbar and configuration dialog but that should be fairly easy to
add. I do worry about what happens if the overlays are installed out of order...

PS R.J.: there isn't much in the way of useful commentary one could add to those
two lines. For that first line you could add the comment "get nsIXULWindow
interface" but that's just recapitulating one rather clear line of code. Though
the whole block could reasonably say "set menuitem's checkmark to match the
window's zLevel."

Comment 21

[R.J. Keller]

danm: I'd prefer to have a function description before every function. This one
doesn't have one. I'd also like the spacing.

How about this:
//toggleZLevel - Toggles whether or not the window will always appear on top.
//   menuitem - The menu item to change the checked state (if on top, it's
//              checked, otherwise the check is removed.)

I like the idea of having it in the context as opposed to UI. There's too much
stuff in the UI already.

Comment 22

[[e-mail]]

Attachment: Added comments

Note: I need to get the webNavigation of the chrome window, not the content.
Note: help.dtd is royally broken wrt line endings so the patch looks odd.

Comment 23

[R.J. Keller]

Comment on attachment 133696 [details] [diff] [review]
Added comments

r=[e-mail].

Looks great! Not sure what happen to the DTD file.

Alecf, can you sr?

Comment 24

[Alec Flett]

Comment on attachment 133696 [details] [diff] [review]
Added comments

this looks fine, but is this sequence:

+  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
+  var webnav = window.getInterface(Components.interfaces.nsIWebNavigation);
+  var dsti = webnav.QueryInterface(Components.interfaces.nsIDocShellTreeItem);
+  var treeowner = dsti.treeOwner;
+  var ifreq =
treeowner.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
+  var xulwin = ifreq.getInterface(Components.interfaces.nsIXULWindow);


i.e. converting window -> xulwin repeated somewhere else, i.e. could it be
consolidated into a helper function?

Even if it isn't it seems like it would be a useful chunk of code to move into
a helper function to encourage others to use it later.

sr=alecf with that stuff moved to a helper.

Comment 25

[R.J. Keller]

-> Neil

Comment 26

[R.J. Keller]

Attachment: Patch with alecfs comments.

Does this look good? I'm not that good at interfaces, so I might be getting the
documentation wrong.

Comment 27

[R.J. Keller]

Comment on attachment 133768 [details] [diff] [review]
Patch with alecfs comments.

good Neil? alecf said he sr'ed with this, so all I want is your review before
checkin. Just want to make sure the docs correct before checkin.

Comment 28

[[e-mail]]

Comment on attachment 133768 [details] [diff] [review]
Patch with alecfs comments.

r=me on your new function assuming you tested it.

Comment 29

[Dan M]

I don't think it's terribly important to make a helper function out of the
window-to-XULWindow code. It's not done very often. But a helper function, if it
existed, would be more useful if it could be shared by other Mozilla components.
If you're just splitting it into another function in your Help source it'll be
an underutilized helper function.

Alec has given you the quest of improving the underlying codebase. He was
probably thinking of something perhaps in comm.jar. Possibly, maybe, an
addition to contentAreaUtils.js. Or maybe some new file of general utility
functions in communicator/content. Alec?

You should construct such a helper function slightly differently. It'd be more
efficient script with all the duplicated Components.interfaces.XXX replaced
with CI.XXX, where const CI=Components.interfaces. And since a general helper
function has the potential to be called not from within an actual nsIXULWindow
and you don't want the error thrown in that case to unravel script execution in
an uncontrolled fashion, you want to wrap the whole thing in a try/catch
clause. And there's no need for all those intermediate vars. For an example, see
the source for the 42557 UI.

Context menu, huh?

Comment 30

[Dan M]

About the try/catch thing, on reconsideration I'd leave that out of the helper
function, allowing it to throw an error. I think that'd actually be more useful.
That's not standard practice though, so the function would need to include a
comment about the potential of a thrown error. The other points in my previous
comment though, all want to be addressed in a centrally available helper function.

Comment 31

[R.J. Keller]

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

Comment 32

[R.J. Keller]

> You should construct such a helper function slightly differently. It'd be more
> efficient script with all the duplicated Components.interfaces.XXX replaced
> with CI.XXX, where const CI=Components.interfaces. And since a general helper

I don't think that this would go in this bug.

> function has the potential to be called not from within an actual nsIXULWindow
> and you don't want the error thrown in that case to unravel script execution in
> an uncontrolled fashion, you want to wrap the whole thing in a try/catch
> clause. And there's no need for all those intermediate vars. For an example, see
> the source for the 42557 UI.

I don't think I understand what you're saying here. You want this to be part of
the XULWindow? why? This is only for help. Also, why would this throw an
exception? What kind of exception would be thrown?

Comment 33

[R.J. Keller]

Attachment: Patch

This is the same one as above, except this one can apply. The other patch has
some hunks fail.

Comment 34

[Dan M]

I think Alec wanted something more. This would be a useful chunk of code to
canonify outside of Help. As a local function it's acceptable, but as a
potentially global function, I have issues with its efficiency. But Alec seems
to have gone silent and I don't intend to push for what I believe was his
vision. It's fine as is.

Comment 35

[R.J. Keller]

> I think Alec wanted something more. This would be a useful chunk of code to
> canonify outside of Help. As a local function it's acceptable, but as a
> potentially global function, I have issues with its efficiency. But Alec seems
> to have gone silent and I don't intend to push for what I believe was his
> vision. It's fine as is.

If we wanted to make it a global function, how would we go about doing that? Is
there a JS file that it would go in (or maybe create a new one)? I'd rather get
it done right the first time and make it a benefit to other mozilla apps.

Comment 36

[[e-mail]]

utilityOverlay.xul looks like a good place.

Comment 37

[R.J. Keller]

> utilityOverlay.xul looks like a good place.

Yes, but aren't we doing a function? We would want a JS file. I don't think
we're going to make default UI for this.

Comment 38

[[e-mail]]

D'oh! Try utilityOverlay.js :-[

Comment 39

[Dan M]

I've been meeting offline with Jag. It seems there's universal agreement that
the best extant source file into which this new function could be thrust would
be utilityOverlay.js. There is something of a problem. utilityOverlay.xul/js has
lost some of its original focus, and this is another step in that direction.

There are seven functions already in the .js file that have nothing to do with
the .xul file and therefore really belong in a more generic utility source file:
extractFileNameFromUrl, gatherTextUnder, GenerateValidFilename, goClickThrobber,
goHelpMenu, goPreferences, validateFileName. This new function we're talking
about would be an eighth. It'd be nice to put all eight functions in a new file
of actual utility functions.

Such a reorganization would be cleaner, and would prevent, for example, help.xul
from having utilityOverlay.js' inapplicable startup code running in it, and its
load listener pointlessly attached. However it would be some trouble, requiring
fix-up on 26 source files that reference those functions.

So, short answer, yes, put the new function in utilityOverlay.js. Crib the more
efficient implementation from navigator.js (search for nsIXULWindow). Yes it's
just a two-liner, though it's seven lines' worth of two-liner. I advise not
putting the try/catch in the function, but adding a short explicit comment
stating that the function could throw if there's no XUL window (likely not a
problem for help.xul) or if anything in the long chain of dereferences is broken
for any reason.

Extra credit for cleaning up utilityOverlay.js while you're in there. But not
really expecting that.

Comment 40

[[e-mail]]

So presumably we'd take the code out of navigator.js, leaving just the call to
the method that returns the xul window?

Comment 41

[Dan M]

Oh yeah. Extra credit also for replacing the, errr, four instances of this code
snippet in the source with the new function. I say errr because two of the four
seem unused and could maybe just be removed.

Be careful, though. Sometimes utilityOverlay.js is already included, sometimes
not. For example, communicator/content/nsContextMenu.js could use the new
function, but it's included in
communicator/content/contentAreaContextOverlay.xul, itself an overlay which is
used by other .xul files which themselves sometimes also overlay
utilityOverlay.xul (e.g. editor/content/TextEditorAppShell.xul) and sometimes
not (e.g. messenger/content/addressbook.xul from Thunderbird).

More reason to think about not sharing domiciles with the already somewhat
cranky utilityOverlay.js. Or just leaving it all in the Help family.

Comment 42

[[e-mail]]

Sigh... addressbook has no business overlaying the content context menu...

Comment 43

[Daniel Wang]

The status update www.mozilla.org/status/2003-11-10.html talks about WinLayer.
Neil, know if this extension can be made cross-platform? And do we still need to
fix this bug?

Comment 44

[Dan M]

Daniel: R.J. has already expressed a preference for an unexposed UI in comment 21.

All: You know, this context menu won't work on Linux. On Linux builds, the Help
window won't be raised in the first place, so the context menu item won't be
necessary to solve any problems. But there it'll be, a dead menu item. Nothing
happens when you uncheck or re-check it. That's rather icky.

Comment 45

[[e-mail]]

Well, we can always add extra code to hide it except for mac and win :-/

Comment 46

[R.J. Keller]

> Well, we can always add extra code to hide it except for mac and win :-/

Could you tell me how to do this? I've been trying to figure out how to do this
for some time now. I've noticed  that Firebird has some kind of #ifdef system
going on in the XUL. Not sure if Help is available to this or not. Maybe the JS
code has some kind of #ifdef system or some constant set up that can tell us.

danm, is this only a problem on linux or does it appear on the Mac also? I
assume it wouldn't, but I've learned to assume nothing :).

Comment 47

[[e-mail]]

Well, we could use something similar - try asking bsmedberg about how to use the
chrome preprocessor.

Alternatively, you could add some onload js along these lines:
canToggle = /Win|Mac/.test(navigator.platform);
toggleOnTop.hidden = !canToggle;
toggleSeparator.hidden = !canToggle;

Comment 48

[Dan M]

The #ifdef stuff would be the xul preprocessor. It's used in Firebird and
Thunderbird but at this time it's never used in the Suite. Don't know why. It's
a handy thing and it works; I just tried it. Slap an #ifndef XP_UNIX around the
helpContextOverlay addition (and the .js addition as well if you like) and
invoke the preprocessor by putting a '*' on its line in the jar.mn file, first
column (see for example browser/base/jar.mn).

That or neil's runtime platform test; either way should be fine.

Barring regressions since last time I looked, window layering works well enough
on the Mac that you want to disable this only on unix.

Comment 49

[R.K.Aa.]

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

Comment 50

[[e-mail]]

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

Comment 51

[R.J. Keller]

Attachment: Latest patch

This bug has been around WAY too long :). Here's an updated patch based on the
latest trunk build plus added the preprocessor to fix the platform issue noted
in comment #44.

Comment 52

[Aaron Slunt]

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

Comment 53

[Stefan [:stefanh]]

Attachment: Updated patch

Here's an updated patch. I added some js at the end of the init() function in help.js to hide the menuitem for platforms that don't support alwaysRaised.

Comment 54

[[e-mail]]

Comment on attachment 216276 [details] [diff] [review]
Updated patch

>+<!ENTITY zLevel.label             "Always on Top">
>+<!ENTITY zLevel.accesskey         "T">
Oops, Cut uses T. BFZCPA are also unavailable. W perhaps?

Comment 55

[Stefan [:stefanh]]

Attachment: New version addressing Neil's comment

Used "W"... Transfering sr+.

Comment 56

[Ian Neal]

Comment on attachment 216417 [details] [diff] [review]
New version addressing Neil's comment

>Index: extensions/help/resources/locale/en-US/help.dtd
>===================================================================
>@@ -22,8 +22,12 @@
> 
> <!-- Command keys -->
> <!ENTITY findAgainCmd.commandkey  "g">
> <!ENTITY findAgainCmd.commandkey2 "VK_F3">
> <!ENTITY findPrevCmd.commandkey   "g">
> <!ENTITY findPrevCmd.commandkey2  "VK_F3">
> <!ENTITY findOnCmd.commandkey     "f">
> <!ENTITY printCmd.commandkey      "p">
>+
>+<!-- Context menu -->
>+<!ENTITY zLevel.label             "Always on Top">
>+<!ENTITY zLevel.accesskey         "W">
You might want to make that "w" rather than "W", other than that r=me

Comment 57

[Stefan [:stefanh]]

Attachment: New version addressing Ian's comment

Correct version using "w" instead of "W". Transfering r/sr and asking for approval sm 1.1a. This is well-tested (toolkit have been using it for a long time) and I think we've had a few complaints about the Help window's "On top" behaviour.

Comment 58

[Karsten Düsterloh]

Checked in on trunk and MOZILLA_1_8_BRANCH.

Comment 59

[Stefan [:stefanh]]

--> Fixed.

Comment 60

[Stefan [:stefanh]]

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

Comment 61

[Nickolay_Ponomarev]

don't we prefer removeAttribute to element.setAttribute("checked", "false")?

Comment 62

[Bruno 'Aqualon' Escherl]

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

Comment 63

[David E. Ross]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1

The help window still remains on top.  

Comment 64

[Bruno 'Aqualon' Escherl]

(In reply to comment #63)
> The help window still remains on top.  
You have to uncheck "Always on Top" in the context menu.

Comment 65

[Ian Neal]

(In reply to comment #63)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111
> SeaMonkey/1.1
> 
> The help window still remains on top.  
> 
David, could you confirm you have unchecked "Always on Top" in the context menu as suggested by Bruno?

Comment 66

[David E. Ross]

Oops!  However, that is truly not obvious.  

Closing.