Pulling this out of the Lion full screen bug... we should update the menu item & keyboard shortcut to align with what is "standard". We use cmd+shift+f, Safari/Chrome use cmd+ctrl+f We say have a checkbox menu item that just says "Full Screen". Safari/Chrome: "Enter Full Screen" & then "Exit Full Screen" as applicable.

Yup, we should do whatever the other Lion apps do, so the "enter" and "exit" menu strings + update the shortcut to match. (if we can keep the shift-cmd-F shortcut alive, that will minimize broken muscle memory too — but the official shortcut should be the one Safari and Chrome uses on Lion)

Dao, what do you think of this? I'm not the hugest fan... it seems awkward. There are other ways to do it so I wanted to get feedback before I started down that path Perhaps the better thing would be to just change the menuitem entirely on OSX? We wouldn't need to make it a checkbox there. We could still change the label dynamically when switching... Or we could just have 2 menuitems & show/hide enable/disable as appropriate. Then the strings all stay in browser.dtd...

Comment on attachment 585894 [details] [diff] [review] Patch v0.1 (WIP) I think the best option would be to have two separate menu items and hide them as needed. Also "Enter Full Screen" doesn't look like proper English to me. "full screen" is either an adjective or it's "the entire screen", none of which I think you can enter (as opposed to the full-screen mode).

(In reply to Dão Gottwald [:dao] from comment #3) > Comment on attachment 585894 [details] [diff] [review] > Patch v0.1 (WIP) > > I think the best option would be to have two separate menu items and hide > them as needed. Should we just change this across all platforms or keep it specific to OS X? > Also "Enter Full Screen" doesn't look like proper English to me. "full > screen" is either an adjective or it's "the entire screen", none of which I > think you can enter (as opposed to the full-screen mode). Yea.. I think it's implying "Full Screen mode". Blame Apple? We don't have to use the same string, but that's what Safari & Chrome both use. Other Apple apps (Terminal, Quicktime, Mail, Photo Booth, DVD Player) use the same "Enter/Exit Full Screen" language.

I'm not 100% satisfied here because I couldn't make use of the command="View:FullScreen". FullScreen.toggle sets .checked on the command, which would make enter/exitFullScreenCmd menuitems checked (even without type=checkbox :/ ). goDoCommand('View:FullScreen') also doesn't work because the <command> isn't part of any controller. So I went with what I know works - setting oncommand to BrowserFullScreen()... I'm not well versed in all the XUL trickery though so if I missed a way to make this work, I'm all ears.

<menuitem ...> <observes element="View:FullScreen" attribute="oncommand"/> <observes element="View:FullScreen" attribute="disabled"/> </menuitem> ?

> <menuitem ...> > <observes element="View:FullScreen" attribute="oncommand"/> > <observes element="View:FullScreen" attribute="disabled"/> > </menuitem> Ah thanks, Dão! I had seen something else using <observes> and thought there might be a way to get that. I wasn't going to observe disabled for exitFullScreenCmd but if for some reason an extension disables View:FullScreen when we're in full screen mode, the menutiem should reflect that (on Lion you'd still be able to exit full screen mode from the OS control).

Comment on attachment 586185 [details] [diff] [review] Patch v0.3 >+ <menuitem id="enterFullScreenItem" >+ accesskey="&fullScreenCmd.accesskey;" >+ label="&enterFullScreenCmd.label;" >+ key="key_fullScreen"> >+ <observes element="View:FullScreen" attribute="oncommand"/> >+ <observes element="View:FullScreen" attribute="disabled"/> >+ </menuitem> >+ oncommand="BrowserFullScreen();"/> broken syntax >+ <key id="key_fullScreen_old" key="&fullScreenCmd.macCommandKey;" command="View:FullScreen" modifiers="accel,shift"/> Do we really need this? I'd prefer a clean cut... >+ // Make sure the menu items are adjusted if necessary. >+ this._toggleMenuItems(enterFS); >+ _toggleMenuItems: function(enterFS) { >+#ifdef XP_MACOSX >+ document.getElementById("enterFullScreenItem").hidden = enterFS;; >+ document.getElementById("exitFullScreenItem").hidden = !enterFS; >+#endif > } You don't seem to really need this method. Can you move these two lines up? >+<!-- LOCALIZATION NOTE (enterFullScreenCmd.label, exitFullScreenCmd.label): >+These should match what Safari and other Apple applications use on OS X Lion. --> >+<!ENTITY enterFullScreenCmd.label "Enter Full Screen"> >+<!ENTITY exitFullScreenCmd.label "Exit Full Screen"> I still don't like this... Note that your l10n note conflicts with the goal of labeling things consistently within Firefox; the current proposal is to use "fullscreen" (bug 705234).

(In reply to Dão Gottwald [:dao] from comment #8) > I still don't like this... Note that your l10n note conflicts with the goal > of labeling things consistently within Firefox; the current proposal is to > use "fullscreen" (bug 705234). We should be labeling things consistently only if we're actually describing the same thing. I know very little about what "Lion full screen" actually is or how this feature works, but it sounds like a separate Mac-only feature. Assuming that's true, there's nothing wrong with labeling it separately, and being consistent with the OS's description of the feature.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10) > (In reply to Dão Gottwald [:dao] from comment #8) > > I still don't like this... Note that your l10n note conflicts with the goal > > of labeling things consistently within Firefox; the current proposal is to > > use "fullscreen" (bug 705234). > > We should be labeling things consistently only if we're actually describing > the same thing. I know very little about what "Lion full screen" actually is > or how this feature works, but it sounds like a separate Mac-only feature. Take a look: https://bugzilla.mozilla.org/attachment.cgi?id=586227 So the truth is that I'm not treating it as a Mac only feature. The "make fullscreen to new desktop" thing is Mac only and more specifically, Lion+ only. But on non-lion & non-mac the feature is the same old resize to fill the screen. > Assuming that's true, there's nothing wrong with labeling it separately, and > being consistent with the OS's description of the feature. Do you still feel the same way?

(In reply to Paul O'Shannessy [:zpao] from comment #11) > So the truth is that I'm not treating it as a Mac only feature. The "make > fullscreen to new desktop" thing is Mac only and more specifically, Lion+ > only. But on non-lion & non-mac the feature is the same old resize to fill > the screen. Given this (it's a cross-platform feature with a Lion-only quirk), I think it makes sense to have it use a consistent string across the app. This seems unlikely to be worth a Lion-specific string, but I might be convinced otherwise.

https://developer.apple.com/library/mac/#documentation/userexperience/conceptual/applehiguidelines/Menus/Menus.html#//apple_ref/doc/uid/TP30000356-TPXREF148 "Note: When you use Mac OS X v10.7 programming interfaces to allows users to take windows full screen, you add the Enter Full Screen menu item to the View menu. If you do not include a View menu, you can add the Enter Full Screen menu item to the Window menu instead."

(In reply to Mano from comment #13) > https://developer.apple.com/library/mac/#documentation/userexperience/ > conceptual/applehiguidelines/Menus/Menus.html#//apple_ref/doc/uid/TP30000356- > TPXREF148 > > "Note: When you use Mac OS X v10.7 programming interfaces to allows users to > take windows full screen, you add the Enter Full Screen menu item to the > View menu. If you do not include a View menu, you can add the Enter Full > Screen menu item to the Window menu instead." I'm not against breaking from the HIG (and we definitely do elsewhere), so I wouldn't get hung up on that. If we're not going to change the string, I still think we should change the keyboard shortcut.

(In reply to Paul O'Shannessy [:zpao] from comment #14) > I'm not against breaking from the HIG (and we definitely do elsewhere), so I > wouldn't get hung up on that. FWIW, Ignoring the Apple HIG based on the fact that it has been done before or for personal reasons based on that it does not sound correct should not justify this change. "Enter/Leave Full Screen" is what Lion users know and naming it differently just causes confusion. Minor issues like this is what causes Firefox to feel like a cross-OS solution based on lowest common shared features instead of a "native" OS X solution. Also, please note comment 1.

Yeah, we should be consistent with the naming elsewhere on the OS when there is a strong precedent. So, again, "Enter/Exit Full Screen" is what we want on the Mac. (I'll try to stay away from pointing out all the ways in which Apple breaks with the HIG themselves, although tempting ;) What Paul's patch is doing is what we want to do.

Now that Limi has laid down the law, here's an updated patch.

Comment on attachment 611643 [details] [diff] [review] Patch v0.4 - remove the extra semi-colon - add separate accesskeys for the lion strings

Can this reviewed patch make it for Firefox 14 (now Aurora)?

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee66329e548a (In reply to José Jeria from comment #19) > Can this reviewed patch make it for Firefox 14 (now Aurora)? Maybe. We can request landing on Aurora after this makes it to Nightly.

Hi, just wondering: are enterFullScreenCmd.accesskey and exitFullScreenCmd.accesskey really used? AFAIK, Mac menu items don't have accesskeys at all. Or are these treated as commandkeys (in which case they should be renamed)?