1283526 - The "Full screen" button is missing when playing videos in pop-up from www.imdb.com homepage

Status: VERIFIED FIXED

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

Description

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Attachment: screencast of the issue.mp4

[Affected versions]:
- Nightly 50.0a1

[Affected Platforms]:
- All Windows
- All Linux
- All Mac OS

[Steps to reproduce]:
1. Navigate to http://www.imdb.com/.
2. Click on the "Play" button for any video from the homepage.
3. In the new opened pop-up window click the "Full screen" button.

[Expected result]:
- The video starts playing in full screen mode.

[Actual result]:
- The "Full screen" button is missing.

[Regression]:
Last good revision: de61823b8e8adcc3c1a78165ec56956dbb8fd26c
First bad revision: 975596fc6440322edb04628995c7154841b22119
Pushlog: https://goo.gl/VIJ88R

[Notes]:
- This only happens for videos in pop-up. If the same video is opened in a separate tab, the "Full screen" button is correctly displayed.
- This issue is not reproducible on the Firefox 47.0.1 release.
- Added a screen recording with the issue.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1270648

Xidorn, can you please look over this?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I guess this is because "allowfullscreen" attribute is added after the document inside <iframe> is loaded.

In old Firefox and all other browsers, whether fullscreen is enabled is checked dynamically whether "allowfullscreen" attribute presents.

However, in bug 1270648, we change to conform the spec that the presence of that attribute is recorded in another variable when document is loaded, so if the attribute does not present when the document is loaded, the document would never be allowed to enter fullscreen.

This may indicate that this change can break the web, so we probably want to revert to the old behavior, and have spec changed accordingly.

What I'm not sure is the behavior of sandbox. It seems other browsers do not follow the sandbox algorithm for fullscreen either. I wonder whether we want to remove fullscreen sandbox as well, or we should just revert the behavior for non-sandboxed iframe.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

This is a testcase shows difference of how fullscreen enabled flag is handled.

On Firefox 49+, it shows:
> iframe fullscreenEnabled: false
> add allowfullscreen
> iframe fullscreenEnabled: false
> remove allowfullscreen
> iframe fullscreenEnabled: false
while in all other browsers and old Firefox, it shows:
> iframe fullscreenEnabled: false
> add allowfullscreen
> iframe fullscreenEnabled: true
> remove allowfullscreen
> iframe fullscreenEnabled: false

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Filed an issue on whatwg/html: https://github.com/whatwg/html/issues/1481

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Sounds like we must change our behavior back to what it was and fix the spec.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1283526 part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

It seems it tends to drop the fullscreen sandbox flag as well. I'll write another patch to drop that tomorrow, so the current patch is ready to review.

(I'm not willing to backout the patches directly, because there are some changes in bug 1270648 I think worth keeping.)

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1283526 part 2 - Remove fullscreen sandbox flag.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8767530 [details]
Bug 1283526 part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61976/diff/1-2/

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8767530 [details]
Bug 1283526 part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe.

https://reviewboard.mozilla.org/r/61976/#review59998

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8767791 [details]
Bug 1283526 part 2 - Remove fullscreen sandbox flag.

https://reviewboard.mozilla.org/r/62204/#review60000

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04b34bdb4715
part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe. r=smaug
https://hg.mozilla.org/integration/autoland/rev/64e752c0270c
part 2 - Remove fullscreen sandbox flag. r=smaug

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8767530 [details]
Bug 1283526 part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe.

Approval Request Comment
[Feature/regressing bug #]: bug 1270648
[User impact if declined]: some websites may not be able to enter fullscreen in certain condition
[Describe test coverage new/current, TreeHerder]: corresponding test has been altered
[Risks and why]: mainly just revert the change of bug 1270648, so not very risky
[String/UUID change made/needed]: n/a

This request is for both patches in this bug.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/04b34bdb4715
https://hg.mozilla.org/mozilla-central/rev/64e752c0270c

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8767530 [details]
Bug 1283526 part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe.

Revert changes from 49 for fullscreen with iframes, please uplift to aurora.

Comment 16

[Carsten Book [:Tomcat]]

does not apply cleanly to aurora:

grafting 353377:64e752c0270c "Bug 1283526 part 2 - Remove fullscreen sandbox flag. r=smaug"
merging docshell/base/nsDocShell.cpp
merging dom/base/nsSandboxFlags.h
merging dom/html/HTMLIFrameElement.cpp
warning: conflicts while merging dom/html/HTMLIFrameElement.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8bff4f92a6d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/18f1606b05b4

Comment 18

[Landry Breuil (:gaston)]

Congrats, this broke aurora on openbsd where there's no sandbox:
http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/1758/steps/build/logs/stdio

15:34.02 In file included from /home/obj/buildslave/mozilla-aurora/dom/html/Unified_cpp_dom_html1.cpp:56:
15:34.02 /home/dusk/home/buildslave-amd64/mozilla-aurora-amd64/build/dom/html/HTMLIFrameElement.cpp:243:26: error: no member named 'ParseSandboxAttributeToFlags' in 'nsContentUtils'
15:34.02   return nsContentUtils::ParseSandboxAttributeToFlags(sandboxAttr);
15:34.02          ~~~~~~~~~~~~~~~~^

Unless a header/idl file wasnt regenerated.... previous build with rev a88bc35e32b1334b0aac964d53ae36b1e4600b7d (http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/1757) was fine.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

There are two followups landed in aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/25b896fe818d
https://hg.mozilla.org/releases/mozilla-aurora/rev/0eda20ce8149

Comment 20

[Landry Breuil (:gaston)]

Yeah, that was probably it, today's aurora build was fine: http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/1759

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

That was because of that... My commits in comment 17 actually broke on all platforms in our CI system as well :/

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced with 2016-06-30 Nightly on Windows 10x64 using the STR provided in the description. 

Confirming the fix on Windows 10x64, Ubuntu 12.04x86 and Mac OS X 10.9.5.
Tested with Firefox 49.0b6 (build ID 20160822111414) and latest 50.0a2 (build ID 20160823004001).