702159 - When in DOM full screen mode, there should be a context menu entry to exit full screen

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

When in DOM full screen mode, there should be a context menu entry to exit full screen.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The changes should be made in /browser/base/content/nsContextMenu.js and the unit test at /browser/base/content/test/test_contextmenu.html will need to be updated as well.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The JS can check for |document.mozFullScreenElement != null| to know if the browser has entered Full Screen.

Comment 3

[Connor Montgomery]

Where do you think the most appropriate place for this context menu is? I think it makes the most sense in the navigationItems (appx line 199 of nsContextMenu.js), something like "Exit full screen mode". Or, would it make sense to give it it's own section in the context menu?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Connor Montgomery from comment #3)
> Where do you think the most appropriate place for this context menu is? I
> think it makes the most sense in the navigationItems (appx line 199 of
> nsContextMenu.js), something like "Exit full screen mode". Or, would it make
> sense to give it it's own section in the context menu?

It probably deserves its own section. Switching between full screen and non-full screen doesn't seem like a navigation event to me.

Comment 5

[Connor Montgomery]

That makes sense to me!

So, I added the following function to nsContextMenu.js's nsContextMenu prototype (http://pastebin.mozilla.org/1386268).  I've also added a test (http://pastebin.mozilla.org/1386267).

I think the next step for me is to go in to the XUL and define the context-closeFullScreen item, and then also add that string in the dtd file. I've built the nightly (I'm on OS X.7), but when I grep or ack for the strings I see in a context menu (i.e. "View Background Image"), it shows a bunch of dtd files - not sure if I have to edit one or all? As far as the XUL goes, for this specific patch, I will be adjusting toolkit/components/help/content/helpContextOverlay.xul, correct? Also, I saw there's one for the En-US locale - when do the translators come into the process? Would I have to make a request that someone who knows other languages see this patch, or how would I go about fixing that problem as well? 

Lots of questions. Sorry to put them all in one post, but I figured people would rather get one email than three! Thank you for your help!

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Thank you for your work on this bug Connor :)

(In reply to Connor Montgomery from comment #5)
> That makes sense to me!
> 
> So, I added the following function to nsContextMenu.js's nsContextMenu
> prototype (http://pastebin.mozilla.org/1386268).  I've also added a test
> (http://pastebin.mozilla.org/1386267).

Can you please add these changes as attachments to this patch. The easiest way to export your changes is to do:
hg diff > name_of_file.diff

You can then upload that diff file to this bug as an attachment, then we can track changes as the work progresses.
 
> I think the next step for me is to go in to the XUL and define the
> context-closeFullScreen item, and then also add that string in the dtd file.
> I've built the nightly (I'm on OS X.7), but when I grep or ack for the
> strings I see in a context menu (i.e. "View Background Image"), it shows a
> bunch of dtd files - not sure if I have to edit one or all? As far as the
> XUL goes, for this specific patch, I will be adjusting
> toolkit/components/help/content/helpContextOverlay.xul, correct? Also, I saw
> there's one for the En-US locale - when do the translators come into the
> process? Would I have to make a request that someone who knows other
> languages see this patch, or how would I go about fixing that problem as
> well? 
 
String updates are done in the DTD files, and we generally only update the en-US locale. Our localizers will see the new strings and update them during the Aurora phase of our development lifecycle. With that being said, we probably won't need to add new strings for this patch.

I think we can reuse the string that is defined for browser full screen mode. See this search on MXR:
https://mxr.mozilla.org/mozilla-central/search?string=fullscreenexit&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

As you can see in the search that I have linked to, the context menu is added in /browser/base/content/browser.xul

> Lots of questions. Sorry to put them all in one post, but I figured people
> would rather get one email than three! Thank you for your help!

Please ask as many questions as you need to get started. IRC is good too, and you can often find help quicker by going to #introduction and #developers on irc.mozilla.org (my nick on IRC is jaws).

Comment 7

[Connor Montgomery]

Attachment: progress towards fixing the bug - add function to nsContextMenu's prototype.

Comment 8

[Connor Montgomery]

Attachment: testing the contextMenu function added before.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 575793 [details] [diff] [review]
progress towards fixing the bug - add function to nsContextMenu's prototype.

Review of attachment 575793 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +108,5 @@
>      this.initSaveItems();
>      this.initClipboardItems();
>      this.initMediaPlayerItems();
>    },
> +Fu

typo?

@@ +195,5 @@
>      this.showItem("context-openlinkincurrent", onPlainTextLink);
>      this.showItem("context-sep-open", shouldShow);
>    },
>  
> +  initNavigationItems: function CM_initNavigationItems() {br

typo?

@@ +210,5 @@
>      //this.setItemAttrFromNode( "context-stop", "disabled", "canStop" );
>    },
>  
> +  initCloseFullScreenItem: function CM_initCloseFullScreenItem(){
> +    var shouldShow = document.mozFullScreenElement != null;

Maybe this should follow the same convention as other parts of this file and add a property to |this| such as |this.isFullScreenMode| (I'm not sure if I like the *Mode name, but that can be decided later).

Comment 10

[Connor Montgomery]

yup - nice catch with the typos :) 

i added the property to |this| after adding the patch, so was thinking the same thing! digging into it last night helped a lot - it's much clearer now how all of the XUL and DTD stuff ties together.

i'm busy driving about 16 hours in the next two days, but will absolutely finish this up once i get home.

Comment 11

[Connor Montgomery]

Attachment: updated progress of context menu

for some reason or another, when building after this code, nothing is showing up in the context menu when the browser goes into html5 full-screen mode. not sure why - it must not be getting initialized, because (as you can see), i've tried manually setting the boolean shouldShow to true, and it's still not even showing the static text "firefox is awesome" there.

Any ideas?

Comment 12

[Connor Montgomery]

Attachment: updated the test so it is a case appended to the previous cases, not injected in the middle

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Connor Montgomery from comment #11)
> Created attachment 576687 [details]
> updated progress of context menu
> 
> for some reason or another, when building after this code, nothing is
> showing up in the context menu when the browser goes into html5 full-screen
> mode. not sure why - it must not be getting initialized, because (as you can
> see), i've tried manually setting the boolean shouldShow to true, and it's
> still not even showing the static text "firefox is awesome" there.
> 
> Any ideas?

Sorry for the confusion, I forgot to mention that the <menuitem> will have to be added to browser/base/content/browser-context.inc

Also, for a quicker build you should be able to do |make -s -C browser/base && make -s -C browser/app|

Next time when you add an attachment, please flag me ([e-mail]) for "feedback?" and I'll get an automated email so I can get to this quicker.

Comment 14

[Connor Montgomery]

Attachment: patch to implement the feature

Manually tested this against this site (http://pearce.org.nz/full-screen/). Click the button to toggle into full-screen mode, and the context menu will then have an option to "Exit Full Screen Mode", which works correctly.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 577041 [details] [diff] [review]
patch to implement the feature

>+      <menuitem id="context-dom-fullscreen"
>+                label="&domFullScreen.label;"
>+                onclick="gContextMenu.closeDOMFullScreen();"/>
>+      <menuseparator id="context-sep-dom-fullscreen"/>

Shouldn't the ids and the entity name indicated that they're about _leaving_ FS mode?

>     this.initOpenItems();
>     this.initNavigationItems();
>     this.initViewItems();
>     this.initMiscItems();
>     this.initSpellingItems();
>     this.initSaveItems();
>     this.initClipboardItems();
>     this.initMediaPlayerItems();
>+    this.initCloseFullScreenItems();
>   },
>-
>   initPageMenuSeparator: function CM_initPageMenuSeparator() {
>     this.showItem("page-menu-separator", this.hasPageMenu);
>   },

Please don't remove that blank line.

>+  initCloseFullScreenItems: function CM_initCloseFullScreenItem(){
>+    // only show the option if the user is in DOM full screen
>+    var shouldShow = (this.target.ownerDocument.mozFullScreenElement != null);
>+    this.showItem("context-sep-dom-fullscreen", shouldShow);
>+    this.showItem("context-dom-fullscreen", shouldShow);
>+    this.showItem("context-sep-dom-fullscreen", shouldShow);
>+  },

this.showItem("context-sep-dom-fullscreen", shouldShow); is duplicated.

> <!ENTITY domFullScreenWarning.label "Press ESC to leave full-screen mode">
>+<!ENTITY domFullScreen.label "Exit Full Screen Mode">

This should probably be "Leave Full-Screen Mode" for consistency with domFullScreenWarning.label.

Comment 16

[Connor Montgomery]

Attachment: Fixes to previous patch, as per dao's comments.

* adjusted the ID's and labels so they are "leaving" full-screen mode
* brought back the line break I had previously removed
* removed the duplicated separator
* adjusted vocabulary to be "leave", so it is consistent.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 577074 [details] [diff] [review]
Fixes to previous patch, as per dao's comments.

This is basically fine, but I'd like to see a couple of things renamed:

>+      <menuitem id="context-dom-leave-fullscreen"

context-leave-dom-fullscreen

>+                label="&domLeaveFullScreen.label;"

leaveDomFullScreen.label

>+                onclick="gContextMenu.closeDOMFullScreen();"/>

gContextMenu.leaveDOMFullScreen

>+      <menuseparator id="context-sep-dom-leave-fullscreen"/>

context-sep-leave-dom-fullscreen

>+    this.initCloseFullScreenItems();

this.initLeaveDOMFullScreenItems

Comment 18

[Connor Montgomery]

Attachment: Adjusting names of assets, labels, and functions, as per Dao's comments.

Thanks for all of your help! I have adjusted the names of everything you suggested. I'm not sure about mozilla's style on all-caps "DOM" or camel-cased "Dom", but I went with all-caps because it made more sense to me.

Thank you again!

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 577094 [details] [diff] [review]
Adjusting names of assets, labels, and functions, as per Dao's comments.

>+  initLeaveDOMFullScreenItems: function CM_initCloseFullScreenItem(){

nit: update the function name as well (CM_...) and insert a space before {

> <!ENTITY domFullScreenWarning.label "Press ESC to leave full-screen mode">
>+<!ENTITY leaveDOMFullScreen.label "Leave Full Screen Mode">

nit: put a hyphen between Full and Screen

Comment 20

[Connor Montgomery]

Attachment: Fix the nits from the previous review

I wasn't sure if I should add this as just an attachment, or make the older one obsolete. So, I decided to go ahead and go through this one more time - thank you very much!

I'm assuming, after this review gets +'d, it just bubbles up to superreview, and the people who are supposed to be notified automatically are? I'm not familiar with this process, and couldn't find a link, so if there is one, could you please put it in here? Thanks!

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 577208 [details] [diff] [review]
Fix the nits from the previous review

No superreview needed here.

Thanks!

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 577208 [details] [diff] [review]
Fix the nits from the previous review

>diff --git a/browser/base/content/browser-context.inc b/browser/base/content/browser-context.inc
>--- a/browser/base/content/browser-context.inc
>+++ b/browser/base/content/browser-context.inc
>@@ -113,16 +113,20 @@
>                 accesskey="&videoHideStats.accesskey;"
>                 label="&videoHideStats.label;"
>                 oncommand="gContextMenu.mediaCommand('hidestats');"/>
>       <menuitem id="context-video-fullscreen"
>                 accesskey="&videoFullScreen.accesskey;"
>                 label="&videoFullScreen.label;"
>                 oncommand="gContextMenu.fullScreenVideo();"/>
>       <menuseparator id="context-media-sep-commands"/>
>+      <menuitem id="context-leave-dom-fullscreen"
>+                label="&leaveDOMFullScreen.label;"
>+                onclick="gContextMenu.leaveDOMFullScreen();"/>

Wait, still need to set an accesskey here.

Comment 23

[Connor Montgomery]

Attachment: Add accesskey

added the accesskey to the commit, as well.

i believe after this gets r+'d, i need to use checkin-needed? excited for that!

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 577472 [details] [diff] [review]
Add accesskey

>+<!ENTITY leaveDOMFullScreen.accesskey "v">

"v" is already taken:
http://mxr.mozilla.org/mozilla-central/search?string=%22v%22&find=browser.dtd&findi=&filter=accesskey&hitlimit=&tree=mozilla-central

However, given the different context menu variants, it may be impossible to find a better key. This means that users may need to hit V twice in order to get to the desired item.

Comment 25

[Connor Montgomery]

What are the next steps I have to complete for this bug to be committed? Dão, correct me if I'm wrong, but do I see that you flagged it for ui review? 

I'm just unfamiliar with the process! Thank you!

Comment 26

[Dão Gottwald [:dao]]

The patch is waiting for limi to sign off on this from a UX perspective. Once the ui-review is granted, just add the checkin-needed keyword.

Comment 27

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 577472 [details] [diff] [review]
Add accesskey

It would be great to have a screenshot or a tryserver build for UI-review, but as far as I can tell from reading the diff, this just adds a context menu item once you have entered fullscreen that exits fullscreen mode — and that seems perfectly reasonable to me.

I'd check with Matej on the wording of this, Mac OS X uses "Enter Full Screen" and "Exit Full Screen", which makes sense to me — I'm not sure that the word "mode" adds anything meaningful here. Also, "enter/exit" vs "leave", and I believe we might call it "fullscreen" officially now instead of fullscreen.

Overall, this looks good to me.

Comment 28

[Alex Limi (:limi) — Firefox UX Team]

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #27)
> I believe we might call it "fullscreen" officially now instead
> of fullscreen.

…should have been:

I believe we might call it "fullscreen" now instead of "full screen".

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #28)
> (In reply to Alex Limi (:limi) — Firefox UX Team from comment #27)
> > I believe we might call it "fullscreen" officially now instead
> > of fullscreen.
> 
> …should have been:
> 
> I believe we might call it "fullscreen" now instead of "full screen".

Yes, according to bug 705234 we are going with "fullscreen".

Comment 30

[Connor Montgomery]

Great. I'll change it to say "Exit Fullscreen".

Comment 31

[Connor Montgomery]

Attachment: Context menu now says "Exit Fullscreen"

Fixed the text to say "Exit Fullscreen", as per the discussion on bugzilla.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 588308 [details] [diff] [review]
Context menu now says "Exit Fullscreen"

Review of attachment 588308 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix this and then reupload.

::: browser/base/content/browser-context.inc
@@ +125,2 @@
>        <menuseparator id="context-media-sep-commands"/>
> +      <menuitem id="context-leave-dom-fullscreen"

Something's not right here. I think this ID got changed on accident, and this menuitem looks to be a duplicate of what is right below it.

Comment 33

[Connor Montgomery]

Attachment: Correcting the diff

Thanks, jwein! Not sure how I missed it, but thank you.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Connor Montgomery from comment #33)
> Created attachment 588317 [details]
> Correcting the diff
> 
> Thanks, jwein! Not sure how I missed it, but thank you.

No problem, but I don't think this is the right patch either. It looks the same as the one before it.

Comment 35

[Connor Montgomery]

Attachment: Fix to previous incorrect diff

You're right - I had accidentally hg diff'd to the old file and just renamed it. This is better.

Thanks!

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 588319 [details] [diff] [review]
Fix to previous incorrect diff

Review of attachment 588319 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-context.inc
@@ -117,4 @@
>                  accesskey="&videoHideStats.accesskey;"
>                  label="&videoHideStats.label;"
>                  oncommand="gContextMenu.mediaCommand('hidestats');"/>
> -      <menuitem id="context-video-fullscreen"

Are you sure we want to remove the context menu option to enter fullscreen? This change wasn't in the patch that got r+'d from Dao.

@@ +120,5 @@
> +      <menuitem id="context-leave-dom-fullscreen"
> +                accesskey="&leaveDOMFullScreen.accesskey;"
> +                label="&leaveDOMFullScreen.label;"
> +                onclick="gContextMenu.leaveDOMFullScreen();"/>
> +      <menuseparator id="context-sep-leave-dom-fullscreen"/>

I think we should just keep the menuseparator id the same as it was before, 'context-media-sep-commands'.

Comment 37

[Connor Montgomery]

I am very sorry for mixing that up. I absolutely want to keep the context menu in there. Making the adjustment now. I really appreciate the help with this - diff coming in a few.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Connor Montgomery from comment #37)
> I am very sorry for mixing that up. I absolutely want to keep the context
> menu in there. Making the adjustment now. I really appreciate the help with
> this - diff coming in a few.

Oh no worries. If I had to give a nickel for each time I made a mistake like that I would have to file for bankruptcy :)

Comment 39

[Connor Montgomery]

Attachment: Fixed diff, as per jaws' comments (again)

This keeps the context menu to enter fullScreen, and also makes the menuseparator work like it's supposed to.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 588334 [details]
Fixed diff, as per jaws' comments (again)

>       <menuitem id="context-video-fullscreen"
>-                accesskey="&videoFullScreen.accesskey;"
>-                label="&videoFullScreen.label;"
>-                oncommand="gContextMenu.fullScreenVideo();"/>
>+                 accesskey="&videoFullScreen.accesskey;"
>+                 label="&videoFullScreen.label;"
>+                 oncommand="gContextMenu.fullScreenVideo();"/>

You're messing up the indentation here.

>+<!ENTITY leaveDOMFullScreen.label "Exit Fullscreen">
>+<!ENTITY leaveDOMFullScreen.accesskey "v">

* "v" doesn't make sense, since you changed Leave to Exit.

* You shouldn't write "Fullscreen" as one word in the label, as this is inconsistent with our other strings. Bug 705234 deals with changing this, doing it for a single label only doesn't help.

* Honestly, I don't understand how "Exit Fullscreen" (without "Mode") makes sense grammatically. Can somebody please explain this to me?

Comment 41

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #40)
> * Honestly, I don't understand how "Exit Fullscreen" (without "Mode") makes
> sense grammatically. Can somebody please explain this to me?

I'm not sure what there is to explain - it sounds fine to me. "fullscreen" is a made-up computer term to begin with, and it generally describes the mode, so there's no need explicitly mention "mode".

Comment 42

[Dão Gottwald [:dao]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #41)
> (In reply to Dão Gottwald [:dao] from comment #40)
> > * Honestly, I don't understand how "Exit Fullscreen" (without "Mode") makes
> > sense grammatically. Can somebody please explain this to me?
> 
> I'm not sure what there is to explain - it sounds fine to me. "fullscreen"
> is a made-up computer term to begin with, and it generally describes the
> mode, so there's no need explicitly mention "mode".

I'm looking for something like <http://en.wiktionary.org/wiki/fullscreen>.

It generally describes a mode, okay. That doesn't automatically make it a noun. "Go fullscreen" (adjective) also sounds more sensible than "enter fullscreen" (noun) to me...

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Seems like we should be discussing this in bug 705234, but...

"go fullscreen", "exit fullscreen", and "enter fullscreen" all seem equally sensible to me. Even more importantly, none of them are ambiguous. Our goals for the UI are to be clear and consistent, and I think any of those strings can achieve that, regardless of whether you understand their grammatical structure.

Comment 44

[Dão Gottwald [:dao]]

Me understanding the grammatical structure isn't an end in itself. I just want to make sure what we put in front of millions of users makes sense grammatically (and is clear and consistent at the same time). Of course my feeling about a foreign language can be wrong, but what I find online seems to support it and nobody else seems to be able to explain the structure...

Comment 45

[:Gavin Sharp [email: [e-mail]]]

"makes sense grammatically" also isn't an end in itself. In this particular case, I don't think it correlates well with any of the metrics we care about.

Comment 46

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Connor Montgomery from comment #39)
> Created attachment 588334 [details]
> Fixed diff, as per jaws' comments (again)

Connor: Is there anything that I can do to help you out here?

Comment 47

[Connor Montgomery]

Jared, thanks but I'm all set. Driving back/moving into school so that's why I've been MIA these last few days. Should patch this up tomorrow. 

Thanks man!

Comment 48

[Dão Gottwald [:dao]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #45)
> "makes sense grammatically" also isn't an end in itself.

Of course it isn't. English grammar doesn't say that "full screen" must not act as a noun and that "exit" + adjective is forbidden because this is how things ought to be. It says that native speakers of the English language don't commonly talk like that -- or maybe it doesn't, which is why I asked.

Comment 49

[Dão Gottwald [:dao]]

So, to avoid a longer debate about the purpose of grammer, what I'm asking is: Will "exit fullscreen" feel natural to the masses? People are adaptive and tolerant towards jargon and quirky language, but that doesn't mean user interfaces shouldn't avoid it if they can.

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #49)
> Will "exit fullscreen" feel natural to the masses?

Yes, it will feel natural to the masses. It is a popular phrase with video online, and is the phrasing that YouTube uses to exit fullscreen (except they use "full screen", but that has already been decided).

Comment 51

[Dão Gottwald [:dao]]

AFAIK Flash says "Press Esc to exit full screen mode." Youtube's full screen button has a tooltip saying just "Full Screen" (no "Enter"), I think. (I get the German interface.)

Comment 52

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #51)
> Youtube's full screen  button has a tooltip saying
> just "Full Screen" (no "Enter"), I think. (I get
> the German interface.)

Yes, that is the tooltip for entering fullscreen. When already in fullscreen, the tooltip says "Exit full screen".

Comment 53

[Connor Montgomery]

Attachment: Add better accesskey

Changed the accesskey to be more appropriate with the text on there.

As per Dão's comment, it seemed like "Exit FullScreen" was OK. I'd love to get this tied up - after reviewing the other comments, it seemed like that and the accesskey were the only things left to change?

Please let me know!

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 590437 [details] [diff] [review]
Add better accesskey

Review of attachment 590437 [details] [diff] [review]:
-----------------------------------------------------------------

There are still a few things that need to be changed before we can land this.

It would also be appreciated if you could update browser/base/content/test/test_contextmenu.html with a testcase for the fullscreen video context menu so we can make sure that menuitems like this new one don't stop working sometime in the future. Let me know if you have any questions or need any help updating the test.

::: browser/base/content/browser-context.inc
@@ +119,5 @@
>                  oncommand="gContextMenu.mediaCommand('hidestats');"/>
>        <menuitem id="context-video-fullscreen"
> +                 accesskey="&videoFullScreen.accesskey;"
> +                 label="&videoFullScreen.label;"
> +                 oncommand="gContextMenu.fullScreenVideo();"/>

These three lines have an extra space added in front of them that should be removed.

@@ +124,5 @@
>        <menuseparator id="context-media-sep-commands"/>
> +      <menuitem id="context-leave-dom-fullscreen"
> +                accesskey="&leaveDOMFullScreen.accesskey;"
> +                label="&leaveDOMFullScreen.label;"
> +                onclick="gContextMenu.leaveDOMFullScreen();"/>

This should be an oncommand handler here, an onclick handler won't work for keyboard navigation.

@@ +125,5 @@
> +      <menuitem id="context-leave-dom-fullscreen"
> +                accesskey="&leaveDOMFullScreen.accesskey;"
> +                label="&leaveDOMFullScreen.label;"
> +                onclick="gContextMenu.leaveDOMFullScreen();"/>
> +      <menuseparator id="context-sep-leave-dom-fullscreen"/>

To be consistent with the menu before entering fullscreen, the separator should be below the fullscreen menuitem. However, we shouldn't add an extra menuseparator. Can you just move the <menuseparator id="context-media-sep-commands"/> to be below the "context-leave-dom-fullscreen" menuitem and remove this new menuseparator?

::: browser/base/content/nsContextMenu.js
@@ +219,5 @@
> +  initLeaveDOMFullScreenItems: function CM_initLeaveFullScreenItem() {
> +    // only show the option if the user is in DOM full screen
> +    var shouldShow = (this.target.ownerDocument.mozFullScreenElement != null);
> +    this.showItem("context-leave-dom-fullscreen", shouldShow);
> +    this.showItem("context-sep-leave-dom-fullscreen", shouldShow);

Please remove the line enabling the "context-sep-leave-dom-fullscreen" separator, since the extra separator isn't needed.

@@ +878,5 @@
> +  leaveDOMFullScreen: function() {
> +    document.mozCancelFullScreen();
> +  },
> +
> +

Can you remove this extra blank line? There should only be one line between these two functions.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +94,4 @@
>  <!ENTITY domFullScreenWarning.label "Press ESC to leave full-screen mode">
> +<!ENTITY leaveDOMFullScreen.label "Exit Fullscreen">
> +<!ENTITY leaveDOMFullScreen.accesskey "u">
> +

Please remove this extra line here. Only one blank line between these sections is necessary.

Comment 55

[Dão Gottwald [:dao]]

Connor, please don't set review+ yourself on a patch that's significantly different from the patch that has actually been granted review (attachment 577472 [details] [diff] [review]). You want to re-request review in such cases.

Comment 56

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #52)
> (In reply to Dão Gottwald [:dao] from comment #51)
> > Youtube's full screen  button has a tooltip saying
> > just "Full Screen" (no "Enter"), I think. (I get
> > the German interface.)
> 
> Yes, that is the tooltip for entering fullscreen. When already in
> fullscreen, the tooltip says "Exit full screen".

I had never read that tooltip, since I know the button from entering... Why do you believe it's a "popular phrase"?

(In reply to Connor Montgomery from comment #53)
> Created attachment 590437 [details] [diff] [review]
> Add better accesskey
> 
> Changed the accesskey to be more appropriate with the text on there.
> 
> As per Dão's comment, it seemed like "Exit FullScreen" was OK.

Apart from "exit fs" vs. "exit fs mode", I said, quote: You shouldn't write "Fullscreen" as one word in the label, as this is inconsistent with our other strings. Bug 705234 deals with changing this, doing it for a single label only doesn't help.

Comment 57

[Connor Montgomery]

Attachment: Fix issues, as per jwein's comments; begin testing

Comment 58

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 597695 [details] [diff] [review]
Fix issues, as per jwein's comments; begin testing

Connor: Do you still have our conversation on what to do to fix this test?

Comment 59

[Dan Wendorf [:dwendorf]]

Attachment: Fixed testcase in Connor's patch

This patch includes the changes from Connor's patch, and updates his testcase to correctly launch fullscreen mode and launch the context menu.

I'm using netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); in two callbacks to get permission to the contextMenu. If there is a more-appropriate way of doing this (as far as I know, enablePrivilege is no longer recommended for new tests), please let me know.

Comment 60

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dan Wendorf [:dwendorf] from comment #59)
> I'm using
> netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); in
> two callbacks to get permission to the contextMenu.

What exceptions do you get when you don't call enablePrivilege?

Comment 61

[Dan Wendorf [:dwendorf]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #60)
> What exceptions do you get when you don't call enablePrivilege?

I get "permission denied to access property 'state'" on line 30 of test_contextmenu.html

Comment 62

[Josh Matthews [:jdm]]

Instead of using enablePrivilege, you can change the throwing reference to |SpecialPowers.wrap(contextMenu).state|.

Comment 63

[Dan Wendorf [:dwendorf]]

Attachment: Replaced enablePrivilege use with SpecialPowers.wrap

Thanks for the help, jdm. SpecialPowers.wrap is working perfectly.

Comment 64

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 621505 [details] [diff] [review]
Replaced enablePrivilege use with SpecialPowers.wrap

Review of attachment 621505 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +219,5 @@
> +  initLeaveDOMFullScreenItems: function CM_initLeaveFullScreenItem() {
> +    // only show the option if the user is in DOM full screen
> +    var shouldShow = (this.target.ownerDocument.mozFullScreenElement != null);
> +    this.showItem("context-leave-dom-fullscreen", shouldShow);
> +    this.showItem("context-media-sep-commands", shouldShow);

The line here for context-media-sep-commands needs to be changed. It is already shown/hidden in initMediaPlayerItems if the context menu is being shown on a media element. We shouldn't be hiding it here in case it was already made visible.

> if (shouldShow)
>   this.showItem("context-media-sep-commands", shouldShow); // or pass true as 2nd argument, your choice.

::: browser/base/content/test/test_contextmenu.html
@@ +703,5 @@
> +                         ].concat(inspectItems));
> +        closeContextMenu();
> +        var full_screen_element = subwindow.document.getElementById("test-dom-full-screen");
> +        subwindow.addEventListener("mozfullscreenchange", function() {
> +            openContextMenuFor(pagemenu, true); // Invoke context menu for next test.

We should also remove this event listener when it is called like you've done for the entering fullscreen case.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +90,5 @@
>  <!ENTITY fullScreenAutohide.accesskey "H">
>  <!ENTITY fullScreenExit.label "Exit Full Screen Mode">
>  <!ENTITY fullScreenExit.accesskey "F">
> +
> +<!ENTITY domFullScreenWarning.label "Press ESC to leave full screen mode">

Let's leave this string unchanged and get it fixed in bug 705234.

Comment 65

[Dan Wendorf [:dwendorf]]

Attachment: Fixed previous patch per jaws' comments

Comment 66

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 622462 [details] [diff] [review]
Fixed previous patch per jaws' comments

Review of attachment 622462 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there :) Can you set a commit message on the patch as well as the User information? See http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed for more information.

Also, can you call SpecialPowers.clearUserPref("full-screen-api.allow-trusted-requests-only"); before the test finishes (around line 773 of test_contextmenu.html) so that the pref state is cleaned up?

Comment 67

[Dan Wendorf [:dwendorf]]

Attachment: Added username and commit message, and unregistered SpecialPowers usage.

Comment 68

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 627103 [details] [diff] [review]
Added username and commit message, and unregistered SpecialPowers usage.

Review of attachment 627103 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! I'll send this to tryserver and if all tests pass then I'll check it in for you :)

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01673165f0ee

Thanks for the patch, Dan! I had to un-bitrot browser.dtd a bit. Please look it over to make sure it's OK.

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01673165f0ee

Comment 71

[Dan Wendorf [:dwendorf]]

Thanks for un-bitrotting it. I know you've already checked it in, but I wanted to confirm that your change looks fine!