Closed
Bug 1283526
Opened 8 years ago
Closed 8 years ago
The "Full screen" button is missing when playing videos in pop-up from www.imdb.com homepage
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla50
People
(Reporter: cmuntean, Assigned: xidorn)
References
Details
(Keywords: regression)
Attachments
(4 files)
[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.
Reporter | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
Filed an issue on whatwg/html: https://github.com/whatwg/html/issues/1481
See Also: → https://github.com/whatwg/html/issues/1481
Comment 5•8 years ago
|
||
Sounds like we must change our behavior back to what it was and fix the spec.
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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.)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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•8 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
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/04b34bdb4715 https://hg.mozilla.org/mozilla-central/rev/64e752c0270c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 15•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
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•8 years ago
|
||
Yeah, that was probably it, today's aurora build was fine: http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/1759
Assignee | ||
Comment 21•8 years ago
|
||
That was because of that... My commits in comment 17 actually broke on all platforms in our CI system as well :/
Updated•8 years ago
|
Flags: qe-verify+
Comment 22•8 years ago
|
||
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
Flags: qe-verify+
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•