YouTube's iframe embeds use HTML5 <video> if the user opted into the HTML5 trial, but the iframes have an "allowfullscreen" attribute HTML5 not a "mozallowfullscreen" attribute. This means these iframes can't go fullscreen. I think we should transition to support "allowfullscreen", but we should support "mozallowfullscreen" in the meantime to so we don't break existing users.

* Rename iframe.mozallowfullscren to allowfullscreen. * Add checks for mozallowfullscren everywhere we check for allowfullscren, i.e. support both. I think we should get this on B2G/Aurora, so that we support fullscreen in YouTube embeds on B2G (we can probably drop the string change if we need to in order to get Aurora approval, since we still support both mozallowfullscreen and allowfullscreen, thus the original string before the change isn't *wrong*).

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen I'm happy to review this code, but I'd like someone more tuned into standards than I to review the decision to start supporting |allowfullscreen| and to continue supporting |mozallowfullscreen|, because I don't have an informed opinion about that. Do you have anyone in mind who can sign off on that call, Chris? Otherwise we can just ask Boris or Jonas..

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen >diff --git a/dom/locales/en-US/chrome/dom/dom.properties b/dom/locales/en-US/chrome/dom/dom.properties >--- a/dom/locales/en-US/chrome/dom/dom.properties >+++ b/dom/locales/en-US/chrome/dom/dom.properties >@@ -67,17 +67,17 @@ nsIJSONEncodeDeprecatedWarning=nsIJSON.e > nsIDOMWindowInternalWarning=Use of nsIDOMWindowInternal is deprecated. Use nsIDOMWindow instead. > InputEncodingWarning=Use of inputEncoding is deprecated. > # LOCALIZATION NOTE: Do not translate "MozBeforePaint" and "mozRequestAnimationFrame" > MozBeforePaintWarning=MozBeforePaint events are no longer supported. mozRequestAnimationFrame must be passed a non-null callback argument. > FullScreenDeniedBlocked=Request for full-screen was denied because this domain has been blocked from full-screen by user. > FullScreenDeniedDisabled=Request for full-screen was denied because full-screen API is disabled by user preference. > FullScreenDeniedFocusedPlugin=Request for full-screen was denied because a windowed plugin is focused. > FullScreenDeniedHidden=Request for full-screen was denied because the document is no longer visible. >-FullScreenDeniedIframeDisallowed=Request for full-screen was denied because at least one of the document's containing iframes does not have a "mozallowfullscreen" attribute. >+FullScreenDeniedIframeDisallowed=Request for full-screen was denied because at least one of the document's containing iframes does not have a "allowfullscreen" attribute. an r=me, but leaving an empty f? for the decision from the previous comment.

>+++ b/dom/tests/mochitest/pointerlock/test_pointerlock-api.html >@@ -15,17 +15,17 @@ https://bugzilla.mozilla.org/show_bug.cg > </a> > <div id="content"> > </div> > <pre id="test"> > <script type="application/javascript"> > /** > * Pointer Lock tests for bug 633602. These depend on the fullscreen api > * which doesn't work when run in the mochitests' iframe, since the >- * mochitests' iframe doesn't have a mozallowfullscreen attribute. To get >+ * mochitests' iframe doesn't have a allowfullscreen attribute. To get oops, another "an".

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen Thanks jlebar, I'll seek maximum cross pollination and ask both Boris and Jonas. Boris/Jonas, what do you guys think about dropping the prefix on iframe.mozallowfullscreen, while still being backwards compatible with the prefixed attribute, i.e. allow fullscreen inside an iframe that has either a mozallowfullscreen or allowfullscreen attribute? We want to do this because YouTube embed iframes have an "allowfullscreen" attribute but not a "mozallowfullscreen" (or a "webkitallowfullscreen") attribute, and we'd like YouTube embeds to be able to go fullscreen, particularly on B2G.

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen This seems fine to me if the spec for this is at all stable. One problem (preexisting), though: if a document is inside a <frame> (not <iframe>) it looks like we'll allow fullscreen even if there are no attributes set on the element. Same for documents inside <object>... That seems odd to me. Is it intended?

(In reply to Boris Zbarsky (:bz) from comment #6) > Comment on attachment 674933 [details] [diff] [review] > Patch: rename mozallowfullscreen to allowfullscreen > > This seems fine to me if the spec for this is at all stable. The spec for allowfullscreen is not contentious, pretty stable. > One problem (preexisting), though: if a document is inside a <frame> (not > <iframe>) it looks like we'll allow fullscreen even if there are no > attributes set on the element. Same for documents inside <object>... That > seems odd to me. Is it intended? Not intentional. However the draft spec explicitly mentions <iframe> as opposed to <frame>, so I'll raise the issue on public-webapps.

The spec sounds to me like things should only be able to go fullscreen if they're in an <iframe fullscreen>, so non-iframe things should just be blocked... But it's certainly pretty vague on details of the processing model there. :(

If we keep mozallowfullscreen around, do we have plan to remove it? We can't keep prefixed attributes ad vitam eternam.

I suggest we remove support for mozallowfullscreen when we remove the moz* prefixed methods of the fullscreen API. Not sure when that'll be, the spec is still in flux.

Sounds great :)

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen Review of attachment 674933 [details] [diff] [review]: ----------------------------------------------------------------- I haven't really been following the specs in this area, so I'm happy to rely on bz's and Chris' call here. I wasn't even aware that the allowfullscreen attribute existed. Reading the spec for it it really needs more rigor and normative text. For example, what happens for top.html: <iframe src=a.html allowfullscreen> <iframe src=b.html> <video/> <script>document.querySelector("video").requestFullscreen();</script> But getting the spec up to snuff doesn't block this bug as long as there's general agreement what should happen.

This patch shouldn't have passed review. http://hg.mozilla.org/mozilla-central/diff/9c5a898efbe9/dom/locales/en-US/chrome/dom/dom.properties If you change a string like this, you're expected to change the key name as well

That's clearly our mistake; I'm sorry. But we don't re-open bugs when there's a mistake (at least in core/gecko; I can't speak for other components). Can you please close this bug and file a new bug blocking this one? Thanks.

(In reply to Justin Lebar [:jlebar] from comment #16) > But we don't re-open bugs when there's a mistake (at least in core/gecko; I > can't speak for other components). Can you please close this bug and file a > new bug blocking this one? Sure. I tried this way because I realized that these comments are usually ignored until things break apart...

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen [Approval Request Comment] This patch allows fullscreen inside an <iframe> element if either the "allowfullscreen" or "mozallowfullscreen" attribute is present on the <iframe> element. Without this patch we only allow content inside an <iframe> to request fullscreen if the "mozallowfullscreen" attribute is present on the <iframe>. Bug caused by (feature/regressing bug #): Fullscreen, behaviour change, not a regression. User impact if declined: YouTube embeds <iframe>s won't be able to enter fullscreen on B2G v1. YouTube is generating embed HTML code with <iframe>s with a "allowfullscreen" attribute rather than a "mozallowfullscreen" attribute. If you want YouTube embed HTML5 videos to be able to enter fullscreen, you need to approve this patch for Aurora. Testing completed (on m-c, etc.): It's been on m-c for about a week. Risk to taking this patch (and alternatives if risky): Seems pretty low risk, it's backwards compatible with existing behaviour. String or UUID changes made by this patch: Minor string change in dom/locales/en-US/chrome/dom/dom.properties. This string is only used in logging to the web console, so given that the web console isn't really visible in B2G, and given that this patch is backwards compatible with existing behaviour, I don't think it's a big deal if we drop the string change if that's required for this to get Aurora approval. Note: I forgot to change the string ID when I updated the string, so if we take the string change, we'll also need Aurora approval for the patch to change the string name in bug 807360.

(In reply to Chris Pearce (:cpearce) from comment #18) > Comment on attachment 674933 [details] [diff] [review] > Patch: rename mozallowfullscreen to allowfullscreen > > [Approval Request Comment] > > This patch allows fullscreen inside an <iframe> element if either the > "allowfullscreen" or "mozallowfullscreen" attribute is present on the > <iframe> element. Without this patch we only allow content inside an > <iframe> to request fullscreen if the "mozallowfullscreen" attribute is > present on the <iframe>. > > Bug caused by (feature/regressing bug #): Fullscreen, behaviour change, not > a regression. > > User impact if declined: YouTube embeds <iframe>s won't be able to enter > fullscreen on B2G v1. YouTube is generating embed HTML code with <iframe>s > with a "allowfullscreen" attribute rather than a "mozallowfullscreen" > attribute. > > If you want YouTube embed HTML5 videos to be able to enter fullscreen, you > need to approve this patch for Aurora. > > Testing completed (on m-c, etc.): It's been on m-c for about a week. > > Risk to taking this patch (and alternatives if risky): Seems pretty low > risk, it's backwards compatible with existing behaviour. > > String or UUID changes made by this patch: Minor string change in > dom/locales/en-US/chrome/dom/dom.properties. This string is only used in > logging to the web console, so given that the web console isn't really > visible in B2G, and given that this patch is backwards compatible with > existing behaviour, I don't think it's a big deal if we drop the string > change if that's required for this to get Aurora approval. > > Note: I forgot to change the string ID when I updated the string, so if we > take the string change, we'll also need Aurora approval for the patch to > change the string name in bug 807360. I understand this is needed for b2g but do not see "blocking-basecamp : + " set on this which makes me think if we really need this on aurora at this time ?Considering string changes have pinched us in the past, I would prefer this ride the trains.

Do you mean that you'd approve this patch without the string change, or that you want this bug to ride the trains?

I tried to make it clear in my approval request, but just to re-iterate: the string change isn't strictly necessary, so I'm happy to land without the string change on Aurora.

(In reply to Justin Lebar [:jlebar] from comment #20) > Do you mean that you'd approve this patch without the string change, or that > you want this bug to ride the trains? I am comfortable to take this patch on aurora(considering the risk assessment in comment 18 and user impact) without any string changes but the reason i suggested this ride the trains earlier was because it's not apparent that this is needed for uplift.Apologies for not outlining that clearly.

(In reply to Chris Pearce (:cpearce) from comment #21) > I tried to make it clear in my approval request, but just to re-iterate: the > string change isn't strictly necessary, so I'm happy to land without the > string change on Aurora. my bad ! please check my latest comment (#22) on getting this approved for aurora.

> the reason i suggested this ride the trains earlier was because it's not apparent > that this is needed for uplift. Please don't use the lack of blocking-basecamp+ when judging whether a bug is necessary for uplift. As you can see, this bug was never even nom'ed for b-b. You should be able to triage the importance of this bug when considering for mozilla-aurora uplift even if the bug has not already gone through b-b triage.

(In reply to bhavana bajaj [:bajaj] from comment #22) > it's not apparent that this is needed for uplift.Apologies for not outlining that > clearly. If we don't take this on Aurora, YouTube videos embedded in web pages won't be able to go fullscreen in B2G v1 release.

(In reply to Justin Lebar [:jlebar] from comment #24) > Please don't use the lack of blocking-basecamp+ when judging whether a bug > is necessary for uplift. As you can see, this bug was never even nom'ed for > b-b. You should be able to triage the importance of this bug when > considering for mozilla-aurora uplift even if the bug has not already gone > through b-b triage. We did look at the importance of this bug, and evaluated it to be quite low as the user wins listed in the nom state only B2G affect, but it's not blocking-basecamp (suggesting not required for v1.0). It was also never nominated for release tracking and we could certainly ship Firefox without this uplift as it is not a regression. We've had website regressions in the past from unprefixing and wanted to be more cautious here. If this is indeed low risk and can be landed without a string change then, as Bhavana said, we'll take this uplift. Just because something is low risk doesn't mean we have to uplift it, we like to know what the user-facing wins are too.

> but it's not blocking-basecamp (suggesting not required for v1.0). This is the miscommunication here about process. The fact that this bug is not currently blocking basecamp does not indicate that it's not important for B2G; this bug was simply never triaged for b-b. Do I need to get blocking-basecamp for bugs before I can request Aurora approval, or are you able to consider a bug's importance for B2G at the same time as you consider whether to accept it for Aurora? I thought it was basically the same set of people making both decisions.

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen Assuming this is super-safe for non-B2G releases, i.e. assuming that this won't see website breakage or otherwise cause regressions, we should take this. Don't forget to flag as fixed on the 18-train once this lands

Jonas, why do you approve a string freeze break against the previous agreement here in the bug? See comment 22.

Comment on attachment 674933 [details] [diff] [review] Patch: rename mozallowfullscreen to allowfullscreen Sorry, I missed that this had string changes. Punting this back to the normal triage team since I don't know exactly what policies we have for that. However it sounds like it'd be better to not change the string here and instead just land the string fix on m-c

In any case this is not blocking.

This patch is against aurora and doesn't have the string change. [Approval Request Comment] This patch allows fullscreen inside an <iframe> element if either the "allowfullscreen" or "mozallowfullscreen" attribute is present on the <iframe> element. Without this patch we only allow content inside an <iframe> to request fullscreen if the "mozallowfullscreen" attribute is present on the <iframe>. Bug caused by (feature/regressing bug #): Fullscreen User impact if declined: YouTube embeds <iframe>s won't be able to enter fullscreen on B2G v1. YouTube is generating embed HTML code with <iframe>s with a "allowfullscreen" attribute rather than a "mozallowfullscreen" attribute. Testing completed (on m-c, etc.): All changes in this patch have been on m-c for a week or so. Risk to taking this patch (and alternatives if risky): Low risk. String or UUID changes made by this patch: No string changes, nsIDOMHTMLIFrameElement's uuid is changed.

Documented: https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers#HTML https://developer.mozilla.org/en-US/docs/HTML/Element/iframe#attr-allowfullscreen