Last Comment Bug 805301 - Rename mozallowfullscreen to allowfullscreen
: Rename mozallowfullscreen to allowfullscreen
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on: 807360
Blocks: 545812
  Show dependency treegraph
 
Reported: 2012-10-24 18:42 PDT by Chris Pearce (:cpearce)
Modified: 2013-03-23 11:44 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed
fixed


Attachments
Patch: rename mozallowfullscreen to allowfullscreen (26.53 KB, patch)
2012-10-24 18:50 PDT, Chris Pearce (:cpearce)
justin.lebar+bug: review+
bzbarsky: feedback+
Details | Diff | Splinter Review
Patch: rename mozallowfullscreen to allowfullscreen (for aurora, without string change) (25.18 KB, patch)
2012-11-07 16:30 PST, Chris Pearce (:cpearce)
cpearce: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-10-24 18:42:17 PDT
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.
Comment 1 Chris Pearce (:cpearce) 2012-10-24 18:50:57 PDT
Created attachment 674933 [details] [diff] [review]
Patch: rename mozallowfullscreen to allowfullscreen

* 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 2 Justin Lebar (not reading bugmail) 2012-10-24 20:38:28 PDT
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 3 Justin Lebar (not reading bugmail) 2012-10-24 20:40:36 PDT
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.
Comment 4 Justin Lebar (not reading bugmail) 2012-10-24 20:41:30 PDT
>+++ 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 5 Chris Pearce (:cpearce) 2012-10-24 21:12:25 PDT
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 6 Boris Zbarsky [:bz] 2012-10-24 21:27:00 PDT
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?
Comment 7 Chris Pearce (:cpearce) 2012-10-24 21:37:36 PDT
(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.
Comment 8 Boris Zbarsky [:bz] 2012-10-24 21:41:38 PDT
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.  :(
Comment 9 Mounir Lamouri (:mounir) 2012-10-25 10:49:58 PDT
If we keep mozallowfullscreen around, do we have plan to remove it? We can't keep prefixed attributes ad vitam eternam.
Comment 10 Chris Pearce (:cpearce) 2012-10-25 13:08:08 PDT
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.
Comment 11 Mounir Lamouri (:mounir) 2012-10-25 15:59:34 PDT
Sounds great :)
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-28 06:48:15 PDT
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.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-10-30 08:27:44 PDT
https://hg.mozilla.org/mozilla-central/rev/9c5a898efbe9
Comment 15 Francesco Lodolo [:flod] 2012-10-31 00:57:34 PDT
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
Comment 16 Justin Lebar (not reading bugmail) 2012-10-31 09:38:54 PDT
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.
Comment 17 Francesco Lodolo [:flod] 2012-10-31 09:45:31 PDT
(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 18 Chris Pearce (:cpearce) 2012-11-04 18:13:04 PST
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.
Comment 19 bhavana bajaj [:bajaj] 2012-11-05 16:20:52 PST
(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.
Comment 20 Justin Lebar (not reading bugmail) 2012-11-05 16:28:36 PST
Do you mean that you'd approve this patch without the string change, or that you want this bug to ride the trains?
Comment 21 Chris Pearce (:cpearce) 2012-11-05 16:49:09 PST
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.
Comment 22 bhavana bajaj [:bajaj] 2012-11-05 16:52:54 PST
(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.
Comment 23 bhavana bajaj [:bajaj] 2012-11-05 16:54:46 PST
(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.
Comment 24 Justin Lebar (not reading bugmail) 2012-11-05 17:11:20 PST
> 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.
Comment 25 Chris Pearce (:cpearce) 2012-11-05 17:15:35 PST
(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.
Comment 26 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-05 17:23:50 PST
(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.
Comment 27 Justin Lebar (not reading bugmail) 2012-11-05 17:29:03 PST
> 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 28 Jonas Sicking (:sicking) PTO Until July 5th 2012-11-07 09:13:49 PST
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
Comment 29 Axel Hecht [:Pike] 2012-11-07 09:34:48 PST
Jonas, why do you approve a string freeze break against the previous agreement here in the bug? See comment 22.
Comment 30 Jonas Sicking (:sicking) PTO Until July 5th 2012-11-07 09:47:04 PST
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
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2012-11-07 09:47:27 PST
In any case this is not blocking.
Comment 32 Chris Pearce (:cpearce) 2012-11-07 16:30:52 PST
Created attachment 679444 [details] [diff] [review]
Patch: rename mozallowfullscreen to allowfullscreen (for aurora, without string change)

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.
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-11-08 13:48:41 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/5eb344f86efb

Note You need to log in before you can comment on or make changes to this bug.