267227 - (baseMenuOverlay) be able to use Window (mac only) and Help (all) menus for every window (view source, bookmarks manager, etc.) with menus

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P1)

Description

[David Catmull]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3) Gecko/20041026 Firefox/1.0RC1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3) Gecko/20041026 Firefox/1.0RC1

When a "view source" window is frontmost, all menus to the right of View
disappear, including Window and Help which should always be there.

Reproducible: Always
Steps to Reproduce:
1. Select "View Source" on any page
Actual Results:  
Go, Bookmarks, Tools, Window, and Help menus disappear.

Expected Results:  
At least Window and Help should stay.

IMO irrelevant menus should be disabled, not removed; the contents of the menu
bar should never change. On the other hand, that's probably easier said than
done with all the cross-platform stuff going on.

Comment 1

[Asaf Romano (gone)]

Attachment: first-part: baseMenuOverlay

This patch moves window, help and Firefox->Preferences (mac) to a separate
overlay. With this patch, either app windows or extentions windows can use
these menus.

This also fixes two mac-menu bugs for clients of this overlay:
 -- About menu item doesn't work
 -- Preferences menu item is disabled.

After this one is landed i will produce a patch for the relevant windows.

(Also need to patch tbird/sbird to use macWindowMenu.inc)

Comment 2

[Steffen Wilberg]

Comment on attachment 167736 [details] [diff] [review]
first-part: baseMenuOverlay

>Index: browser/base/content/baseMenuOverlay.xul
>+#ifdef XP_MACOSX
...
>+<!-- Mac window menu -->
>+#include ../../../toolkit/content/macWindowMenu.inc
>+#endif
This is the only place you use macWindowMenu.inc right now. Do you plan to use
it in other places? I thought you wanted to use baseMenuOverlay.xul for other
windows?

>Index: browser/base/content/browser-sets.inc
>+# Used by macWindowMenu.inc
>+  <commandset id="macWindowMenuCommandSet" />
>+  <keyset id="macWindowMenuKeyset" />
Remove the spaced before the slashes. That's a XHTML compatibility hack and is
not to be used in XML/XUL.

This is not your code, but can you explain what macBrowserOverlay.xul does as
an overlay to browser.xul? Why can't browser.xul include the *.inc files itself
as on the other platforms?

Comment 3

[Asaf Romano (gone)]

(In reply to comment #2)
> (From update of attachment 167736 [details] [diff] [review])
> >Index: browser/base/content/baseMenuOverlay.xul
> >+#ifdef XP_MACOSX
> ...
> >+<!-- Mac window menu -->
> >+#include ../../../toolkit/content/macWindowMenu.inc
> >+#endif
> This is the only place you use macWindowMenu.inc right now.Do you plan to use
> it in other places? I thought you wanted to use baseMenuOverlay.xul for other
> windows?

I do plan to use both, baseMenuOverlay for ff, and macWindowMenu in a similar
tbird and sbird overlays. see last two lines in comment 1 ;)

> >Index: browser/base/content/browser-sets.inc
> >+# Used by macWindowMenu.inc
> >+  <commandset id="macWindowMenuCommandSet" />
> >+  <keyset id="macWindowMenuKeyset" />
> Remove the spaced before the slashes. That's a XHTML compatibility hack and is
> not to be used in XML/XUL.
> 
> This is not your code, but can you explain what macBrowserOverlay.xul does as
> an overlay to browser.xul? Why can't browser.xul include the *.inc files itself
> as on the other platforms?
> 

see hiddenWindow.xul for mac hacks. Actually, I am working on a new hiddenWindow
xul file (in order to fix some "no window is open" bugs) which will make
macBrowserOverlay obselte.

Comment 4

[Asaf Romano (gone)]

Attachment: first-part: baseMenuOverlay v2 (Kill helpMenuOverlay)

(and remove helpMenuOverlay.xul/dtd)

Comment 5

[Asaf Romano (gone)]

Attachment: first-part: v3

nits fixed

Comment 6

[Asaf Romano (gone)]

Attachment: bookmarks manger patch

Comment 7

[O. Atsushi (Torisugari)]

(In reply to comment #5)
IMHO, any overlay is blaot.
I don't know why mac is using overlay,
but we can use *.inc files instead.

Comment 8

[Mike Connor [:mconnor]]

using .inc files isn't always the best option, especially in cases where we want
to allow windows from extensions etc to add this on easily

Comment 9

[Asaf Romano (gone)]

Comment on attachment 167897 [details] [diff] [review]
first-part: v3

Thanks Steffen!

>+# Disable For IE Users menu item on Unix systems.
>+#ifndef XP_UNIX
>+#ifndef XP_OS2
>+            <menuitem label="&helpForIEUsers.label;"
>+                      accesskey="&helpForIEUsers.accesskey;"
>+                      oncommand="openHelp('ieusers');"/>
>+#endif
>+#endif

going to change it to XP_WIN|XP_MACOSX, since our documentaion (now) covers
MacIE as well.

>+            <menuseparator/>
>+            <menuitem accesskey="&promote.accesskey;"
>+                      label="&promote.label;"
>+                      oncommand="openUILink(getUILink('promote'), event, false, true);"
>+                      onclick="checkForMiddleClick(this, event);"/> -->

hell with typos :-/

Comment 10

[Steffen Wilberg]

> going to change it to XP_WIN|XP_MACOSX, since our documentaion (now) covers
> MacIE as well.
I've talked with Mano about this. We agreed to not introduce a "for IE users"
menu item on Mac, because IE's marketshare is significantly less on Mac.
So instead of "not Unix (including Mac), not OS/2", we propose to change this to
#ifdef XP_WIN.

Comment 11

[Asaf Romano (gone)]

Attachment: first-part: v4

up to date; address typo and cleanup #ifdefs in baseMenuOverlay.

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 170102 [details] [diff] [review]
first-part: v4


>Index: browser/base/content/baseMenuOverlay.xul
>===================================================================
>RCS file: browser/base/content/baseMenuOverlay.xul
>diff -N browser/base/content/baseMenuOverlay.xul
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/base/content/baseMenuOverlay.xul	2 Jan 2005 22:38:31 -0000
>@@ -0,0 +1,113 @@
>+<?xml version="1.0"?>
>+
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+# 
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+# 
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+# 
>+# The Original Code is Mozilla.org Code.
>+# 
>+# The Initial Developer of the Original Code is.
>+# Portions created by the Initial Developer are Copyright (C) 2004
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Asaf Romano <[e-mail]>
>+# 
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the LGPL or the GPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+# 
>+# ***** END LICENSE BLOCK *****

Initial developer should be filled in.

>+<!DOCTYPE overlay [
>+<!ENTITY % brandDTD SYSTEM "chrome://global/locale/brand.dtd" >
>+%brandDTD;
>+<!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">
>+%browserDTD;
>+]>
>+<overlay id="baseMenuOverlay"
>+         xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+<script type="application/x-javascript" src="chrome://browser/content/utilityOverlay.js"/>
>+<script type="application/x-javascript" src="chrome://help/content/contextHelp.js"/>
>+
>+#ifdef XP_MACOSX
>+<!-- nsMenuBarX moves menu_preferences to the Application Menu -->
>+    <menupopup id="menu_ToolsPopup">
>+        <menuitem id="menu_preferences" oncommand="openPreferences();"/>
>+    </menupopup>
>+<!-- Mac window menu -->
>+#include ../../../toolkit/content/macWindowMenu.inc
>+#endif
>+
>+    <script type="application/x-javascript">
>+    <![CDATA[
>+      function openAboutDialog()
>+      {
>+        window.openDialog("chrome://browser/content/aboutDialog.xul",
>+            "About", "modal,centerscreen,chrome,resizable=no");
>+      }
>+    ]]>
>+    </script>

why is this inline and not in a .js file?  utilityOverlay.js would be
appropriate, and used everywhere already.

>--- browser/base/content/browser-sets.inc	21 Dec 2004 15:30:57 -0000	1.31
>+++ browser/base/content/browser-sets.inc	2 Jan 2005 22:38:34 -0000
> 
>+# Used by baseMenuOverlay
>+  <commandset id="baseMenuCommandSet" />

as this is only used on mac, #ifdef XP_MACOSX please

>+  <keyset id="baseMenuKeyset" />

>Index: browser/locales/en-US/chrome/browser/browser.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v
>retrieving revision 1.9
>diff -u -r1.9 browser.dtd
>--- browser/locales/en-US/chrome/browser/browser.dtd	30 Nov 2004 08:22:50 -0000	1.9
>+++ browser/locales/en-US/chrome/browser/browser.dtd	2 Jan 2005 22:38:53 -0000
>@@ -386,3 +386,11 @@
> <!ENTITY findAgainCmd.accesskey "g">
> <!ENTITY findAgainCmd.commandkey "g">
> <!ENTITY findAgainCmd.commandkey2 "VK_F3">
>+
>+<!ENTITY helpContents.label       "Help Contents">
>+<!ENTITY helpContents.accesskey   "H">
>+<!ENTITY helpForIEUsers.label     "For Internet Explorer Users">
>+<!ENTITY helpForIEUsers.accesskey "I">
>+<!ENTITY openHelp.commandkey      "VK_F1">
>+<!ENTITY openHelpMac.commandkey   "VK_HELP">
>+

This is forcing all consumers of the overlay to include browser.dtd.  This
should be in a new baseMenuOverlay.dtd instead.

>Index: toolkit/content/macWindowMenu.inc
>===================================================================
>RCS file: toolkit/content/macWindowMenu.inc
>diff -N toolkit/content/macWindowMenu.inc
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/content/macWindowMenu.inc	2 Jan 2005 22:40:38 -0000
>@@ -0,0 +1,48 @@
>+    <script type="application/x-javascript">
>+    <![CDATA[
>+      function zoomWindow()
>+      {
>+        if (window.windowState == STATE_NORMAL)
>+          window.maximize();
>+        else
>+          window.restore();
>+      }
>+    ]]>
>+    </script>

I'm not a fan of the inline bit here, but there really isn't a place worth
putting it.

Comment 13

[Asaf Romano (gone)]

Attachment: v5

Comment 14

[Asaf Romano (gone)]

Attachment: v5.1

oops! i forgot to include the other three window menu functions.

At some point, we should move them to a coreFunctions.js (in tookit) or
something similar.

Comment 15

[O. Atsushi (Torisugari)]

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

Comment 16

[Asaf Romano (gone)]

Attachment: v.5.2

up-to-date

Comment 17

[Asaf Romano (gone)]

Comment on attachment 172435 [details] [diff] [review]
v.5.2

I need to update this patch...

Comment 18

[Asaf Romano (gone)]

Attachment: v6

*cough*

Changes:
  * ...update to tip
  * separate the macWindowMenu functions into a new file
  * "workaorun" to hide the about menu item on (it's in the application menu)
  * Chnage the HelpContents label on mac to "Firefox Help" per hig

Comment 19

[Asaf Romano (gone)]

Attachment: [checked in] v6.1 - update to tip

http://tinyurl.com/43pmh

regression are of course against me.

Comment 20

[Asaf Romano (gone)]

Fixed, I'll open separate bugs for the relevant windows.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 178481 [details] [diff] [review]
[checked in] v6.1 - update to tip 

>Index: browser/base/content/baseMenuOverlay.xul
>===================================================================
>+# The Initial Developer of the Original Code is Netscape
>+# Communications Corporation. Portions created by Netscape are
>+# Copyright (C) 1998-2000 Netscape Communications Corporation. All
>+# Rights Reserved.

Is this really Netscape code? If you wrote the file from scratch, put your own
name in as original developer, or your employer, if they own your work.

Also, it's rather important to the people updating licenses (Gerv) that you use
the exact boilerplate text at http://www.mozilla.org/MPL/boilerplate-1.1/ .

Comment 22

[Asaf Romano (gone)]

(In reply to comment #21)
> (From update of attachment 178481 [details] [diff] [review] [edit])
> >Index: browser/base/content/baseMenuOverlay.xul
> >===================================================================
> >+# The Initial Developer of the Original Code is Netscape
> >+# Communications Corporation. Portions created by Netscape are
> >+# Copyright (C) 1998-2000 Netscape Communications Corporation. All
> >+# Rights Reserved.

> Is this really Netscape code? If you wrote the file from scratch, put your own
> name in as original developer, or your employer, if they own your work.

There's still a lot of code there from Netscpae, so: no.
 
> Also, it's rather important to the people updating licenses (Gerv) that you use
> the exact boilerplate text at http://www.mozilla.org/MPL/boilerplate-1.1/ .

have i used something else?

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #22)
> There's still a lot of code there from Netscpae, so: no.

Ok, just checking.

> have i used something else?

Yes, the format of the part I quoted in the last comment differs from the
boilerplate.

Comment 24

[Asaf Romano (gone)]

FWIW, short draft-doc for extension authors:
http://mano.fastmail.fm/Mozilla/documentation/baseMenuOverlay.html

Comment 25

[Peter van der Woude [:Peter6]]

is there a way to link to a relevant help subject ?
e.g. if I open help from the BM window it opens help at a page about Bookmarks
(about BM would be better, but that subject isn't there yet)