470628 - Provide a Full Screen button

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, enhancement)

Description

[Biju]

One of the item we left out in bug 448909 was the Full Screen button

Comment 2

[Biju]

Justin I thought bug 453063 is for back end not for controls

Comment 3

[Justin Dolske [:Dolske]]

When the back end exists we can look at how to expose various controls, let's not get ahead of ourselves.

Comment 4

[u88484]

Now that fullscreen support has landed, should we reopen this bug or file a new one?

Comment 5

[Biju]

Thanks for fixing bug 453063 and bug 513144.

(In reply to comment #2)
> Justin I thought bug 453063 is for back end not for controls
But at the end I was almost correct about bug 453063, so reopening this.

We need this as Youtube and every major site provide this intuitive button feature.

Comment 6

[balachandar m]

can i work on this?

Comment 7

[Justin Dolske [:Dolske]]

Feel free, but I suspect the hard part here is designing the API and mechanism to securely allow web page content to trigger full-screen mode. The WHATWG list had some discussion on this topic last month.

Comment 8

[balachandar m]

i some how tweaked the UI and added the fullscreen button. but i do not get any api for the full screen. probably i should make it fullscreen based on the screen size.

also, how can i assign the bug to myself?

Comment 9

[balachandar m]

Attachment: video full screen button

this is the first version of the fullscreen button on the video element

Comment 10

[(no longer active)]

Does this actually require a content privileged full screen API?  The video controls xbl actually has chrome privileges, right?

If we get this for Firefox 4, this will probably compete with TabCandy to get my favorite UI feature, ever!

Comment 11

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

(In reply to comment #10)
> The video controls xbl actually has chrome privileges, right?

It doesn't, AFAIK.

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to comment #10)
> > The video controls xbl actually has chrome privileges, right?
> 
> It doesn't, AFAIK.

Hmm, so how does this code work, for example? <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml?force=1#265>

Comment 13

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

There's nothing that stops unprivileged code from implementing XPCOM interfaces, AFAIK.

Comment 14

[(no longer active)]

Oh, I was under the impression that in order to access Components.xxx, you have to have chrome privileges.

Comment 15

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

Attachment: Patch for bug 470628

I think we'll need a better icon for the fullscreen button, but my MSPaint-level skillz might get us far enough.

Comment 16

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

Attachment: Patch for bug 470628 (with icon)

Forgot the sweet icon.

Comment 17

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

Also, you'll need to set the pref full-screen-api.enabled to true in about:config for the button to work.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 563792 [details] [diff] [review]
Patch for bug 470628 (with icon)

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

Better wait until the full-screen API is enabled before landing this.

Comment 19

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

Attachment: Patch for bug 470628 v2

Updated patch to make use of document.mozFullScreenEnabled API (which is yet to be implemented), also updated the unit tests to be aware of the new button.

There is a line that will need to be partially commented out in the unit tests until document.mozFullScreenEnabled has been implemented.

Comment 20

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

Attachment: Patch for bug 470628 v3

Updated patch to include a better mochitest for the full-screen button (but the test hangs when run in batch ATM).

Also fixed the checking for document.mozFullScreenEnabled.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

We can also use mozRequestFullScreen() for the context-menu full-screen when document.mozFullScreenEnabled is true. Then the context-menu full-screen mode will be the same as the controls full-screen mode.

I can't see anything to prevent the full-screen button showing when viewing an <audio> element?

Comment 22

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

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #21)
> We can also use mozRequestFullScreen() for the context-menu full-screen when
> document.mozFullScreenEnabled is true. Then the context-menu full-screen
> mode will be the same as the controls full-screen mode.

Good idea. Would we be able to remove the code for video-fullscreen.xhtml?

It seems that based on the security restrictions around mozRequestFullScreen, that we might want to keep around the old code path so users can explicitly choose full screen even if iframes don't have the mozallowfullscreen attribute.
 
> I can't see anything to prevent the full-screen button showing when viewing
> an <audio> element?

Good catch. I've fixed it now to check for audio as well.

Comment 23

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

Attachment: Patch for bug 470628 v4

Updated to fix the test failure when run in batch, as well as to check for audio files.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jared Wein [:jwein] from comment #22)
> (In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #21)

> It seems that based on the security restrictions around
> mozRequestFullScreen, that we might want to keep around the old code path so
> users can explicitly choose full screen even if iframes don't have the
> mozallowfullscreen attribute.

So it's safe enough to relax the constraints for the context-menu case, but not for the general video controls case. In the video controls case, the Bad Guys could trick a user to click on the controls' full-screen button, and then overlay something over the video once its gone full-screen. Going full-screen from the context-menu would be a much more conscious action on the part of the user.

This may be as simple as not doing the mozallowfullscreen check in mozFullScreenEnabled() if the caller is chrome, but I'm not sure if that would fire in both the controls and the context menu case.

So... In the meantime, if mozFullScreenEnabled() returns true in the context-menu function/listener/oncommand, use the DOM full-screen API, otherwise fallback to the existing code. We can remove the exiting code once we enable the full-screen API...

Comment 25

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

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #24)
> So... In the meantime, if mozFullScreenEnabled() returns true in the
> context-menu function/listener/oncommand, use the DOM full-screen API,
> otherwise fallback to the existing code. We can remove the exiting code once
> we enable the full-screen API...

Great! That's the approach I took in v4 of the patch :)

Comment 26

[Stephen Horlander [:shorlander]]

Comment on attachment 564670 [details] [diff] [review]
Patch for bug 470628 v4

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

Attaching a two-state fullscreen icon. ui-r with that.

Comment 27

[Stephen Horlander [:shorlander]]

Attachment: Fullscreen video icon

Comment 28

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

Attachment: Patch for bug 470628 v5

I've added the button that shorlander created. Carrying forward ui-r+ from shorlander.

Comment 29

[Justin Dolske [:Dolske]]

Comment on attachment 565097 [details] [diff] [review]
Patch for bug 470628 v5

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

Looks mostly good from a quickish skim, but a couple remarks.

Random question:

Suppose a page has <div><video controls/><div> and makes the <div> full-screen. What happens, and what should we do? [Also consider <div><deeply nested><video/></deep></div>]

::: browser/base/content/nsContextMenu.js
@@ +858,5 @@
>    },
>  
>    fullScreenVideo: function () {
> +    if (document.mozFullScreenEnabled) {
> +      this.target.mozRequestFullScreen();

Does this behave any differently when chrome calls it vs when the user does?

@@ +865,3 @@
>  
> +      openDialog("chrome://browser/content/fullscreen-video.xhtml",
> +                 "", "chrome,centerscreen,dialog=no", this.target);

I think we'll want to remove this code path sometime after this patch lands and sticks. Followup bug? Probably for the mozLoadFrom() stuff too, but that's up to the platform guys.

::: toolkit/content/widgets/videocontrols.xml
@@ +818,5 @@
>                      // controlling volume.
>                  },
>  
> +                isVideoInFullScreen : function () {
> +                    return document.mozFullScreen && document.mozFullScreenElement == this.video;

See, now I can' remember if .isSameNode() and == (===?) do the same thing in all case. Jog my memory?

@@ +823,5 @@
> +                },
> +
> +                toggleFullscreen : function () {
> +                    this.isVideoInFullScreen() ?
> +                        document.mozCancelFullScreen() :

What happens if the video does full-screen, and then the page makes some other element go full-screen afterwards? Or the user uses the context-menu to change state? Seems like you need a listener to watch for externally-driven state changes. (I presume such an event exists!)

::: toolkit/themes/pinstripe/global/media/videocontrols.css
@@ +77,5 @@
> +  min-width: 28px;
> +  border: none;
> +}
> +
> +html|*:-moz-full-screen > .mediaControlsFrame > stack > .controlsOverlay > .controlBar > .fullscreenButton {

Hmm. That's kind of unfortunate CSS.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Justin Dolske [:Dolske] from comment #29)
> Suppose a page has <div><video controls/><div> and makes the <div>
> full-screen. What happens, and what should we do? [Also consider
> <div><deeply nested><video/></deep></div>]

In the former case the <div> would become the full-screen element, and the <video> is rendered with whatever style rules apply to it as per usual. I'd say it's the author's responsibility to ensure they have appropriate style rules to get their <video> element to behave as desired. We shouldn't do anything special in that case, otherwise we'd break the not-doing-anything-special case.

Also his isVideoInFullScreen() function will return false in that case, since the video isn't the current full-screen element.

> ::: browser/base/content/nsContextMenu.js
> @@ +858,5 @@
> >    },
> >  
> >    fullScreenVideo: function () {
> > +    if (document.mozFullScreenEnabled) {
> > +      this.target.mozRequestFullScreen();
> 
> Does this behave any differently when chrome calls it vs when the user does?

It will. We will relax some of the security constraints when this is called from chrome (bug 691947). The video controls run in content JS (correct me if I'm wrong), so are subject to the same restrictions as regular full-screen requests.

> @@ +823,5 @@
> > +                },
> > +
> > +                toggleFullscreen : function () {
> > +                    this.isVideoInFullScreen() ?
> > +                        document.mozCancelFullScreen() :
> 
> What happens if the video does full-screen, and then the page makes some
> other element go full-screen afterwards?

The bad guys could easily just write their own video controls (a verbatim copy of ours even) with a full-screen button, and then get full-screen control that way. This is why we have the same restrictions on full-screen in video controls as in the normal content case.

> Or the user uses the context-menu
> to change state? Seems like you need a listener to watch for
> externally-driven state changes. (I presume such an event exists!)

We dispatch a "mozfullscreenchange" event when full-screen is entered or exited. You don't get a notification when the full-screen element changes (without the document's full-screen state changing) however. So if some other element took full-screen while a <video> was full-screen, we wouldn't get a notification and we'd still be showing the exit full-screen button on the controls.

Comment 31

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

(In reply to Justin Dolske [:Dolske] from comment #29)
> Comment on attachment 565097 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 470628 v5
> 
> Review of attachment 565097 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly good from a quickish skim, but a couple remarks.
> 
> Random question:
> 
> Suppose a page has <div><video controls/><div> and makes the <div>
> full-screen. What happens, and what should we do? [Also consider
> <div><deeply nested><video/></deep></div>]

In both cases, if the <div> goes full-screen, the "enter full-screen" button will still be shown to the user. As Chris said, making this work as users expect it relies on a good web developer.

> ::: browser/base/content/nsContextMenu.js
> @@ +858,5 @@
> >    },
> >  
> >    fullScreenVideo: function () {
> > +    if (document.mozFullScreenEnabled) {
> > +      this.target.mozRequestFullScreen();
> 
> Does this behave any differently when chrome calls it vs when the user does?

Please see Chris' answer.

> @@ +865,3 @@
> >  
> > +      openDialog("chrome://browser/content/fullscreen-video.xhtml",
> > +                 "", "chrome,centerscreen,dialog=no", this.target);
> 
> I think we'll want to remove this code path sometime after this patch lands
> and sticks. Followup bug? Probably for the mozLoadFrom() stuff too, but
> that's up to the platform guys.

Followup bug is good. I've logged it here:
https://bugzilla.mozilla.org/show_bug.cgi?id=693728

> ::: toolkit/content/widgets/videocontrols.xml
> @@ +818,5 @@
> >                      // controlling volume.
> >                  },
> >  
> > +                isVideoInFullScreen : function () {
> > +                    return document.mozFullScreen && document.mozFullScreenElement == this.video;
> 
> See, now I can' remember if .isSameNode() and == (===?) do the same thing in
> all case. Jog my memory?
> 

Based on the comments in bug 687400, .isSameNode() simply uses ==. Bug 687400 is to kill .isSameNode().

> @@ +823,5 @@
> > +                },
> > +
> > +                toggleFullscreen : function () {
> > +                    this.isVideoInFullScreen() ?
> > +                        document.mozCancelFullScreen() :
> 
> What happens if the video does full-screen, and then the page makes some
> other element go full-screen afterwards? Or the user uses the context-menu
> to change state? Seems like you need a listener to watch for
> externally-driven state changes. (I presume such an event exists!)
>

The |toggleFullscreen| function will still work as expected if full screen elements change while already in full screen. The button state will also update since it is based on the pseudo-selector :-moz-full-screen which is only applied to the element that is full-screen.
 
> ::: toolkit/themes/pinstripe/global/media/videocontrols.css
> @@ +77,5 @@
> > +  min-width: 28px;
> > +  border: none;
> > +}
> > +
> > +html|*:-moz-full-screen > .mediaControlsFrame > stack > .controlsOverlay > .controlBar > .fullscreenButton {
> 
> Hmm. That's kind of unfortunate CSS.

While unfortunate, it is the best CSS I could come up with to limit display of the full-screen button to only show the "Exit full screen" icon when the <video> is the actual full-screen element.

Comment 32

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

Comment on attachment 565097 [details] [diff] [review]
Patch for bug 470628 v5

Can you please re-review based on the discussion feedback?

Comment 33

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

Attachment: Patch for bug 470628 v5 (unbitrotted)

Unbitrotted. Carrying forward ui-r+ from shorlander.

Comment 34

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

Attachment: Patch for bug 470628 v6

Simplifying the associated mochitest.

Comment 35

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

Comment on attachment 567979 [details] [diff] [review]
Patch for bug 470628 v6

Canceling review and feedback request because Frank and I have noticed a couple issues with this patch.

1. We can now remove the code path for the legacy full screen support since chrome code has a less-restrictive api for full-screen now.
2. The aria-label attribute of the full-screen button wasn't getting updated after initially being set.
3. The selector for the full-screen button can be made simpler by setting an attribute on the button at the same time that we update the aria-label.

Comment 36

[Dão Gottwald [:dao]]

> 1. We can now remove the code path for the legacy full screen support since
> chrome code has a less-restrictive api for full-screen now.

Nope. We still need to be able to disable the full-screen API during the release process. In fact, it's still disabled by default at this point.

Comment 37

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

Attachment: Part 1: CSS, JS, XBL, and locale changes

Fixed the issues that were mentioned earlier. Carrying forward ui-r+ from shorlander.

Comment 38

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

Attachment: Part 2: Full screen button graphic

Comment 39

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

Attachment: Part 3: Mochitest to confirm that the full-screen button works as intended

Comment 40

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

I've added the |tracking-firefox10?| flag since the full-screen DOM API is planned for Fx10 and we should make sure to coordinate this patch with the introduction of the new API.

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 572282 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes

See comment 36.

Comment 42

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

Attachment: Part 1: CSS, JS, XBL, and locale changes (with fall-back support)

Added the ability to fall-back to the legacy implementation.

Comment 43

[Frank Yan (:fryn)]

Comment on attachment 572287 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes (with fall-back support)

> <!ENTITY videoFullScreen.label       "Full Screen">
> <!ENTITY videoFullScreen.accesskey   "F">
>+<!ENTITY videoCancelFullScreen.label     "Exit Full Screen">
>+<!ENTITY videoCancelFullScreen.accesskey "F">
> <!ENTITY videoSaveImage.label        "Save Snapshot As…">
> <!ENTITY videoSaveImage.accesskey    "S">
> <!ENTITY videoShowStats.label        "Show Statistics">
> <!ENTITY videoShowStats.accesskey    "t">
> <!ENTITY videoHideStats.label        "Hide Statistics">
> <!ENTITY videoHideStats.accesskey    "t">

Perhaps, there should be a comment explaining that Show/Hide Statistics should share the same accesskey and so should the (Exit) Full Screen pair.

Comment 44

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

Attachment: Part 1: CSS, JS, XBL, and locale changes (with fall-back support)

Added a localization note to state why the accesskeys are the same for Full Screen and Exit Full Screen.

Carrying forward ui-r+ from shorlander and feedback+ from fryn.

Comment 45

[Justin Dolske [:Dolske]]

Comment on attachment 572739 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes (with fall-back support)

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

Mostly just small fixes needed, and removing the new context menu (or convincing me to keep it ;).

::: browser/base/content/browser-context.inc
@@ +119,5 @@
>                  oncommand="gContextMenu.fullScreenVideo();"/>
> +      <menuitem id="context-video-cancelfullscreen"
> +                accesskey="&videoCancelFullScreen.accesskey;"
> +                label="&videoCancelFullScreen.label;"
> +                oncommand="gContextMenu.cancelFullScreenVideo();"/>

Hmm. Why add this? I'm thinking it would be better to do nothing in this bug, and in a followup add an "Exit Full Screen" to all context menus when in DOM-FS mode.

Also, whenever this is added browser/base/content/test/test_contextmenu.html will need to be updated.

::: browser/base/content/nsContextMenu.js
@@ +417,5 @@
>      this.showItem("context-media-mute",   onMedia && !this.target.muted);
>      this.showItem("context-media-unmute", onMedia && this.target.muted);
>      this.showItem("context-media-showcontrols", onMedia && !this.target.controls);
>      this.showItem("context-media-hidecontrols", onMedia && this.target.controls);
> +    this.showItem("context-video-fullscreen", this.onVideo && (!this.target.ownerDocument.mozFullScreenEnabled || this.target.ownerDocument.mozFullScreenElement != this.target));

I'll defer to UX here, but I'd suggest that we should only show "View Full Screen" when nothing is currently FS. Making something "more full-screener" seems like it would be confusing, and raises the question of how you go back to the "less full-screener" state.

Also, does requesting FS mode (via context menu or video control) when already in FS mode cause the "Press ESC to exit" notice to show again?

@@ +870,5 @@
> +      // Fallback to the legacy full-screen video implementation.
> +      this.target.pause();
> +      openDialog("chrome://browser/content/fullscreen-video.xhtml",
> +                 "", "chrome,centerscreen,dialog=no", this.target);
> +    }

This would read slightly nicer with a |var video = this.target;|.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +429,5 @@
>  <!ENTITY mediaUnmute.label           "Unmute">
>  <!ENTITY mediaUnmute.accesskey       "m">
> +<!-- LOCALIZATION NOTE: The access keys for "Show Controls" and
> +"Hide Controls" are the same because the two context-menu
> +items are mutually exclusive. -->

Might as well do this for play/pause and mute/unmute as well... In for a penny, in for a pound!

::: toolkit/content/widgets/videocontrols.xml
@@ +828,5 @@
>                      // controlling volume.
>                  },
>  
> +                isVideoInFullScreen : function () {
> +                    return document.mozFullScreen && document.mozFullScreenElement == this.video;

Hmm. Any reason why the spec (?) has both, instead of just |.mozFullScreenElement == null| implying not-fullscreen?

@@ +850,5 @@
> +
> +                    if (this.isVideoInFullScreen())
> +                        this.fullscreenButton.setAttribute("cancelFullScreen", "true");
> +                    else
> +                        this.fullscreenButton.removeAttribute("cancelFullScreen");

For consistency with the play/pause buttons... Don't set/remove, just set an attribute to true/false. "isFullScreen" or "fullscreened", perhaps.

@@ +1187,5 @@
>                        this.volumeStack.addEventListener("mouseout",  function(e) { self.onVolumeMouseInOut(e); }, false);
>                      }
>  
>                      this.videocontrols.addEventListener("transitionend", function(e) { self.onTransitionEnd(e); }, false);
> +                    this.videocontrols.addEventListener("mozfullscreenchange", this.setFullscreenButtonState, false);

Please also remove this in terminateEventListeners().

Also seems you'll need a function wrapper here (as with all the addEventListeners above it), otherwise |this| is the event not |Utils|. Does this just happen to work in your current function?

::: toolkit/themes/pinstripe/global/media/videocontrols.css
@@ +65,5 @@
>    background-color: rgba(255,255,255,.5);
>    border-radius: 4px 4px;
>  }
>  
> +.fullscreenButton {

Nit: put this after the .muteButton rules, so all the buttons are together.

@@ +66,5 @@
>    border-radius: 4px 4px;
>  }
>  
> +.fullscreenButton {
> +  background-color: transparent;

I don't think this is needed, the other buttons don't have it.

@@ +74,5 @@
> +  margin: 0;
> +  padding: 0;
> +  min-height: 28px;
> +  min-width: 28px;
> +  border: none;

Not needed for pinstripe (is for winstripe). See other buttons.

Comment 46

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Justin Dolske [:Dolske] from comment #45)

> Also, does requesting FS mode (via context menu or video control) when
> already in FS mode cause the "Press ESC to exit" notice to show again?

Nope. Unless my code doesn't work as expected. ;)


> ::: toolkit/content/widgets/videocontrols.xml
> @@ +828,5 @@
> >                      // controlling volume.
> >                  },
> >  
> > +                isVideoInFullScreen : function () {
> > +                    return document.mozFullScreen && document.mozFullScreenElement == this.video;
> 
> Hmm. Any reason why the spec (?) has both, instead of just
> |.mozFullScreenElement == null| implying not-fullscreen?

We only have both because the original spec needed both. The current implementation doesn't need both, and the W3C draft spec actually doesn't have a document.fullScreen attribute. In our implementation you can safely assume that (document.mozFullScreenElement!=null) implies that you're in full-screen mode. So you could drop the document.mozFullScreen check if you like jwein.

Comment 47

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

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #46)
> (In reply to Justin Dolske [:Dolske] from comment #45)
> 
> > Also, does requesting FS mode (via context menu or video control) when
> > already in FS mode cause the "Press ESC to exit" notice to show again?
> 
> Nope. Unless my code doesn't work as expected. ;)
> 
> 
> > ::: toolkit/content/widgets/videocontrols.xml
> > @@ +828,5 @@
> > >                      // controlling volume.
> > >                  },
> > >  
> > > +                isVideoInFullScreen : function () {
> > > +                    return document.mozFullScreen && document.mozFullScreenElement == this.video;
> > 
> > Hmm. Any reason why the spec (?) has both, instead of just
> > |.mozFullScreenElement == null| implying not-fullscreen?
> 
> We only have both because the original spec needed both. The current
> implementation doesn't need both, and the W3C draft spec actually doesn't
> have a document.fullScreen attribute. In our implementation you can safely
> assume that (document.mozFullScreenElement!=null) implies that you're in
> full-screen mode. So you could drop the document.mozFullScreen check if you
> like jwein.

Ok, that's cool, I'll drop it.

cpearce: Was there a time where document.mozFullScreenElement would equal null if the full-screen element was not inserted into the DOM?

Comment 48

[Justin Dolske [:Dolske]]

Oh, one more comment: I think this breaks mobile, because their controls won't have a FS button, and and the code will croak there when .fullscreenButton == null.

Should be sufficient to add an invisible button in the TouchControls just to get things working, we can get art and test in a followup bug. Margaret knows videocontrols on Android, so she can probably help. :)

Comment 49

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

(In reply to Justin Dolske [:Dolske] from comment #48)
> Oh, one more comment: I think this breaks mobile, because their controls
> won't have a FS button, and and the code will croak there when
> .fullscreenButton == null.
> 
> Should be sufficient to add an invisible button in the TouchControls just to
> get things working, we can get art and test in a followup bug. Margaret
> knows videocontrols on Android, so she can probably help. :)

Doesn't mobile use the TouchControls, and so this code won't be executed on mobile?

Comment 50

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

(In reply to Justin Dolske [:Dolske] from comment #45) 
> > +                    if (this.isVideoInFullScreen())
> > +                        this.fullscreenButton.setAttribute("cancelFullScreen", "true");
> > +                    else
> > +                        this.fullscreenButton.removeAttribute("cancelFullScreen");
> 
> For consistency with the play/pause buttons... Don't set/remove, just set an
> attribute to true/false. "isFullScreen" or "fullscreened", perhaps.
> 

I think we should instead make the mute/unmute and play/pause buttons follow this boolean style. This is the style that is recommended in https://wiki.mozilla.org/DevTools/CSSTips (which is to assume that the presence of an attribute implies |="true"|).

Comment 51

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

Attachment: Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2

Carrying forward feedback+ from fryn and ui-r+ from shorlander.

(In reply to Justin Dolske [:Dolske] from comment #45)
> Comment on attachment 572739 [details] [diff] [review] [diff] [details] [review]
> Part 1: CSS, JS, XBL, and locale changes (with fall-back support)
> 
> Review of attachment 572739 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Mostly just small fixes needed, and removing the new context menu (or
> convincing me to keep it ;).
> 
> ::: browser/base/content/browser-context.inc
> @@ +119,5 @@
> >                  oncommand="gContextMenu.fullScreenVideo();"/>
> > +      <menuitem id="context-video-cancelfullscreen"
> > +                accesskey="&videoCancelFullScreen.accesskey;"
> > +                label="&videoCancelFullScreen.label;"
> > +                oncommand="gContextMenu.cancelFullScreenVideo();"/>
> 
> Hmm. Why add this? I'm thinking it would be better to do nothing in this
> bug, and in a followup add an "Exit Full Screen" to all context menus when
> in DOM-FS mode.

Yeah, we don't need to add to the context menu since there is now a button. I've removed it from this patch, but left it for when the DOM full-screen API is disabled.

> @@ +870,5 @@
> > +      // Fallback to the legacy full-screen video implementation.
> > +      this.target.pause();
> > +      openDialog("chrome://browser/content/fullscreen-video.xhtml",
> > +                 "", "chrome,centerscreen,dialog=no", this.target);
> > +    }
> 
> This would read slightly nicer with a |var video = this.target;|.

OK, I've made the change, but used |let| instead.

> ::: toolkit/content/widgets/videocontrols.xml
> @@ +828,5 @@
> >                      // controlling volume.
> >                  },
> >  
> > +                isVideoInFullScreen : function () {
> > +                    return document.mozFullScreen && document.mozFullScreenElement == this.video;
> 
> Hmm. Any reason why the spec (?) has both, instead of just
> |.mozFullScreenElement == null| implying not-fullscreen?

I'm just checking |.mozFullScreenElement != null| now.

> @@ +850,5 @@
> > +
> > +                    if (this.isVideoInFullScreen())
> > +                        this.fullscreenButton.setAttribute("cancelFullScreen", "true");
> > +                    else
> > +                        this.fullscreenButton.removeAttribute("cancelFullScreen");
> 
> For consistency with the play/pause buttons... Don't set/remove, just set an
> attribute to true/false. "isFullScreen" or "fullscreened", perhaps.

As I've stated above, I've changed the play/pause and mute/unmute to use the attributes as boolean conditions.

> @@ +1187,5 @@
> >                        this.volumeStack.addEventListener("mouseout",  function(e) { self.onVolumeMouseInOut(e); }, false);
> >                      }
> >  
> >                      this.videocontrols.addEventListener("transitionend", function(e) { self.onTransitionEnd(e); }, false);
> > +                    this.videocontrols.addEventListener("mozfullscreenchange", this.setFullscreenButtonState, false);
> 
> Please also remove this in terminateEventListeners().
> 
> Also seems you'll need a function wrapper here (as with all the
> addEventListeners above it), otherwise |this| is the event not |Utils|. Does
> this just happen to work in your current function?

A function wrapper here is unnecessary because the function doesn't need the event passed to it. |this| resolves to the |Utils| object, because the scope hasn't changed while calling the |.addEventListener| function. The event listeners that are added above are created with anonymous functions and thus can't have their event listeners removed, which I'll file as a follow-up bug.

Comment 52

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jwein and :jaws] from comment #51)
> Created attachment 573919 [details] [diff] [review] [diff] [details] [review]
> Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2
> 
> Carrying forward feedback+ from fryn and ui-r+ from shorlander.
> 
> (In reply to Justin Dolske [:Dolske] from comment #45)
> > Comment on attachment 572739 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Part 1: CSS, JS, XBL, and locale changes (with fall-back support)
> > 
> > Review of attachment 572739 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > Mostly just small fixes needed, and removing the new context menu (or
> > convincing me to keep it ;).
> > 
> > ::: browser/base/content/browser-context.inc
> > @@ +119,5 @@
> > >                  oncommand="gContextMenu.fullScreenVideo();"/>
> > > +      <menuitem id="context-video-cancelfullscreen"
> > > +                accesskey="&videoCancelFullScreen.accesskey;"
> > > +                label="&videoCancelFullScreen.label;"
> > > +                oncommand="gContextMenu.cancelFullScreenVideo();"/>
> > 
> > Hmm. Why add this? I'm thinking it would be better to do nothing in this
> > bug, and in a followup add an "Exit Full Screen" to all context menus when
> > in DOM-FS mode.
> 
> Yeah, we don't need to add to the context menu since there is now a button.
> I've removed it from this patch, but left it for when the DOM full-screen
> API is disabled.

As I understand it, dolske was asking that you don't add context-video-cancelfullscreen, not that you hide context-video-fullscreen when DOM full-screen is enabled. The latter seems bogus since a user may want to make a video full-screen even when controls are disabled.

Comment 53

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

(In reply to Dão Gottwald [:dao] from comment #52)
> (In reply to Jared Wein [:jwein and :jaws] from comment #51)
> > Created attachment 573919 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2
> > 
> > Carrying forward feedback+ from fryn and ui-r+ from shorlander.
> > 
> > (In reply to Justin Dolske [:Dolske] from comment #45)
> > > Comment on attachment 572739 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]
> > > Part 1: CSS, JS, XBL, and locale changes (with fall-back support)
> > > 
> > > Review of attachment 572739 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Mostly just small fixes needed, and removing the new context menu (or
> > > convincing me to keep it ;).
> > > 
> > > ::: browser/base/content/browser-context.inc
> > > @@ +119,5 @@
> > > >                  oncommand="gContextMenu.fullScreenVideo();"/>
> > > > +      <menuitem id="context-video-cancelfullscreen"
> > > > +                accesskey="&videoCancelFullScreen.accesskey;"
> > > > +                label="&videoCancelFullScreen.label;"
> > > > +                oncommand="gContextMenu.cancelFullScreenVideo();"/>
> > > 
> > > Hmm. Why add this? I'm thinking it would be better to do nothing in this
> > > bug, and in a followup add an "Exit Full Screen" to all context menus when
> > > in DOM-FS mode.
> > 
> > Yeah, we don't need to add to the context menu since there is now a button.
> > I've removed it from this patch, but left it for when the DOM full-screen
> > API is disabled.
> 
> As I understand it, dolske was asking that you don't add
> context-video-cancelfullscreen, not that you hide context-video-fullscreen
> when DOM full-screen is enabled. The latter seems bogus since a user may
> want to make a video full-screen even when controls are disabled.

Wow, yeah I don't know how but I clearly misunderstood that. Thanks for pointing that out, I'll clear review for now and upload a different patch soon.

Comment 54

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

(In reply to Justin Dolske [:Dolske] from comment #45)
> Comment on attachment 572739 [details] [diff] [review] [diff] [details] [review]
> Part 1: CSS, JS, XBL, and locale changes (with fall-back support)
> 
> Review of attachment 572739 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> @@ +66,5 @@
> >    border-radius: 4px 4px;
> >  }
> >  
> > +.fullscreenButton {
> > +  background-color: transparent;
> 
> I don't think this is needed, the other buttons don't have it.

The other buttons use the |background| property to set the background-image (which also sets the |background-color| to transparent), whereas this uses list-style-image because both states of the button are in the same file. I think either |-moz-appearance: none| or xul:button is setting a background-color on the button, which this is resetting to the initial value. Removing this line makes the button white-ish.

Comment 55

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

Attachment: Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2.1

Added back the context-menu option for full screen if DOM full-screen is enabled.

Carrying forward ui-r+ from shorlander and f+ from fryn.

I've filed bugs for the event listeners that aren't removed (bug 702161) as well as adding a context-menu for exiting full-screen mode (bug 702159).

Comment 56

[Justin Dolske [:Dolske]]

Comment on attachment 574199 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2.1

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

I'm going to mark this r+, though I'd like to understand the interaction of mozFullScreenElement and iframes.

::: browser/base/content/nsContextMenu.js
@@ +417,5 @@
>      this.showItem("context-media-mute",   onMedia && !this.target.muted);
>      this.showItem("context-media-unmute", onMedia && this.target.muted);
>      this.showItem("context-media-showcontrols", onMedia && !this.target.controls);
>      this.showItem("context-media-hidecontrols", onMedia && this.target.controls);
> +    this.showItem("context-video-fullscreen", this.onVideo && this.target.ownerDocument.mozFullScreenElement == null);

I think this is mostly right (though perhaps it would be nice to disable the entry instead of hiding it), but I'm not sure what the FS code does when frames are involved.

For example, if the top-level document is full-screen, and includes a cross-origin iframe with a video in it... What will video.ownerDocument.mozFullScreenElement be?

::: toolkit/content/widgets/videocontrols.xml
@@ +858,5 @@
>                  setPlayButtonState : function(aPaused) {
> +                  if (aPaused)
> +                      this.playButton.setAttribute("paused", "true");
> +                  else
> +                      this.playButton.removeAttribute("paused");

Kind of a gratuitous change, since it means themes have to change to support how the mute/play button attributes work now. But it's minor, so meh.

@@ +1194,5 @@
>                        this.volumeStack.addEventListener("mouseout",  function(e) { self.onVolumeMouseInOut(e); }, false);
>                      }
>  
>                      this.videocontrols.addEventListener("transitionend", function(e) { self.onTransitionEnd(e); }, false);
> +                    this.videocontrols.addEventListener("mozfullscreenchange", this.setFullscreenButtonState, false);

This should be on this.video.ownerDocument, so we notice when other things go FS.

::: toolkit/locales/en-US/chrome/global/videocontrols.dtd
@@ +1,5 @@
>  <!ENTITY playButton.playLabel "Play">
>  <!ENTITY playButton.pauseLabel "Pause">
>  <!ENTITY muteButton.muteLabel "Mute">
>  <!ENTITY muteButton.unmuteLabel "Unmute">
> +<!ENTITY fullscreenButton.enterfullscreenlabel "Enter Full Screen">

Just "Full Screen", to match the context menu.

Comment 57

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #56)

> For example, if the top-level document is full-screen, and includes a
> cross-origin iframe with a video in it... What will
> video.ownerDocument.mozFullScreenElement be?

roc says it'll be null, and generally can't tell anything about if the parent doc is FS or not.

We may want to tweak out controls so that we always offer full-screen (unless the video itself is the FS element). But I think this patch is fine for now. It's just slightly inconsistent when content in an iframe is involved.

Comment 58

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jwein and :jaws] from comment #49)

> Doesn't mobile use the TouchControls, and so this code won't be executed on
> mobile?

Oh, forgot to reply to this. :/ Yes, they use the TouchControls binding, but it just extends the base videocontrol binding. So I suspect there's still potential breakage.

Comment 59

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

I have pushed parts 1 and 2 to fx-team, and moved part 3 to a follow-up bug due to random oranges on Linux opt/debug and WinXP opt/debug.

https://tbpl.mozilla.org/?tree=Fx-Team&rev=c5c8d300a9d3

Comment 60

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

Comment on attachment 572284 [details] [diff] [review]
Part 3: Mochitest to confirm that the full-screen button works as intended

Obsoleted, see bug 704924 for updates to this patch.

Comment 61

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

https://hg.mozilla.org/integration/fx-team/rev/c5c8d300a9d3
https://hg.mozilla.org/integration/fx-team/rev/87390d4620e2

Comment 62

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

Attachment: Part 3: Update tests to accommodate new full screen button.

This was part of the part 3 patch that was moved to the other bug, but this part needs to be pushed because it just updates the previous controls tests to accommodate the presence of the new full screen button.

https://hg.mozilla.org/integration/fx-team/rev/73158c3d78ff

Comment 63

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/87390d4620e2

Comment 64

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/c5c8d300a9d3

https://hg.mozilla.org/mozilla-central/rev/73158c3d78ff

Comment 65

[SpeciesX]

Attachment: Full screen button

Why is the full screen button so big?

Comment 66

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

(In reply to speciesx from comment #65)
> Created attachment 576933 [details]
> Full screen button
> 
> Why is the full screen button so big?

I can't reproduce that bug with the latest nightly from 25-Nov-2011 06:30 on Windows 7. What platform are you on and what build? Can you please file a bug with more details and set this bug as a dependency?

Comment 67

[SpeciesX]

i found the problem, it caused by Australis theme, without it works right. sry my fault.

Comment 68

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

(In reply to speciesx from comment #67)
> i found the problem, it caused by Australis theme, without it works right.
> sry my fault.

Thank you for tracking down the cause, but a theme should not be able to change the appearance of these controls...

Comment 69

[SpeciesX]

(In reply to Jared Wein [:jwein and :jaws] from comment #68)
> Thank you for tracking down the cause, but a theme should not be able to
> change the appearance of these controls...

should not, but it do.

https://addons.mozilla.org/en-us/firefox/addon/australis

Comment 70

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

(In reply to speciesx from comment #69)
> (In reply to Jared Wein [:jwein and :jaws] from comment #68)
> > Thank you for tracking down the cause, but a theme should not be able to
> > change the appearance of these controls...
> 
> should not, but it do.
> 
> https://addons.mozilla.org/en-us/firefox/addon/australis

Sorry, I didn't mean to say that a theme shouldn't be able to change the appearance. I meant to say that this shouldn't have happened on mistake. Thanks for the link to the theme.

Comment 71

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

Comment on attachment 576600 [details] [diff] [review]
Part 3: Update tests to accommodate new full screen button.

I have requested for these three attachments to be approved for Firefox Aurora so the fullscreen button will align with the enabling of the DOM fullscreen feature. These patches are small and can be easily backed out if they are found to cause issues.

I should note that as per comment 67, some themes may alter the appearance of the fullscreen button in a negative way. I have not spent the time yet to figure out why the Australis theme is breaking the fullscreen button.

Comment 72

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

If the patches for this bug get aurora-approval, then we will also want to uplift the patch for bug 705906 to Aurora as well.

Comment 73

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think we should implement bug 699719 before we ship this on any branch (i.e. we'd want bug 699719 on Aurora too...). Without the patch in bug 699719 the video controls will always be visible while a video is in full-screen mode with the changes in this bug.

Comment 74

[Alex Keybl [:akeybl]]

Comment on attachment 574199 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2.1

[Triage Comment]
Since the user can still fullscreen by right clicking and this isn't a regression, let's let this ride the train.

Comment 75

[[e-mail]]

Comment on attachment 574199 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes (with fall-back support) v2.1

>+                    this.video.ownerDocument.addEventListener("mozfullscreenchange", this.setFullscreenButtonState, false);
This calls setFullscreenbuttonState with a "this" of the document...

Comment 76

[[e-mail]]

Oh, that's bug 706329.

Comment 77

[Mihaela Velimiroviciu (:mihaelav)]

Verified on latest Aurora (11.0a2) builds accross platforms (XP, Win7 x86, Win 7 64 bit, Ubuntu 11.04 x86, Ubuntu 11.04 64 bit, Mac 10.6) and full screen/exit full screen buttons are attached to html5 videos.
Marking as VERIFIED.