Last Comment Bug 25287 - Inappropriate menu items show as enabled when no windows are open
: Inappropriate menu items show as enabled when no windows are open
Status: RESOLVED FIXED
: fixed-seamonkey1.1
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: PowerPC Mac OS X
: P1 normal (vote)
: seamonkey1.1final
Assigned To: Stefan [:stefanh]
:
:
Mentors:
: 14180 107190 (view as bug list)
Depends on:
Blocks: 4252 261030
  Show dependency treegraph
 
Reported: 2000-01-27 11:36 PST by kirbyt
Modified: 2006-12-02 05:28 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix the major issues (when no windows are open) (10.55 KB, patch)
2006-11-08 14:35 PST, Stefan [:stefanh]
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review
Updated patch (10.63 KB, patch)
2006-11-19 15:53 PST, Stefan [:stefanh]
stefanh: review+
stefanh: superreview+
Details | Diff | Splinter Review
Follow-up patch (use openTopWin on fallback as well) (1.21 KB, patch)
2006-11-23 16:09 PST, Stefan [:stefanh]
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Patch for branch (11.21 KB, patch)
2006-11-27 13:34 PST, Stefan [:stefanh]
kairo: approval‑seamonkey1.1+
Details | Diff | Splinter Review

Description kirbyt 2000-01-27 11:36:00 PST
The following menu items should be disabled (greyed out) when there is no 
browser window open:

File
   Save Page As...
   Send Link

View Menu
   Enlarge Text Size
   Reduce Text Size
   Reload
   Show Images
   Stop
   Page Source
   Page Info

Search
   Find on this page...
   Find again
Comment 1 Eli Goldberg 2000-01-31 13:31:15 PST
QA Assigning to Sarah.
Comment 2 sairuh (rarely reading bugmail) 2000-01-31 15:38:24 PST
should this go to the XPApps folks instead...?
Comment 3 don 2000-01-31 21:35:21 PST
Let's try to fix this for beta 1 if we get a chance.  Low priority tho' ...
Comment 4 Ben Goodger (use ben at mozilla dot org for email) 2000-03-22 21:45:17 PST
not a priority, pushing out as far as possible.
Comment 5 don 2000-05-25 14:12:21 PDT
Move to M21 target milestone.
Comment 6 Ben Goodger (use ben at mozilla dot org for email) 2000-10-15 19:24:55 PDT
Other components get around this by using the command updater, and Navigator 
doesn't use this at all... o_O

Should look at this for the next release. 
Comment 7 Ben Goodger (use ben at mozilla dot org for email) 2000-10-15 20:28:10 PDT
targetting mozilla/1.0, setting severity to normal. 
Comment 8 Adam 2000-11-18 19:42:02 PST
This bug is a dup of 14180 although the comments here are more recent and this
actually has a valid target MS so marking the other way around.
Comment 9 Adam 2000-11-18 19:43:26 PST
*** Bug 14180 has been marked as a duplicate of this bug. ***
Comment 10 Paul Chen 2000-12-28 10:27:01 PST
nav triage team:

Should fix this, marking nsbeta1, reassigning to pchen
Comment 11 selmer (gone) 2001-02-05 11:31:34 PST
We're past time to cut these low priority bugs from mozilla0.8.  Please update
these bugs today. 
Comment 12 Paul Chen 2001-02-05 16:35:48 PST
Talked to pinkerton about this, need to play with hiddenWindow.xul which
controls the menubar when there are no windows on Mac. Sounds simple enough. ;-)
Comment 13 Paul Chen 2001-02-07 13:29:31 PST
Although not a perfect dup, it's pretty much the same problem as 50877, just
need to apply the fix to both view source and hidden window, and I just don't
have the heart to nsbeta1- this one.

*** This bug has been marked as a duplicate of 50877 ***
Comment 14 sairuh (rarely reading bugmail) 2002-01-03 20:01:59 PST
mass verification of duplicate bugs: to find all bugspam pertaining to this, set
your search string to "DuplicateBugsBelongInZahadum".

if you think this particular bug is *not* a duplicate, please provide a
compelling reason, as well as check a recent *trunk* build (on the appropriate
platform[s]), before reopening.
Comment 15 Matthew Paul Thomas 2002-04-10 13:45:26 PDT
Bug 50877 was fixed by adding a menu bar to the view source window. This bug
remained unfixed. Reopening.
Comment 16 Matthew Paul Thomas 2002-04-10 13:48:44 PDT
*** Bug 107190 has been marked as a duplicate of this bug. ***
Comment 17 Andrew Hagen 2002-04-10 18:52:30 PDT
We need a new target milestone on this one.
Comment 18 Peter Trudelle 2002-04-15 15:03:14 PDT
->ben
Comment 19 Judson Valeski 2002-05-30 07:34:43 PDT
minusing this until there's some traction (like a patch). I don't think we'd
hold 1.0.1 for this, but, if an easy patch becomes available, please re-nominate.
Comment 20 Greg K. 2003-04-14 00:28:27 PDT
Using FizzillaMach/2003-04-11-08-trunk, the following menu items are
inappropriately shown with no windows open.

File               Edit                View
 Close Other Tabs   Find in This Page   Show/Hide
 Save Page As       Find Again          Reload
 Send Page          Find Previous       Use Style
 Send Link                              Character Coding
 Print Preview                          Page Source
                                        Page Info

Bookmarks
 Bookmark This Group of Tabs

Reassigning to XP Toolkit/Widgets: Menus.
Comment 21 Stefan [:stefanh] 2006-11-04 13:21:58 PST
I'm working on this...
Comment 22 Stefan [:stefanh] 2006-11-04 13:30:21 PST
Apart from menuitems that should be disabled, there are a few menuitems that should work (but doesn't) when all windows are closed (not talking about Debug and QA menus), those are:
 - View --> Apply Theme --> Get New Themes
 - Go --> Home
 - Search the Web
Comment 23 Stefan [:stefanh] 2006-11-08 14:35:36 PST
Created attachment 245032 [details] [diff] [review]
Fix the major issues (when no windows are open)

This patch will also fix bug 114967, bug 83253 and parts of bug 130891.

It will:

1) Disable inappropiate menuitems when all windows are closed. I have added id's to a few menuitems. I don't know if it's worth the extra lines to use a <command/> for Reload etc (for branch as well), but I could of course do it if someone insists (at least for Reload).

2) Fix the following js errors:

Opening the bookmarks menu (updateGroupmarkCommand):
Error: gBrowser has no properties
Source File: chrome://navigator/content/navigator.js
Line: 1017

(Since the menuitem now gets disabled by the js I removed it in the disabledItems array)

Tools --> Popup Manager (createShowPopupsMenu):
Error: getBrowser() has no properties
Source File: chrome://navigator/content/navigator.js
Line: 2427

3) Fix a few menuitems that should work when all windows are closed( "Go --> Home", "View --> Apply Theme --> Get New Themes" and "Tools --> Search the Web". This patch makes them work - mainly by using openTopWin instead of loadURI (I also needed a few more stringbundles etc).


There are a few more things to fix, but  I'll leave that for other bugs (Debug, QA menu and a js error from calling the updateGoMenu function (it's defined in sessionHistory.js...). We should also fix the Open Web Location dialog (open in current window/tab).

Karsten, the patch applies on branch as well (some lines offset) if you prefer to test it on a branch build. I haven't used branch for testing, but when I wanted to see the Open Web Location sheet and verifying that theme switching worked I had to use a trunk build with the old toolkit :-(
Comment 24 Stefan [:stefanh] 2006-11-08 14:36:51 PST
(sorry for spam)
Comment 25 neil@parkwaycc.co.uk 2006-11-09 06:19:27 PST
Comment on attachment 245032 [details] [diff] [review]
Fix the major issues (when no windows are open)

>   var target = aEvent ? BookmarksUtils.getBrowserTargetFromEvent(aEvent) : "current";
>+  var browser = getBrowser();
>+  // Force target if we don't have a browser window open.
>+  if (!browser)
>+    target = "window";
> 
>   if (homePage.length == 1) {
>     switch (target) {
>     case "current":
>       loadURI(homePage[0]);
>       break;
>     case "tab":
>       tab = gBrowser.addTab(homePage[0]);
As this function already uses gBrowser, I don't see why you can't use it above, and then you could folder it into the ?: i.e.
var target = gBrowser ? aEvent ? BookmarksUtils.getBrowserTargetFromEvent(aEvent) : "current" : "window";
or maybe you would prefer to reverse the tests:
var target = !gBrowser ? "window": !aEvent ? "current" : BookmarksUtils.getBrowserTargetFromEvent(aEvent);
Comment 26 Stefan [:stefanh] 2006-11-12 12:30:59 PST
Comment on attachment 245032 [details] [diff] [review]
Fix the major issues (when no windows are open)

I'll update the patch with one of Neil's suggestions after it has been reviewed.
Comment 27 Stefan [:stefanh] 2006-11-16 10:28:17 PST
Note to self: this will also fix bug 83343.
Comment 28 Stefan [:stefanh] 2006-11-16 10:35:31 PST
(In reply to comment #27)
> Note to self: this will also fix bug 83343.
> 
and bug 147237
Comment 29 Karsten Düsterloh 2006-11-19 14:28:21 PST
Comment on attachment 245032 [details] [diff] [review]
Fix the major issues (when no windows are open)

>Index: suite/browser/navigator.js
>===================================================================
> function updateGroupmarkCommand()
> {
>-  const disabled = gBrowser.browsers.length == 1;
>+  const disabled = (!gBrowser || gBrowser.browsers.length == 1);
>   document.getElementById("Browser:AddGroupmarkAs")
>           .setAttribute("disabled", disabled);

You probably may want to remove the disabled attribute instaed of setting it to false.

r=me either way.
Comment 30 Stefan [:stefanh] 2006-11-19 15:53:08 PST
Created attachment 245983 [details] [diff] [review]
Updated patch

Updated patch (Used Neil's second suggestion from comment #25). Moving review flags.
Comment 31 Stefan [:stefanh] 2006-11-19 16:10:31 PST
Fix checked in by ajschult (thanks!).
Comment 32 Stefan [:stefanh] 2006-11-22 08:56:41 PST
Comment on attachment 245983 [details] [diff] [review]
Updated patch

This fixes some of the major issues when no windows are open on mac. I'd regard this as safe (and  the patch have baked a few days on trunk as well).
Comment 33 Robert Kaiser 2006-11-22 14:24:07 PST
Comment on attachment 245983 [details] [diff] [review]
Updated patch

first-a=me for SeaMonkey 1.1
Comment 34 Stefan [:stefanh] 2006-11-23 16:09:40 PST
Created attachment 246442 [details] [diff] [review]
Follow-up patch (use openTopWin on fallback as well)

1250   // Fallback if the stuff above fails: use the hard-coded search engine
1251   loadURI(gNavigatorRegionBundle.getString("otherSearchURL"));

I forgot to change the loadURI call here - this patch does it :-/
Comment 35 Stefan [:stefanh] 2006-11-25 06:33:09 PST
Re-opening, need the last patch to go in before this is  really fixed. Neil, can you check it in please?
Comment 36 Stefan [:stefanh] 2006-11-27 13:29:03 PST
Comment on attachment 245983 [details] [diff] [review]
Updated patch

I'll post a "real" branch patch.
Comment 37 Stefan [:stefanh] 2006-11-27 13:34:27 PST
Created attachment 246705 [details] [diff] [review]
Patch for branch

Here's a diff against branch, including the  follow-up patch. I consider this a quite important improvement for mac users - nearly all the issues in the menubar when no windows are opened are fixed now.
Comment 38 Stefan [:stefanh] 2006-11-27 16:06:38 PST
(In reply to comment #25)
> (From update of attachment 245032 [details] [diff] [review] [edit])
> >   var target = aEvent ? BookmarksUtils.getBrowserTargetFromEvent(aEvent) : "current";
> >+  var browser = getBrowser();
> >+  // Force target if we don't have a browser window open.
> >+  if (!browser)
> >+    target = "window";
> > 
> >   if (homePage.length == 1) {
> >     switch (target) {
> >     case "current":
> >       loadURI(homePage[0]);
> >       break;
> >     case "tab":
> >       tab = gBrowser.addTab(homePage[0]);
> As this function already uses gBrowser, I don't see why you can't use it above,
> and then you could folder it into the ?: i.e.
> var target = gBrowser ? aEvent ?
> BookmarksUtils.getBrowserTargetFromEvent(aEvent) : "current" : "window";
> or maybe you would prefer to reverse the tests:
> var target = !gBrowser ? "window": !aEvent ? "current" :
> BookmarksUtils.getBrowserTargetFromEvent(aEvent);
> 

Neil,
Home now works from downloadmanager. This means that if you select Home when downloadmanager is open (and you have a browser window open) the homepage will load in a new browser window. Note that this is probably because I picked the second suggestion (the first suggestion would probably make a browserHome call from downloadmanager fail because DM doesn't know about BookmarksUtils). It's a little bit late to start thinking of a solution, though. Maybe check if window.location.href is hiddenWindow.xul?
Comment 39 Robert Kaiser 2006-11-28 06:07:15 PST
Is bug 362055 related to this? If so, could some of you mac people resolve it accordingly? Thanks.
Comment 40 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-11-28 15:39:11 PST
Comment on attachment 246705 [details] [diff] [review]
Patch for branch

first-a=me for 1.1; one more needed.
Comment 41 Stefan [:stefanh] 2006-11-30 23:35:25 PST
KaiRo, I don't know if CTho missed comment #33 (which is basically the same patch, except for the follow-up) or if he wanted another Council member's opinion.
Comment 42 Robert Kaiser 2006-12-01 04:59:29 PST
Comment on attachment 246705 [details] [diff] [review]
Patch for branch

I think he missed that one ;-)
Comment 43 Stefan [:stefanh] 2006-12-01 05:22:21 PST
(In reply to comment #42)
> (From update of attachment 246705 [details] [diff] [review] [edit])
> I think he missed that one ;-)

Thanks :-)
Comment 44 Karsten Düsterloh 2006-12-01 16:54:58 PST
Comment on attachment 246705 [details] [diff] [review]
Patch for branch

Landed this on MOZILLA_1_8_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.