Last Comment Bug 181721 - Disable menus commands when sheets or dialogs are frontmost
: Disable menus commands when sheets or dialogs are frontmost
Status: RESOLVED FIXED
: fixed1.8.1.2
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
P3 minor (vote)
: Camino1.5
Assigned To: Stuart Morgan
:
:
Mentors:
: 187849 329331 333438 347630 (view as bug list)
Depends on:
Blocks: 341853
  Show dependency treegraph
 
Reported: 2002-11-24 03:58 PST by Stephane Moureau
Modified: 2006-12-27 10:18 PST (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (15.22 KB, patch)
2006-11-15 22:06 PST, Stuart Morgan
no flags Details | Diff | Splinter Review
full fix (15.91 KB, patch)
2006-11-15 23:31 PST, Stuart Morgan
bugzilla-graveyard: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
unbitrotted (14.15 KB, patch)
2006-12-27 10:17 PST, Stuart Morgan
no flags Details | Diff | Splinter Review

Description User image Stephane Moureau 2002-11-24 03:58:39 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021122 Chimera/0.6+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021122 Chimera/0.6+

"Sheet Behavior
...
Only one sheet may be open for a window at any one time. A sheet PREVENTS ANY
OTHER OPERATION on that window until the sheet is dismissed. If, when the user
responds to a sheet, another sheet for that document must open, the first sheet
closes before the second one opens."

Reproducible: Always

Steps to Reproduce:
Open a New Window with the Toolbar and Location field.
Reduce its size to the minimum with the Grow icon.
Click on the double arrow in the Toolbar and select Location.
Min/Max-imizes, Resize, Save, Print, Find, Go, Bookmarks commands are available.
Comment 1 User image S Woodside 2002-11-24 19:06:43 PST
confirmed in 2002111604. Severity to trivial (it was enhancement)... There's no
harm done by this bug AFAICT, although if/when sheets are used for printing
there might be more of an issue.
Comment 2 User image Mike Pinkerton (not reading bugmail) 2003-08-11 18:32:19 PDT
hahah cute.
Comment 3 User image Samuel Sidler (old account; do not CC) 2005-07-28 22:28:22 PDT
This is still present and should definitely be fixed by 1.0. :)
Comment 4 User image Simon Fraser 2006-01-28 17:07:05 PST
I don't see any harm in this; I wasn't able to get into a locked state with the fix for bug 314072 in my tree.
Comment 5 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-01-28 20:05:46 PST
For polish, we should probably grey out some additional menu items that currently just beep (Open File�), but I can't get into a locked state anywhere, either.
Comment 6 User image Simon Fraser 2006-01-28 21:15:08 PST
*** Bug 187849 has been marked as a duplicate of this bug. ***
Comment 7 User image Simon Fraser 2006-01-28 21:16:15 PST
I disabled openFile: and openLocation: when the frontmost window has a sheet up. Loading bookmarks seems harmless, and as we don't block all the other various ways you can change the browser content, I don't see a compelling reason to disable bookmarks.
Comment 8 User image Brad Kemper 2006-01-29 00:35:35 PST
Really? You can load bookmarks and change the browser content when a sheet is open on that page? Doesn't that fall under the "ANY OTHER OPERATION" restriction?
Comment 9 User image Simon Fraser 2006-01-29 09:36:38 PST
(In reply to comment #8)
> Really? You can load bookmarks and change the browser content when a sheet is
> open on that page? Doesn't that fall under the "ANY OTHER OPERATION"
> restriction?

Yes, but rather than disable all the commands that can possibly change the window ocontent, and risk introducing bugs at this stage in the release (close to 1.0), I just fixed the most obvious issue. That's why this bug is still open.
Comment 10 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-03-04 09:23:01 PST
*** Bug 329331 has been marked as a duplicate of this bug. ***
Comment 11 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-20 22:52:45 PDT
*** Bug 333438 has been marked as a duplicate of this bug. ***
Comment 12 User image Chris Lawson (gone) 2006-06-21 20:13:46 PDT
Taking.
Comment 13 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-06 17:41:41 PDT
*** Bug 347630 has been marked as a duplicate of this bug. ***
Comment 14 User image Stuart Morgan 2006-10-25 23:13:58 PDT
Taking; I'll fix this after bug 159337 lands.
Comment 15 User image Stuart Morgan 2006-11-15 22:06:25 PST
Created attachment 245736 [details] [diff] [review]
fix

This turned into sort of a game of whack-a-mole once I realized that all the context menus needed disabling too. This should cover everything with two exceptions:
- Use as Dock Menu, which works and is harmless, and therefore doesn't seem worth adding a menu validator to the bookmark class or a passthrough in some other class for.
- Show/Hide Toolbar, which Cocoa should be handling for us, and I can't figure out how we're confusing it. It doesn't do any harm, so we can file it as a low-priority follow-up bug.
Comment 16 User image Stuart Morgan 2006-11-15 23:31:36 PST
Created attachment 245743 [details] [diff] [review]
full fix

Missed a file.
Comment 17 User image Chris Lawson (gone) 2006-11-17 18:01:32 PST
Comment on attachment 245743 [details] [diff] [review]
full fix

>@@ -1486,19 +1498,34 @@
> 
>   // >NSLog(@"MainController validateMenuItem for %@ (%s)", [aMenuItem title], action);
> 
>+  // disable window-related menu items if a sheet is up
>+  if (browserController && [[browserController window] attachedSheet] &&

Patch looks good, except it fails to account for sheets on non-browser windows.

For example, on the Downloads window, you can Customise Toolbar to get a sheet, and observe that nothing gets disabled.

r=me with that issue addressed.
Comment 18 User image Stuart Morgan 2006-11-17 18:11:17 PST
(In reply to comment #17)
> Patch looks good, except it fails to account for sheets on non-browser windows.
> 
> For example, on the Downloads window, you can Customise Toolbar to get a sheet,
> and observe that nothing gets disabled.

Erm... what exactly are you looking for to be disabled when there's a sheet on the downloads window? None of the loading sorts of things apply to it.
Comment 19 User image Chris Lawson (gone) 2006-11-17 18:16:14 PST
(In reply to comment #18)
> Erm... what exactly are you looking for to be disabled when there's a sheet on
> the downloads window? None of the loading sorts of things apply to it.

Yeah, upon looking over the File menu again, I guess none of that stuff really needs to be disabled anyway (although I still think it's weird that stuff like "Search the Web" and "Open Location..." are active when a non-browser window is frontmost).

cl
Comment 20 User image Stuart Morgan 2006-11-17 21:45:01 PST
So is there still a reason this is minused?
Comment 21 User image Chris Lawson (gone) 2006-11-18 08:10:22 PST
Comment on attachment 245743 [details] [diff] [review]
full fix

No. :)
Comment 22 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-11-18 10:11:33 PST
(In reply to comment #19)
> (although I still think it's weird that stuff like
> "Search the Web" and "Open Location..." are active when a non-browser window is
> frontmost).

So you can get focus back to the browser window :P
Comment 23 User image Mike Pinkerton (not reading bugmail) 2006-12-23 10:05:06 PST
Comment on attachment 245743 [details] [diff] [review]
full fix

sr=pink
Comment 24 User image Stuart Morgan 2006-12-27 10:17:46 PST
Created attachment 249776 [details] [diff] [review]
unbitrotted

As checked in on trunk and MOZILLA_1_8_BRANCH.

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