The default bug view has changed. See this FAQ.

Provide a Full Screen button

VERIFIED FIXED in mozilla11

Status

()

Toolkit
Video/Audio Controls
--
enhancement
VERIFIED FIXED
8 years ago
5 years ago

People

(Reporter: Biju, Assigned: jaws)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10-)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(5 attachments, 14 obsolete attachments)

320 bytes, image/png
Details
6.68 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
23.09 KB, patch
Dolske
: review+
jaws
: feedback+
Details | Diff | Splinter Review
1.80 KB, patch
jaws
: review+
Details | Diff | Splinter Review
170.64 KB, image/png
Details
(Reporter)

Description

8 years ago
One of the item we left out in bug 448909 was the Full Screen button
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 453063
(Reporter)

Updated

8 years ago
Blocks: 470629
(Reporter)

Comment 2

8 years ago
Justin I thought bug 453063 is for back end not for controls
No longer depends on: 453063
When the back end exists we can look at how to expose various controls, let's not get ahead of ourselves.

Comment 4

8 years ago
Now that fullscreen support has landed, should we reopen this bug or file a new one?
(Reporter)

Comment 5

8 years ago
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Updated

8 years ago
Status: REOPENED → NEW
Component: Video/Audio → General
Product: Core → Firefox
QA Contact: video.audio → general
Target Milestone: --- → Firefox 3.6
(Reporter)

Updated

8 years ago
Flags: wanted-firefox3.6?

Comment 6

7 years ago
can i work on this?
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.
Component: General → Video/Audio Controls
Flags: wanted-firefox3.6?
Product: Firefox → Toolkit
QA Contact: general → video.audio
Target Milestone: Firefox 3.6 → ---

Comment 8

7 years ago
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

7 years ago
Created attachment 429741 [details]
video full screen button

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

Updated

7 years ago
Assignee: nobody → mbchandar

Updated

7 years ago
Summary: AV: Provide a Full Screen button → Provide a Full Screen button
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!
(In reply to comment #10)
> The video controls xbl actually has chrome privileges, right?

It doesn't, AFAIK.
(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>
There's nothing that stops unprivileged code from implementing XPCOM interfaces, AFAIK.
Oh, I was under the impression that in order to access Components.xxx, you have to have chrome privileges.

Updated

7 years ago
Depends on: 585165

Updated

7 years ago
Depends on: 545812
No longer depends on: 585165
Assignee: mbchandar → nobody
Attachment #429741 - Attachment is obsolete: true
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Created attachment 563789 [details] [diff] [review]
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.
Attachment #563789 - Flags: ui-review?(shorlander)
Attachment #563789 - Flags: review?(dolske)
Created attachment 563792 [details] [diff] [review]
Patch for bug 470628 (with icon)

Forgot the sweet icon.
Attachment #563789 - Attachment is obsolete: true
Attachment #563792 - Flags: ui-review?(shorlander)
Attachment #563792 - Flags: review?(dolske)
Attachment #563789 - Flags: ui-review?(shorlander)
Attachment #563789 - Flags: review?(dolske)
Also, you'll need to set the pref full-screen-api.enabled to true in about:config for the button to work.
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.
Depends on: 689058
Created attachment 563927 [details] [diff] [review]
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.
Attachment #563792 - Attachment is obsolete: true
Attachment #563927 - Flags: ui-review?(shorlander)
Attachment #563927 - Flags: review?(dolske)
Attachment #563792 - Flags: ui-review?(shorlander)
Attachment #563792 - Flags: review?(dolske)
Created attachment 564392 [details] [diff] [review]
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.
Attachment #563927 - Attachment is obsolete: true
Attachment #564392 - Flags: ui-review?(shorlander)
Attachment #564392 - Flags: review?(dolske)
Attachment #563927 - Flags: ui-review?(shorlander)
Attachment #563927 - Flags: review?(dolske)
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?
(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.
Created attachment 564670 [details] [diff] [review]
Patch for bug 470628 v4

Updated to fix the test failure when run in batch, as well as to check for audio files.
Attachment #564392 - Attachment is obsolete: true
Attachment #564670 - Flags: ui-review?(shorlander)
Attachment #564670 - Flags: review?(dolske)
Attachment #564392 - Flags: ui-review?(shorlander)
Attachment #564392 - Flags: review?(dolske)
Keywords: uiwanted
(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...
(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 :)
Depends on: 691947
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.
Attachment #564670 - Flags: ui-review?(shorlander) → ui-review+
Created attachment 565027 [details]
Fullscreen video icon
Created attachment 565097 [details] [diff] [review]
Patch for bug 470628 v5

I've added the button that shorlander created. Carrying forward ui-r+ from shorlander.
Attachment #564670 - Attachment is obsolete: true
Attachment #565097 - Flags: ui-review+
Attachment #565097 - Flags: review?(dolske)
Attachment #564670 - Flags: review?(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.
Attachment #565097 - Flags: review?(dolske) → review-
(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.
(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 on attachment 565097 [details] [diff] [review]
Patch for bug 470628 v5

Can you please re-review based on the discussion feedback?
Attachment #565097 - Flags: review- → review?(dolske)
Keywords: uiwanted
Created attachment 567978 [details] [diff] [review]
Patch for bug 470628 v5 (unbitrotted)

Unbitrotted. Carrying forward ui-r+ from shorlander.
Attachment #565097 - Attachment is obsolete: true
Attachment #567978 - Flags: ui-review+
Attachment #567978 - Flags: review?(dolske)
Attachment #565097 - Flags: review?(dolske)
Created attachment 567979 [details] [diff] [review]
Patch for bug 470628 v6

Simplifying the associated mochitest.
Attachment #567978 - Attachment is obsolete: true
Attachment #567979 - Flags: ui-review+
Attachment #567979 - Flags: review?(dolske)
Attachment #567978 - Flags: review?(dolske)
Attachment #567979 - Flags: feedback?(fryn)
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.
Attachment #567979 - Flags: review?(dolske)
Attachment #567979 - Flags: feedback?(fryn)
> 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.
Created attachment 572282 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes

Fixed the issues that were mentioned earlier. Carrying forward ui-r+ from shorlander.
Attachment #567979 - Attachment is obsolete: true
Attachment #572282 - Flags: ui-review+
Attachment #572282 - Flags: review?(dolske)
Created attachment 572283 [details] [diff] [review]
Part 2: Full screen button graphic
Attachment #572283 - Flags: review?(dolske)
Created attachment 572284 [details] [diff] [review]
Part 3: Mochitest to confirm that the full-screen button works as intended
Attachment #572284 - Flags: review?(dolske)
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.
tracking-firefox10: --- → ?
Comment on attachment 572282 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes

See comment 36.
Attachment #572282 - Flags: review?(dolske) → review-
Created attachment 572287 [details] [diff] [review]
Part 1: CSS, JS, XBL, and locale changes (with fall-back support)

Added the ability to fall-back to the legacy implementation.
Attachment #572282 - Attachment is obsolete: true
Attachment #572287 - Flags: ui-review+
Attachment #572287 - Flags: review?(dolske)

Updated

6 years ago
Blocks: 693728

Comment 43

6 years ago
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.
Attachment #572287 - Flags: feedback+
Created attachment 572739 [details] [diff] [review]
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.
Attachment #572287 - Attachment is obsolete: true
Attachment #572739 - Flags: ui-review+
Attachment #572739 - Flags: review?(dolske)
Attachment #572739 - Flags: feedback+
Attachment #572287 - Flags: review?(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.
Attachment #572739 - Flags: review?(dolske) → review-
Attachment #572283 - Flags: review?(dolske) → review+
Attachment #572284 - Flags: review?(dolske) → review+
(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.
(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?
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. :)
(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?

Updated

6 years ago
Blocks: 572213
(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"|).
Created attachment 573919 [details] [diff] [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]
> 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.
Attachment #572739 - Attachment is obsolete: true
Attachment #573919 - Flags: ui-review+
Attachment #573919 - Flags: review?(dolske)
Attachment #573919 - Flags: feedback+
(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.
(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.
Attachment #573919 - Flags: review?(dolske)
(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.
Created attachment 574199 [details] [diff] [review]
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).
Attachment #573919 - Attachment is obsolete: true
Attachment #574199 - Flags: ui-review+
Attachment #574199 - Flags: review?(dolske)
Attachment #574199 - Flags: feedback+
Blocks: 702894
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.
Attachment #574199 - Flags: review?(dolske) → review+
(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.
(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.
Blocks: 704229
Blocks: 704924
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
Whiteboard: [fixed-in-fx-team]
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.
Attachment #572284 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/c5c8d300a9d3
https://hg.mozilla.org/integration/fx-team/rev/87390d4620e2
Created attachment 576600 [details] [diff] [review]
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
Attachment #576600 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/87390d4620e2
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/c5c8d300a9d3

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

Comment 65

5 years ago
Created attachment 576933 [details]
Full screen button

Why is the full screen button so big?
(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

5 years ago
i found the problem, it caused by Australis theme, without it works right. sry my fault.
(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

5 years ago
(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
(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.
Attachment #572283 - Flags: approval-mozilla-aurora?
Attachment #574199 - Flags: approval-mozilla-aurora?
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.
Attachment #576600 - Flags: approval-mozilla-aurora?
Blocks: 705906
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.
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 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.
Attachment #574199 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

5 years ago
Attachment #572283 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

5 years ago
Attachment #576600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Depends on: 706329
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...
Oh, that's bug 706329.

Updated

5 years ago
Depends on: 712489

Updated

5 years ago
Depends on: 713608
Blocks: 715469

Updated

5 years ago
tracking-firefox10: ? → -
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.