181721 - Disable menus commands when sheets or dialogs are frontmost

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect, P3)

Description

[Stephane Moureau]

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

[S Woodside]

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

[Mike Pinkerton (not reading bugmail)]

hahah cute.

Comment 3

[Samuel Sidler (old account; do not CC)]

This is still present and should definitely be fixed by 1.0. :)

Comment 4

[Simon Fraser [no longer active]]

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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[Simon Fraser [no longer active]]

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

Comment 7

[Simon Fraser [no longer active]]

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

[Brad Kemper]

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

[Simon Fraser [no longer active]]

(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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 12

[Chris Lawson (gone)]

Taking.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 14

[Stuart Morgan]

Taking; I'll fix this after bug 159337 lands.

Comment 15

[Stuart Morgan]

Attachment: 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

[Stuart Morgan]

Attachment: full fix

Missed a file.

Comment 17

[Chris Lawson (gone)]

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

[Stuart Morgan]

(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

[Chris Lawson (gone)]

(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

[Stuart Morgan]

So is there still a reason this is minused?

Comment 21

[Chris Lawson (gone)]

Comment on attachment 245743 [details] [diff] [review]
full fix

No. :)

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(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

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 245743 [details] [diff] [review]
full fix

sr=pink

Comment 24

[Stuart Morgan]

Attachment: unbitrotted

As checked in on trunk and MOZILLA_1_8_BRANCH.