204636 - Make minimize (Cmd+M) and zoom menu commands work

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, defect, P2)

Description

[Mitchell Lewandowski]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030505 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030505 Mozilla Firebird/0.6

cmd-m should minimize the browser window to the dock, but the command seems
unmapped.

Reproducible: Always

Steps to Reproduce:
1. Open Phoenix.
2. Hold Cmd & m at the same time, the browser should minimize and doesn't.
3.

Actual Results:  
The browser window remains viewable.

Expected Results:  
Minimize the browser window to the dock.

Comment 1

[Greg K.]

Confirmed using Firebird/2003-05-03.

Comment 2

[Kevin Gerich]

Dupe of bug 137523 ? or does this depend on 137523?

Comment 3

[Jo Hermans]

Bug 137523 is for Mozilla, and Firebird has a different UI anyway.

I think it depends on bug 204420 (missing windows menu), where the shortcut has
to be visible. Unlees we inlcude it somewhere else.

Comment 4

[Blake Ross]

reassigning mac bugs, sorry for the spam.

Comment 5

[Mike Connor [:mconnor]]

--> hyatt

I think this was fixed by the checkin for the Window menu, but I'm not
completely sure.  hyatt, if its fixed, just mark it when you see this.

Comment 6

[David Hyatt]

This isn't fixed yet.  I just stubbed out the command without hooking it up yet.

Comment 7

[TGOS]

But in the Window menu there already is an entry "Minimize Window C+M", it's
just not active. I don't know if Mozilla Firebird uses Cocoa or Carbon windows,
but in Cocoa the menu item just has to call the "-
(void)performMiniaturize:(id)sender" method of the "NSWindow" object (which will
simulate a click on the minimize button on the window frame).

Comment 8

[Sam Katz]

Just checking. Is Cmd-M really the correct guideline? Because when I use OS X, I
expect Cmd-H to hide the window?

I apologize if minimizing and hiding are two seperate functions.

Comment 9

[Paul Baker]

They are two separate things. Minimize is when it shrinks down into the dock.

Comment 10

[Paul Jones]

Using recent nighly builds, I have noticed that Cmd+M opens mail.app and brings
a new blank message window to the front.

Comment 11

[Asaf Romano (gone)]

(In reply to comment #10)
> Using recent nighly builds, I have noticed that Cmd+M opens mail.app and brings
> a new blank message window to the front.

Same behavior for me

Comment 12

[Mike Connor [:mconnor]]

yeah, the mail integration hasn't been #ifdeffed for OS X yet, probably won't
get touched until after 0.9 though

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

+ing - bad. 

Comment 14

[Brian Mastenbrook]

While this isn't technically in the HIG, every other OS X program I use uses
Cmd-M for minimize, and it's VERY frustrating to have this do something
unexpected in Firefox. (It's disappointing how Firefox is smoother to use on
Windows than on OS X because of little things like this. What good is being a
model of usability, as MPT likes to tout, if you can't adhere to platform
conventions?)

Comment 15

[Joseph R. Kiniry]

Confirmed that splat-m brings up a new Mail.app compose window rather than
minimizing Firebird.  (Firebird 0.9.1 on OS X 10.3.4)  This is unexpected
behavior and should be high return for little effort.

Comment 16

[Steuard Jensen]

Attachment: Partial fix: change mail key on OS X to Cmd+Shift+M

This patch changes browser-sets.inc so that the "newMessage" key conflict is
fixed the same way as "gotoHistory" case: Cmd+M is replaced by Cmd+Shift+M for
OS X.  (I've also added an explanatory comment for the Cmd+H case, to match the
comment for Cmd+I.)  In other words, Cmd+M no longer opens a mail window on a
Mac.  However, it doesn't do anything else yet, either.

Comment 17

[Steuard Jensen]

Sadly, I don't seem to understand the code well enough to actually get Cmd+M to
minimize the window (or, for that matter, to make the item in the Window menu do
so).  I'd hoped that adding 'oncommand="window.minimize();"' to the
'minimizeWindow' command definition in browser-sets.inc would do it, since that
looks like it's the 'oncommand' setting for the 'minimize-button' item in
browser.xul.  However, naively attempting that replacement (along with removing
the 'disabled="true"' on 'windowMinimize') didn't seem to work.  (Cmd+M made the
Window menu flash as if it was active for a moment, but nothing else happened.)  

A full fix of this problem will clearly require someone who actually knows the
proper syntax and the proper function to call here.  I posted the partial patch
above for the sake of those who think that doing nothing is better than doing
the wrong thing.

Comment 18

[Asaf Romano (gone)]

i have a patch.

Comment 19

[Asaf Romano (gone)]

Attachment: Fix for nsMacWindow and browser frontend

So this patch enables the minimize and zoom menu commands (and changes Accel+M
for newMsg to Accel+Shift+M on mac).

I will add a patch for thunderbird frontend after this one is reviewed.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 162559 [details] [diff] [review]
Fix for nsMacWindow and browser frontend

This should be reviewed by both frontend and a mac backend peer.

Comment 21

[Asaf Romano (gone)]

Attachment: Fix for nsMacWindow and browser frontend - v2

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 162589 [details] [diff] [review]
Fix for nsMacWindow and browser frontend - v2

> 
>@@ -300,8 +301,14 @@
>     <key id="key_gotoHistory"  key="&historySidebarCmd.commandKey;"  command="viewHistorySidebar" modifiers="accel"/>
> #endif
> 
>-    <key id="key_newMessage" key="&sendMessage.commandkey;" command="Browser:NewMessage" modifiers="accel"/>
>-
>+    <key id="key_newMessage"
>+        key="&sendMessage.commandkey;"
>+        command="Browser:NewMessage"
>+#ifndef XP_MACOSX
>+        modifiers="accel"/>
>+#else
>+        modifiers="accel,shift"/>
>+#endif

fix the spacing here, key should line up with id, ignoring 2/4 spacing. 
Otherwise, Shouldn't the key attribute be there or is it convention on Mac to
not show that?

Comment 23

[Asaf Romano (gone)]

(In reply to comment #22)
> (From update of attachment 162589 [details] [diff] [review])
> > 
> >@@ -300,8 +301,14 @@
> >     <key id="key_gotoHistory"  key="&historySidebarCmd.commandKey;" 
command="viewHistorySidebar" modifiers="accel"/>
> > #endif
> > 
> >-    <key id="key_newMessage" key="&sendMessage.commandkey;"
command="Browser:NewMessage" modifiers="accel"/>
> >-
> >+    <key id="key_newMessage"
> >+        key="&sendMessage.commandkey;"
> >+        command="Browser:NewMessage"
> >+#ifndef XP_MACOSX
> >+        modifiers="accel"/>
> >+#else
> >+        modifiers="accel,shift"/>
> >+#endif
> 
> fix the spacing here, key should line up with id, ignoring 2/4 spacing.

will be fixed on checkin.

> Otherwise, Shouldn't the key attribute be there or is it convention on Mac to
> not show that?

It _is_ shown  as it is inherited from the command.

Comment 24

[jhp (no longer active)]

Comment on attachment 162589 [details] [diff] [review]
Fix for nsMacWindow and browser frontend - v2

For SetSizeMode(), I would rather do this (so as to not duplicate the Resize()
code):

if (NS_SUCCEEDED(rv)) {
  if (aMode == nsSizeMode_Minimized) {
    ::CollapseWindow(mWindowPtr, true);
  } else {
    if (aMode == nsSizeMode_Maximized) {
      CalculateAndSetZoomedSize();
      ::ZoomWindow(mWindowPtr, inZoomOut, ::FrontWindow() == mWindowPtr);
    } else {
      ::ZoomWindow(mWindowPtr, inZoomIn, ::FrontWindow() == mWindowPtr);
    }
    ::GetWindowPortBounds(mWindowPtr, &macRect);
    Resize(macRect.right - macRect.left, macRect.bottom - macRect.top,
PR_FALSE);
  }
}

+#ifndef XP_MACOSX
+	 modifiers="accel"/>
+#else
+	 modifiers="accel,shift"/>
+#endif

Is there a reason you do an "#ifndef XP_MACOSX" rather than reverse it and do
an "#ifdef XP_MACOSX"?	I usually prefer the latter.

Otherwise, looks good.

Comment 25

[Asaf Romano (gone)]

Attachment: comment 22-24

Comment 26

[Asaf Romano (gone)]

Comment on attachment 162598 [details] [diff] [review]
comment 22-24

moving review from javier, requesting approval.

Comment 27

[Asaf Romano (gone)]

(In reply to comment #26)
> (From update of attachment 162598 [details] [diff] [review])
> moving review from javier, requesting approval.

Thunderbird frontend patch on bug 239984.

Comment 28

[Asaf Romano (gone)]

Comment on attachment 162598 [details] [diff] [review]
comment 22-24

lets get "ok" from smfr.

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 162598 [details] [diff] [review]
comment 22-24

a=asa for aviary checkin.

Comment 30

[Mitchell Lewandowski]

confirming fixed on 10.3.5.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3) Gecko/20041023
Firefox/1.0

thanks to all for making this annoyance a thing of the past.

Comment 31

[sairuh (rarely reading bugmail)]

tested with 2004102406-0.9+ on os x 10.3.5: minimize/cmd+M works nicely. zoom is
a bit odd: I had to select the menu item twice before it actually maximized the
window (1st time kept the same window size, but moved it to the upper-left
corner of the screen; 2nd time maximized). Bring All to Front is still not
implemented --but what I've seen here is definitely an improvement!

Comment 32

[Asaf Romano (gone)]

(In reply to comment #31)
> tested with 2004102406-0.9+ on os x 10.3.5: minimize/cmd+M works nicely. zoom is
> a bit odd: I had to select the menu item twice before it actually maximized the
> window (1st time kept the same window size, but moved it to the upper-left
> corner of the screen; 2nd time maximized).

While the zoom impl' does have some problems, I can't repodruce the behavior you
mentioned.

> Bring All to Front is still not implemented

w-i-p :)

Comment 33

[Oakwine]

(In reply to comment #31)
zoom is
> a bit odd: I had to select the menu item twice before it actually maximized the
> window (1st time kept the same window size, but moved it to the upper-left
> corner of the screen; 2nd time maximized).

Verified Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3)
Gecko/20041025 Firefox/1.0. 

Comment 34

[Friðrik Már Jónsson]

This needs to be fixed, I hate getting my Mail application up every time I try
to minimize.  Additionally, the minimize command does not work from the "Window
> Minimize" menu

Comment 35

[Asaf Romano (gone)]

(In reply to comment #34)
> This needs to be fixed, I hate getting my Mail application up every time I try
> to minimize.  Additionally, the minimize command does not work from the "Window
> > Minimize" menu

Grep latest-aviary nightly build from:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-0.9/

next-time please read the bug before commenting, thanks.

Comment 36

[José Jeria]

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

Comment 37

[jhp (no longer active)]

I checked in the nsMacWindow.cpp changes into the trunk.  How will the Firefox
changes be handled?

Comment 38

[Asa Dotzler [:asa]]

moving blocking1.0mac bugs to Firefox1.1 Target Milestone.

Comment 39

[Mitchell Lewandowski]

this patch was already landed for 1.0 - why is there a need to re-assign for 1.1?

Comment 40

[Steuard Jensen]

(In reply to comment #39)
> this patch was already landed for 1.0 - why is there a need to re-assign for 1.1?

At a guess, it's because the current zoom implementation still "ha[s] some
problems" (according to its author in comment #32).  Comment #31 mentions one of
those, which I know that I've seen myself (though I'm not sure that I've seen it
recently, and I've never figured out how to reproduce it reliably).

Another problem with the current fix is that selecting "Zoom" from the menu
twice (let's call that "Zoom^2") is not always a "null operation".  Selecting
the Apple built-in "+" button twice not only brings the window back to its
original size but moves it back to the point on the screen where it came from. 
"Zoom^2" does _not_ put the window back in the right place: if you've used the
"+" button in the current session, "Zoom^2" moves the window to whatever
position it had when "+" was last used to maximize it.  If "+" has not been
pressed in the current session, "Zoom^2" leaves the window hanging in the upper
left corner of the screen.  (It looks like the Zoom command is not properly
storing the window's original location when used to maximize, but it is reading
some standard stored location that defaults to "upper left" when used to minimize.)

Don't get me wrong: I'm very, very happy that this issue was essentially fixed
for 1.0 (especially the Cmd+M issue).  But it would be good to keep this bug on
the radar until those last few kinks are worked out.  (On the other hand, the
remaining issue probably fits better in component "Menus" or even "OS
Integration" than in "Keyboard Navigation".)

Comment 41

[Asaf Romano (gone)]

No, Asa just did a wrong query yesterday (forgot to exclude "fixed-aviary" bugs
from his blocking-aviary1.0mac+ list), this is not the only one :)

In reply to the zoom issues: i'm well aware of (most?) them. I'm planning to fix
them for 1.1, hopefuly (but not in this bug).

Comment 42

[Prognathous]

As mentioned already, Minimize works very well in FF 1.0. Wouldn't it be better
to resolve this bug as fixed and deal with any remaining issues in separate bugs?

Prog.

Comment 43

[Asaf Romano (gone)]

(In reply to comment #42)
> As mentioned already, Minimize works very well in FF 1.0. Wouldn't it be better
> to resolve this bug as fixed and deal with any remaining issues in separate bugs?
> 
> Prog.

The only reason I'm not resolving this bug is brnach-landing (in other words,
the bug is not fixed on the trunk).

The zoom issue(s) is/are already coverd by the patch on bug 269480 (which waits
for sr from smfr).

Comment 44

[Asaf Romano (gone)]

Fixed on trunk.

bug 269480 has a patch, waiting for smfr's sr. If there are any remaing issues
after this << patch is checked in, please file separate bugs and cc me, thanks.

Comment 45

[sairuh (rarely reading bugmail)]

works nicely in ffox 1.0 and today's 2005011011-trunk ffox build on mac os x 10.3.7.