Closed Bug 181721 Opened 17 years ago Closed 13 years ago

Disable menus commands when sheets or dialogs are frontmost

Categories

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

PowerPC
macOS

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: stf, Assigned: stuart.morgan+bugzilla)

References

Details

(Keywords: fixed1.8.1.2)

Attachments

(1 file, 2 obsolete files)

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.
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.
Severity: enhancement → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: AAHIG: Page Sheet allows other operations → Disable commands when OpenLocation sheet is up
hahah cute.
Assignee: saari → pinkerton
Target Milestone: --- → Camino1.0
This is still present and should definitely be fixed by 1.0. :)
Target Milestone: Camino1.0 → Camino1.1
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.
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.
Severity: trivial → minor
Priority: -- → P3
Summary: Disable commands when OpenLocation sheet is up → Disable menus commands when sheets or dialogs are frontmost
*** Bug 187849 has been marked as a duplicate of this bug. ***
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.
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?
(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.
*** Bug 329331 has been marked as a duplicate of this bug. ***
Component: General → Toolbars & Menus
QA Contact: winnie → toolbars
*** Bug 333438 has been marked as a duplicate of this bug. ***
Blocks: 341853
Taking.
Assignee: mikepinkerton → bugzilla
Whiteboard: [good first bug]
*** Bug 347630 has been marked as a duplicate of this bug. ***
Taking; I'll fix this after bug 159337 lands.
Assignee: bugzilla → stuart.morgan
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #245736 - Flags: review?
Attached patch full fix (obsolete) — Splinter Review
Missed a file.
Attachment #245736 - Attachment is obsolete: true
Attachment #245743 - Flags: review?
Attachment #245736 - Flags: review?
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.
Attachment #245743 - Flags: review? → review-
(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.
(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
So is there still a reason this is minused?
Comment on attachment 245743 [details] [diff] [review]
full fix

No. :)
Attachment #245743 - Flags: review- → review+
Attachment #245743 - Flags: superreview?(mikepinkerton)
(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
No longer blocks: 294129
Comment on attachment 245743 [details] [diff] [review]
full fix

sr=pink
Attachment #245743 - Flags: superreview?(mikepinkerton) → superreview+
Attached patch unbitrottedSplinter Review
As checked in on trunk and MOZILLA_1_8_BRANCH.
Attachment #245743 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: fixed1.8.1.2
Resolution: --- → FIXED
Whiteboard: [good first bug]
You need to log in before you can comment on or make changes to this bug.