1299795 - Video full screen cannot be toggled from the sidebar

Status: RESOLVED FIXED

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

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[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)

Comment 1

[Alice0775 White]

Attachment: Bug 1299795 fullscreen.html

Fullscreen api maybe fail in sidebar

Comment 2

[Kanchan Kumari QA]

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

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1299795 - Disable fullscreen for sidebar.

Review commit: https://reviewboard.mozilla.org/r/76726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76726/

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

With this patch, the website would know that fullscreen is not available and can notify user accordingly.

Comment 6

[Olli Pettay [:smaug]]

Could you explain why fullscreen doesn't work in sidebar?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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 8

[Olli Pettay [:smaug]]

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.

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Too late to make 49, as we are heading into the RC2 build tomorrow.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aabb19fcdfe
Disable fullscreen for sidebar. r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5aabb19fcdfe

Comment 12

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

On latest Nightly 51.0a1 (2016-09-14) the problem is still triggered by the "Full Screen" context menu option.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

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...

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Submitted patches for bug 1302976. Cancel ni? here.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to fix in 50.1.0 release