Last Comment Bug 714172 - Change fullscreen menu for Lion
: Change fullscreen menu for Lion
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: Firefox 15
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
Mentors:
Depends on: 639705
Blocks: lion-compatibility
  Show dependency treegraph
 
Reported: 2011-12-29 13:52 PST by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-05-20 12:26 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (WIP) (7.00 KB, patch)
2012-01-04 14:29 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dao+bmo: feedback-
Details | Diff | Review
Patch v0.2 (7.76 KB, patch)
2012-01-05 11:33 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Review
Patch v0.3 (7.07 KB, patch)
2012-01-05 12:51 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dao+bmo: review-
Details | Diff | Review
Patch v0.4 (6.20 KB, patch)
2012-04-02 16:21 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
gavin.sharp: review+
Details | Diff | Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-12-29 13:52:12 PST
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.
Comment 1 Alex Limi (:limi) — Firefox UX Team 2011-12-29 21:52:15 PST
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)
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-04 14:29:15 PST
Created attachment 585894 [details] [diff] [review]
Patch v0.1 (WIP)

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 3 Dão Gottwald [:dao] 2012-01-04 14:52:33 PST
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).
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-04 15:18:06 PST
(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.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-05 11:33:03 PST
Created attachment 586156 [details] [diff] [review]
Patch v0.2

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.
Comment 6 Dão Gottwald [:dao] 2012-01-05 11:37:03 PST
<menuitem ...>
  <observes element="View:FullScreen" attribute="oncommand"/>
  <observes element="View:FullScreen" attribute="disabled"/>
</menuitem>

?
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-05 12:51:13 PST
Created attachment 586185 [details] [diff] [review]
Patch v0.3

> <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 8 Dão Gottwald [:dao] 2012-01-09 04:28:48 PST
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).
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-09 17:00:30 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=714172
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-06 13:14:44 PST
(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.
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-17 16:38:26 PST
(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?
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-18 14:21:11 PST
(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.
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-13 05:35:49 PDT
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."
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-21 12:18:02 PDT
(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.
Comment 15 José Jeria 2012-03-23 01:28:54 PDT
(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.
Comment 16 Alex Limi (:limi) — Firefox UX Team 2012-03-29 15:36:27 PDT
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.
Comment 17 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-02 16:21:07 PDT
Created attachment 611643 [details] [diff] [review]
Patch v0.4

Now that Limi has laid down the law, here's an updated patch.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-23 11:23:10 PDT
Comment on attachment 611643 [details] [diff] [review]
Patch v0.4

- remove the extra semi-colon
- add separate accesskeys for the lion strings
Comment 19 José Jeria 2012-04-28 02:18:55 PDT
Can this reviewed patch make it for Firefox 14 (now Aurora)?
Comment 20 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-14 17:07:42 PDT
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.
Comment 21 Ed Morley [:emorley] 2012-05-15 06:32:26 PDT
https://hg.mozilla.org/mozilla-central/rev/ee66329e548a
Comment 22 Rimas Kudelis 2012-05-20 12:09:24 PDT
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)?

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