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

VERIFIED FIXED in Firefox 49

Status

()

Core
DOM
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: CosminMCG, Assigned: xidorn)

Tracking

({regression})

Trunk
mozilla50
x86_64
All
regression
Points:
---

Firefox Tracking Flags

(firefox49 verified, firefox50 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Created attachment 8766811 [details]
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.
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?
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
Created attachment 8767034 [details]
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
Filed an issue on whatwg/html: https://github.com/whatwg/html/issues/1481
Sounds like we must change our behavior back to what it was and fix the spec.
Created attachment 8767530 [details]
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/
Attachment #8767530 - Flags: review?(bugs)
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.)
Created attachment 8767791 [details]
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/
Attachment #8767530 - Attachment description: Bug 1283526 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe. → Bug 1283526 part 1 - Revert behavior of allowfullscreen attribute for non-sandboxed iframe.
Attachment #8767791 - Flags: review?(bugs)
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/
Assignee: nobody → xidorn+moz
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
Attachment #8767530 - Flags: review?(bugs) → review+
Comment on attachment 8767791 [details]
Bug 1283526 part 2 - Remove fullscreen sandbox flag.

https://reviewboard.mozilla.org/r/62204/#review60000
Attachment #8767791 - Flags: review?(bugs) → review+

Comment 12

2 years ago
Pushed by xquan@mozilla.com:
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 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.
Attachment #8767530 - Flags: approval-mozilla-aurora?

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04b34bdb4715
https://hg.mozilla.org/mozilla-central/rev/64e752c0270c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Attachment #8767530 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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')
Flags: needinfo?(xidorn+moz)
https://hg.mozilla.org/releases/mozilla-aurora/rev/8bff4f92a6d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/18f1606b05b4
status-firefox49: --- → fixed
Flags: needinfo?(xidorn+moz)
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.
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
Yeah, that was probably it, today's aurora build was fine: http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/1759
That was because of that... My commits in comment 17 actually broke on all platforms in our CI system as well :/
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.