Closed Bug 1299795 Opened 3 years ago Closed 3 years ago

Video full screen cannot be toggled from the sidebar

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: JuliaC, Assigned: xidorn)

References

Details

(Keywords: regression)

Attachments

(2 files)

[Affected versions]:
- latest Nightly 51.0a1 (2016-08-31)
- latest Aurora 50.0a2 (2016-08-31)
- 49.0b8 build1 (20160829102229)
- 48.0.2 build1 (20160823121617)

[Affected platforms]:
- Windows 10 x64
- Mac OS X 10.10.5
- Ubuntu 14.04 x86 

[Steps to reproduce]:
1. Launch Firefox
2. Open a popular video website (e.g. https://www.youtube.com/ or https://vimeo.com/) and choose a random video
3. Bookmark the chosen video
4. Open the Bookmarks Menu, look for the previous created bookmark and open its Properties (context menu option)
5. Check the "Load this bookmark in the sidebar" option and click the "Save" button
6. Open again the Bookmarks Menu and click the bookmark in question
- the chosen video is loaded in the sidebar
7. Toggle full screen for this video (click the full screen player specific button / double click on video / choose "Full Screen" context menu option)
- inspect the Browser Console

[Expected result]:
- The user is able to enter full screen; a notification stating that this action is not possible from the sidebar is displayed otherwise

[Actual result]:
- The user isn't able to enter full screen for the chosen video from the sidebar
- [TelemetryStopwatch: key "FULLSCREEN_CHANGE_MS" was already initialized] error is thrown in Browser Console

[Regression range]:
- I will investigate this as soon as possible

[Additional notes]:
- The issue is reproducible with other video formats, too (e.g. http://techslides.com/demos/sample-videos/small.mp4 or http://techslides.com/demos/sample-videos/small.webm)
Fullscreen api maybe fail in sidebar
Component: Audio/Video: Playback → DOM
Flags: needinfo?(xidorn+moz)
regression range:
Last good revision: cef590a6f946 (2014-11-27)
First bad revision: 17de0f463944 (2014-11-28)

Push log
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cef590a6f946&tochange=17de0f463944
I guess we should probably just ban fullscreen request in sidebar. I might miss something, but it doesn't seem to me supporting entering fullscreen from sidebar is something very useful.

The browser console error is just a side effect of the issue that entering fullscreen fails. That is not the root reason.
With this patch, the website would know that fullscreen is not available and can notify user accordingly.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Could you explain why fullscreen doesn't work in sidebar?
My guess is that would at least fail in the check of whether the browser requesting fullscreen is the current selected browser [1]. This check is essential in case that the user switches the tab at a good time. I guess it is possible to make it also check whether it is from an open sidebar, but that may make things more complicated, e.g. if the current selected tab also requests fullscreen, then we may mess the fullscreen state of the window.

[1] https://dxr.mozilla.org/mozilla-central/rev/dbe4b47941c7b3d6298a0ead5e40dd828096c808/browser/base/content/browser-fullScreenAndPointerLock.js#417
Comment on attachment 8788137 [details]
Bug 1299795 - Disable fullscreen for sidebar.

https://reviewboard.mozilla.org/r/76726/#review75178

I guess we can take this for now, so that we don't even try to enter fullscreen from sidebar. But please file a followup to support fullscreen in sidebar, since that is totally valid use case as long as we support opening bookmarks in sidebars. And mention this bug number in the new bug.
Attachment #8788137 - Flags: review?(bugs) → review+
Too late to make 49, as we are heading into the RC2 build tomorrow.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aabb19fcdfe
Disable fullscreen for sidebar. r=smaug
See Also: → 1300935
https://hg.mozilla.org/mozilla-central/rev/5aabb19fcdfe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
On latest Nightly 51.0a1 (2016-09-14) the problem is still triggered by the "Full Screen" context menu option.
Flags: needinfo?(xidorn+moz)
Ah, "Full Screen" context menu triggers fullscreen via chrome code...

I have no idea then. Probably we should not give chrome code priviledge on fullscreen. It seems Chrome doesn't allow user putting a video into fullscreen if its document is not allowed to do that. Perhaps we can do the same here...
See Also: → 1302976
Submitted patches for bug 1302976. Cancel ni? here.
Flags: needinfo?(xidorn+moz)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.