Open Bug 713608 Opened 12 years ago Updated 2 years ago

HTML5 Video controls are missing in Fullscreen

Categories

(Toolkit :: Video/Audio Controls, defect)

11 Branch
defect

Tracking

()

Webcompat Priority P3

People

(Reporter: alice0775, Unassigned)

References

()

Details

(Whiteboard: [lang=js][webcompat:p3])

Attachments

(7 obsolete files)

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/9f29daaecbcc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20111226 Firefox/12.0a1 ID:20111226031002

HTML5 Video controls are missing in Fullscreen 

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with clean profile
2. Open http://mozilla.jp/firefox/videos/
3. Play the video
4. Right click on the video and choose "Full Screen"

5. Move mouse pointer(if necessary)

Actual Results:
  No video controls appear

Expected Results:
  The video controls should appear at bottom of screen
  (and the video controls should re-appear when I moves mouse pointer)


Regression window:
Works;
http://hg.mozilla.org/mozilla-central/rev/de483d897af4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111124 Firefox/11.0a1 ID:20111124031031
Fails:
http://hg.mozilla.org/mozilla-central/rev/1ac01535c4df
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111123 Firefox/11.0a1 ID:20111124074526
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=de483d897af4&tochange=1ac01535c4df
Severity: normal → trivial
The Mozilla Japan site uses their own custom controls, and choosing "Full Screen" from the context menu doesn't bring their custom controls along with the video (probably shouldn't, since it will be impossible to know which element contains the custom controls).

However, we probably should by default enable the stock controls if the user enters Full Screen via the context menu. But then, do we remove the stock controls when they exit full screen mode?

Also, I noticed that the "Exit Full Screen" context menu item is not available when in Full Screen mode. Alice: Do you see the "Exit Full Screen" context menu option when in Full Screen mode?
(In reply to Jared Wein [:jwein and :jaws] (Vacation December 23-January 3) from comment #1)

> Also, I noticed that the "Exit Full Screen" context menu item is not
> available when in Full Screen mode. Alice: Do you see the "Exit Full Screen"
> context menu option when in Full Screen mode?

There is not "Exit Full Screen"menuitem  in context menu.
Also,no "Exit Full Screen"menuitem on the following
http://www.mozilla.org/projects/firefox/prerelease.html
http://www.mozilla.org/en-US/firefox/9.0.1/whatsnew/

I think Fullscreen API  does not support "Exit Full Screen"menuitem  in context menu.
Thanks for filing this bug Alice.

Mozilla Japan can work around this problem by having their custom controls create a transparent <div> over the video element. This <div> will capture the right mouse click, stopping click from being delivered to the <video>, so the video's context menu won't show. This means the "Full screen" context menu item will never be used. YouTube do something similar in their HTML5 player, and they create their own right-click context menu.

I'm not sure what we (Firefox developers) can or should do about this...
Ok
I think that this is the site problem.
and this seems to be INVALID.
Let's go ahead and enable the stock controls if the user enters fullscreen via the context menu and disable them when exiting if they were disabled prior to entering fullscreen.

The video controls will need to listen for the mozfullscreenchange event. When they get it and see that they are in fullscreen, then they can add the controls attribute to the video. They will also need to remember if they added it themselves, so that they can remove the attribute when the next mozfullscreenchange event and they find out that they are no longer in fullscreen.

The video controls file is located at:
/toolkit/content/widgets/videocontrols.xml
Whiteboard: [good first bug][mentor=jwein][lang=js]
Greg: Would you like to work on this bug?
OS: Windows 7 → All
Hardware: x86 → All
Assignee: nobody → diogo.gmt
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
When entering fullscreen via the context menu the default video controls are enable.
When the video exits fullscreen, the controls are restored to their previous state prior entering fullscreen.

I'm getting some inconsistent failures that causes the solution to break.

[10:07:48.068] TypeError: this.video is null @ chrome://global/content/bindings/videocontrols.xml:394

The line that is causing the error:
|this.setPlayButtonState(this.video.paused);|
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#394


[10:27:48.049] ReferenceError: Utils is not defined @ chrome://global/content/bindings/videocontrols.xml:846
The line that is causing the error:
|if (!Utils.scrubber.isDragging) {|
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#827


[17:40:45.326] TypeError: this.scrubber.valueChanged is not a function @ chrome://global/content/bindings/videocontrols.xml:932

The line that is causing the error:
|this.scrubber.valueChanged("curpos", this.video.currentTime * 1000, false);|
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#913


If the code doesn't hit the errors above the solution seems to be working.


One thing I wasn't sure what was the proper behaviour.
If the video enter fullscreen via the custom controls, then from the context menu selects the "Show Controls" option.
When the video exits fullscreen the default controls should be hidden, right?
That's how the patch works right now. However, if the video enters fullscreen via the custom controls, then the default controls are selected from the context menu and then the video goes fullscreen via the default controls. When fullscreen is lost the default controls are shown with the custom controls altogether.
What happens is the div goes fullscreen in the first place, then with the controls being displayed the video goes fullscreen, so it thinks that when fullscreen is lost the default controls should be kept visible, but in the first place, before the div went to fullscreen the default controls were hidden.

What would be the best behaviour in this case?
Should be up to the developer decide what to do with the default controls?


** The reason why there is a check on maybeShowControls before saving the current state of the video controls is because when the video changes fullscren mode the mozfullscreenChange is fired twice. Maybe it is related to Bug 718107
Attachment #619462 - Flags: feedback?(jwein)
Comment on attachment 619462 [details] [diff] [review]
Patch v1

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

I think we might be able to remove the maybeShowControls variable. This part of html.css (http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#744) is what make the controls visible/hidden based on the [controls] attribute.

I think what we can do is:
Within Utils.init(binding):
> this.binding = binding;

Have an event listener for mozfullscreenchange that does:
> if (document.mozFullScreenElement == this.video)
>   this.binding.classList.add("forceControls");
> else
>   this.binding.classList.remove("forceControls");

And then within toolkit/content/widgets/videocontrols.css:
> html|video:not([controls]) > videocontrols.forceControls,
> html|audio:not([controls]) > videocontrols.forceControls {
>   visibility: visible;
> }

::: toolkit/content/widgets/videocontrols.xml
@@ +947,5 @@
> +                    }
> +
> +                    // Toggle default controls when entering/exiting fullscreen
> +                    if (this.isVideoInFullScreen()) {
> +                        this.video.setAttribute("controls", "true");

I don't think we can/should set attributes on observable elements since that can affect script on the page if they are listening for DOMMutationEvents.
Attachment #619462 - Flags: feedback?(jwein)
Sorry for leading you down the path of setting the attribute in comment #5.
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
(In reply to Jared Wein [:jaws] from comment #9)
> Sorry for leading you down the path of setting the attribute in comment #5.

No worries.

Thank you for the feedback.
I'll start working on a new patch following your suggestions.

One question about making the controls visible:
If we set the css to display the controls, will it be able to auto hide when there is no mouse interaction? Since the controls attribute of the video would be undefined and the over,out and move handlers wouldn't be attached to the video.
Attached patch v2 (obsolete) — Splinter Review
I followed the suggestions given in the feedback and now a mozfullscreenchange listener adds and removes a class that sets the visibility of the stock controls.

I ran some tests on the page http://mozilla.jp/firefox/videos/ and the results were the following:

1 - Enter fullscreen via the context menu
2 - Stock controls are displayed in the video
3 - Exit fullscreen
4 - Stock controls are hidden again

One thing I noticed:
Once the video is in fullscreen mode, there is no context menu option to exit fullscreen.
Shouldn't the video have an option to exit fullscreen from the context menu?
Attachment #619462 - Attachment is obsolete: true
Attachment #625874 - Flags: review?(jaws)
(In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #11)
> Once the video is in fullscreen mode, there is no context menu option to
> exit fullscreen.
> Shouldn't the video have an option to exit fullscreen from the context menu?

That is bug 702159 :)
Comment on attachment 625874 [details] [diff] [review]
v2

David, is adding this style to html.css acceptable?
Attachment #625874 - Flags: review?(dbaron)
Comment on attachment 625874 [details] [diff] [review]
v2

Adding to html.css seems fine, but why not just modify the previous rule to be:

video:not([controls]) > xul|videocontrols:not(.forceControls),
audio:not([controls]) > xul|videocontrols:not(.forceControls) {
  visibility: hidden;
}

or perhaps even:

:-moz-any(video,audio):not([controls]) > xul|videocontrols:not(.forceControls) {
  visibility: hidden;
}
Comment on attachment 625874 [details] [diff] [review]
v2

Diogo, can you make the change that David asked for?
Attachment #625874 - Flags: review?(jaws)
Attached patch v3 (obsolete) — Splinter Review
Updated the css rule for video and audio controls as suggested in the review.
Attachment #625874 - Attachment is obsolete: true
Attachment #628205 - Flags: review?(jaws)
Comment on attachment 628205 [details] [diff] [review]
v3

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

This looks good to me. I just want to double check with Justin to make sure that he agrees with this approach.
Attachment #628205 - Flags: review?(jaws)
Attachment #628205 - Flags: review?(dolske)
Attachment #628205 - Flags: review+
Comment on attachment 628205 [details] [diff] [review]
v3

New html.css changes look fine to me; thanks.
Attachment #628205 - Flags: review+
Attachment #628205 - Flags: feedback+
Attachment #628205 - Flags: review?(dolske)
Thanks for sticking with this Diogo. I've pushed this to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/feec545f5f98
Flags: in-testsuite-
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/feec545f5f98
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Jared Wein [:jaws] from comment #19)
> Thanks for sticking with this Diogo. I've pushed this to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/feec545f5f98

Thank you for helping me with this and with the other bugs. I really appreciate your help :D
I learned a lot of cool stuff during the process.
I'm just glad that I was able to contribute something back to Firefox.
Blocks: 760696
Depends on: 762903
Attached patch Backout of patch (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug and the patch for bug 760696
User impact if declined: Unable to turn off controls in fullscreen video
Testing completed (on m-c, etc.): backout of patch
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: none

This patch and the patch for bug 760696 are planned to be backed out with a new patch for bug 713608 on its way.

See the discussion in bug 762903 (https://bugzilla.mozilla.org/show_bug.cgi?id=762903#c8) for more information here.
Attachment #634579 - Flags: approval-mozilla-aurora?
Backed out on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd641bd82e52

Diogo, please move your work in bug 762903 to this bug.

Changing to REOPENED as the plan that is described here: https://bugzilla.mozilla.org/show_bug.cgi?id=762903#c8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #634579 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out of aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc7eb13d18de
Flags: in-testsuite-
Target Milestone: mozilla15 → ---
Attached patch v4 (obsolete) — Splinter Review
Renamed maybeHideControls flag to mustRemoveControlsOnExitFullscreen
Initializing flag to "undefined" instead of -1

Videos on pages:
http://mozilla.jp/firefox/videos/
www.getpersonas.com
http://tinyvid.tv/
Behave as expected.

When video enters fullscreen from the context menu the controls are made visible and the previous state of the controls is saved.
When exiting fullscreen the state of the controls prior acquiring fullscreen is restored.
Attachment #628205 - Attachment is obsolete: true
Attachment #634579 - Attachment is obsolete: true
Attachment #636578 - Flags: review?(cpearce)
Comment on attachment 636578 [details] [diff] [review]
v4

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

::: toolkit/content/widgets/videocontrols.xml
@@ +401,5 @@
>                      this.setFullscreenButtonState();
>  
> +                    // Bug 713608. Show the default controls if video
> +                    // enters fullscreen mode via the context menu.
> +                    this.mustRemoveControlsOnExitFullscreen = undefined;

You dont' need to init mustRemoveControlsOnExitFullscreen; it is already undefined when the controls are created.

@@ +1023,5 @@
>                  },
>  
> +                maybeShowControls: function () {
> +                    // Keep track if the default controls were enabled when entering fullscreen
> +                    if (this.mustRemoveControlsOnExitFullscreen === undefined) {

Move this check inside the isVideoInFullScreen brach, keep the if undefined check; since maybeShowControls is called multiple times during the transition into fullscreen, without this you'll overwrite the value of maybeShowControls during the transition.

@@ +1031,5 @@
> +                    // Toggle default controls when entering/exiting fullscreen
> +                    if (this.isVideoInFullScreen()) {
> +                        this.video.setAttribute("controls", "true");
> +                    } else {
> +                        this.mustRemoveControlsOnExitFullscreen && this.video.removeAttribute("controls");

} else if (this.mustRemoveControlsOnExitFullscreen) {
    this.video.removeAttribute("controls");
    this.mustRemoveControlsOnExitFullscreen = undefined;
}

i.e. reset mustRemoveControlsOnExitFullscreen so that it doesn't keep its value and interfere with the next time the video enters fullscreen. Without this, if you hide controls, enter fullscreen, exit fullscreen, show controls, enter fullscreen, exit fullscreen, the controls will be hidden, but they should still be shown. This happens because mustRemoveControlsOnExitFullscreen retains the value for when we entered fullscreen the first time without controls.
Attachment #636578 - Flags: review?(cpearce) → review-
Attached patch v5 (obsolete) — Splinter Review
Addressed review comments.
Attachment #636578 - Attachment is obsolete: true
Attachment #642470 - Flags: review?(cpearce)
Comment on attachment 642470 [details] [diff] [review]
v5

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1011,5 @@
>                      // controlling volume.
>                  },
>  
>                  isVideoInFullScreen : function () {
> +                    // Keep track if the default controls were enabled when entering fullscreen

Put this in the "if (this.isVideoInFullScreen())" true path. If you put this in isVideoInFullScreen() then that function is a modifier, but it looks like a modifier, and anytime it's called we'll be setting up andmustRemoveControlsOnExitFullscreen, which may not be what other callers expect.
Attachment #642470 - Flags: review?(cpearce) → review-
since diogo hasn't replied for long can i complete this bug as my first?
Assignee: diogo.gmt → nobody
Assignee: nobody → kunalbansal.02
Attached patch v6 (obsolete) — Splinter Review
i have made the changes as instructed.
Attachment #732164 - Flags: review?(cpearce)
Comment on attachment 732164 [details] [diff] [review]
v6

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

Hmm... I built this patch, but the original problem reported in comment 0 is not actually fixed by it?
Kunal, could you address Chris' review in comment 32?
Flags: needinfo?(kunalbansal.02)
Assignee: kunalbansal.02 → nobody
Flags: needinfo?(kunalbansal.02)
Status: REOPENED → NEW
I tried both of the patches (they both have hunk errors), and after applying them by hand, I couldn't get the controls to show up. Instead, an overlay asking me if I am alright with allowing mozilla.jp to be displayed fullscreen shows up. Once I open up the context menu (after selecting allow on the overlay), a menu item called "Show Controls" is available, and selecting it displays controls at the bottom.
Tareq, it sounds like both of the patches don't work. I'll mark them as obsolete.
Attachment #642470 - Attachment is obsolete: true
Attachment #732164 - Attachment is obsolete: true
Mentor: jaws
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][lang=js]
Issue still reproducible with STR from comment 0 and 34 beta 10 (Build ID: 20141117202603) on Windows 7 64-bit, Windows 8 32-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit.
Hey guys and gals,
I am interested in taking a stab at this bug.  I am completely new to open source and the Mozilla source code.  I looked at the link listed in comment 0 and I was wondering if this is the file that I should be looking at to find the bug.  Any direction would be much appreciated.  Thanks in advance.
Flags: needinfo?(jaws)
While you're waiting for direction, you could take a look at the previous patches attached here for some inspiration. Click "Show Obsolete" to see them.
The source file mentioned in comment 5 is also likely relevant here.
Hi Benjamin, the file you'll want to edit is still toolkit/content/widgets/videocontrols.xml. The website in comment 0 still shows the bug.
Flags: needinfo?(jaws)
Excellent, thanks for all of the info guys. I will jump on this and start taking a look at it.
Hi, I want to fix this bug can you guide me through
I have access the file and modified the code by changing the video:not controls from hidden to visibility. With this the video controls will be visible and active on full screen.

video:not([controls]) > xul|videocontrols,
audio:not([controls]) > xul|videocontrols {
  visibility: visible;
This bug will require a much more complicated patch than the one outlined in comment #43. Please read through all the comments in this bug to get an idea of the complications related to fixing this.
This isn't a regression per-se. This bug existed when fullscreen support was added, but prior to adding fullscreen support there was no way to show the video in fullscreen natively.
Keywords: regression
Whiteboard: [good first bug][lang=js] → [lang=js]
Let's give this bug a little nudge. 

In bug 1407604, we received a report about missing full screen controls for a Baidu property. As it turns out, they simply call requestFullscreen() on the <video> element, which is hiding their controls, obviously. However, in Chrome, native controls show up.

I did a bit of investigation: Edge, Safari and Chrome display native video controls when in fullscreen, but we do not. As it turns out, showing video controls when in fullscreen is perfectly valid, even if the controls attribute is missing:

> In such an independent viewing mode, however, user agents may make full user interfaces visible, even if the controls attribute is absent.
(https://html.spec.whatwg.org/multipage/media.html#the-video-element:expose-a-user-interface-to-the-user)

For the sake of having consistent behavior across browsers, we should do that as well.
Flags: webcompat?
See Also: → 1407604
(In reply to Dennis Schubert [:denschub] from comment #46)
> I did a bit of investigation: Edge, Safari and Chrome display native video
> controls when in fullscreen, but we do not. As it turns out, showing video
> controls when in fullscreen is perfectly valid, even if the controls
> attribute is missing:

Parity for the sake of predictability for developers seems good here, IMO.
Flags: webcompat? → webcompat+
Whiteboard: [lang=js] → [lang=js][webcompat:p3]

See bug 1547409. Migrating webcompat priority whiteboard tags to project flags.

Webcompat Priority: --- → P3
Severity: trivial → S4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: