Open
Bug 713608
Opened 12 years ago
Updated 2 years ago
HTML5 Video controls are missing in Fullscreen
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
NEW
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
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Severity: normal → trivial
Comment 1•12 years ago
|
||
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?
Reporter | ||
Comment 2•12 years ago
|
||
(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.
Comment 3•12 years ago
|
||
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...
Reporter | ||
Comment 4•12 years ago
|
||
Ok I think that this is the site problem. and this seems to be INVALID.
Comment 5•12 years ago
|
||
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]
Comment 6•12 years ago
|
||
Greg: Would you like to work on this bug?
OS: Windows 7 → All
Hardware: x86 → All
Updated•12 years ago
|
Assignee: nobody → diogo.gmt
Status: NEW → ASSIGNED
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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)
Comment 9•12 years ago
|
||
Sorry for leading you down the path of setting the attribute in comment #5.
Updated•12 years ago
|
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jaws][lang=js]
Comment 10•12 years ago
|
||
(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.
Comment 11•12 years ago
|
||
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)
Comment 12•12 years ago
|
||
(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 13•12 years ago
|
||
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; }
Attachment #625874 -
Flags: review?(dbaron) → review-
Comment 15•12 years ago
|
||
Comment on attachment 625874 [details] [diff] [review] v2 Diogo, can you make the change that David asked for?
Attachment #625874 -
Flags: review?(jaws)
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #628205 -
Flags: feedback+
Updated•12 years ago
|
Attachment #628205 -
Flags: review?(dolske)
Comment 19•12 years ago
|
||
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
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/feec545f5f98
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 21•12 years ago
|
||
(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.
Comment 22•12 years ago
|
||
[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?
Comment 23•12 years ago
|
||
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 → ---
Updated•12 years ago
|
Attachment #634579 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•12 years ago
|
||
Backed merged: https://hg.mozilla.org/mozilla-central/rev/dd641bd82e52
Comment 25•12 years ago
|
||
Backed out of aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/bc7eb13d18de
Flags: in-testsuite-
Target Milestone: mozilla15 → ---
Comment 26•12 years ago
|
||
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 27•12 years ago
|
||
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-
Comment 28•12 years ago
|
||
Addressed review comments.
Attachment #636578 -
Attachment is obsolete: true
Attachment #642470 -
Flags: review?(cpearce)
Comment 29•12 years ago
|
||
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-
Comment 30•11 years ago
|
||
since diogo hasn't replied for long can i complete this bug as my first?
Updated•11 years ago
|
Assignee: diogo.gmt → nobody
Updated•11 years ago
|
Assignee: nobody → kunalbansal.02
Comment 31•11 years ago
|
||
i have made the changes as instructed.
Attachment #732164 -
Flags: review?(cpearce)
Comment 32•11 years ago
|
||
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?
Comment 33•11 years ago
|
||
Kunal, could you address Chris' review in comment 32?
Flags: needinfo?(kunalbansal.02)
Updated•11 years ago
|
Attachment #732164 -
Flags: review?(cpearce)
Updated•11 years ago
|
Assignee: kunalbansal.02 → nobody
Flags: needinfo?(kunalbansal.02)
Updated•11 years ago
|
Status: REOPENED → NEW
Comment 34•11 years ago
|
||
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.
Comment 35•11 years ago
|
||
Tareq, it sounds like both of the patches don't work. I'll mark them as obsolete.
Updated•11 years ago
|
Attachment #642470 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #732164 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Mentor: jaws
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][lang=js]
Comment 36•10 years ago
|
||
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.
Comment 37•9 years ago
|
||
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)
Comment 38•9 years ago
|
||
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.
Comment 39•9 years ago
|
||
The source file mentioned in comment 5 is also likely relevant here.
Comment 40•9 years ago
|
||
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)
Comment 41•9 years ago
|
||
Excellent, thanks for all of the info guys. I will jump on this and start taking a look at it.
Comment 42•8 years ago
|
||
Hi, I want to fix this bug can you guide me through
Comment 43•8 years ago
|
||
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;
Comment 44•8 years ago
|
||
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.
Updated•8 years ago
|
Mentor: jaws
Comment 45•8 years ago
|
||
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]
Comment 46•6 years ago
|
||
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
Comment 47•6 years ago
|
||
(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]
Comment 48•5 years ago
|
||
See bug 1547409. Migrating webcompat priority whiteboard tags to project flags.
Webcompat Priority: --- → P3
Updated•2 years ago
|
Severity: trivial → S4
You need to log in
before you can comment on or make changes to this bug.
Description
•