1212299 - Forbid documents inside <frame> and <object> from requesting fullscreen

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

According to the spec, only documents inside <iframe> should be able to request fullscreen, because a document which is neither topmost nor inside <iframe> should never have the "fullscreen enabled flag" set.

For us, we should still allow <xul:browser>, but other elements which can include nested document should be banned.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1212299 part 1 - Forbid documents inside elements other than iframe from requesting fullscreen.

Bug 1212299 part 1 - Forbid documents inside elements other than iframe from requesting fullscreen.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1212299 part 2 - Rewrite fullscreen-denied test to have a clearer structure.

Bug 1212299 part 2 - Rewrite fullscreen-denied test to have a clearer structure.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1212299 part 3 - Add test for requesting fullscreen from doc inside frame/object.

Bug 1212299 part 3 - Add test for requesting fullscreen from doc inside frame/object.

Comment 4

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

Comment on attachment 8671329 [details]
MozReview Request: Bug 1212299 part 2 - Rewrite fullscreen-denied test to have a clearer structure.

We really should unprefix the API.

Comment 5

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

Comment on attachment 8671330 [details]
MozReview Request: Bug 1212299 part 3 - Add test for requesting fullscreen from doc inside frame/object.

rs+

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8671329 [details]
> MozReview Request: Bug 1212299 part 2 - Rewrite fullscreen-denied test to
> have a clearer structure.
> 
> We really should unprefix the API.

Yes, see dependencies of bug 743198. I'm going to unprefix the API when we have a reasonably up-to-date impl.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5265f27a0b92aa1c82ff5f9eb3d212991438c51
Bug 1212299 part 1 - Forbid documents inside elements other than iframe from requesting fullscreen. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd171d5bcb5c7dffe5c36bfa17b509fe3fb295a
Bug 1212299 part 2 - Rewrite fullscreen-denied test to have a clearer structure. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a578edd91140c62b6c1a555f0acf8fd7ee28536
Bug 1212299 part 3 - Add test for requesting fullscreen from doc inside frame/object. rs=smaug

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5265f27a0b9
https://hg.mozilla.org/mozilla-central/rev/3bd171d5bcb5
https://hg.mozilla.org/mozilla-central/rev/9a578edd9114

Comment 9

[Kohei]

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/documents-in-frame-or-object-can-no-longer-request-fullscreen/

Comment 10

[Ton [:Tonnes]]

(In reply to Carsten Book [:Tomcat] from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/d5265f27a0b9
> 
> +++ b/dom/locales/en-US/chrome/dom/dom.properties
> -FullScreenDeniedIframeNotAllowed=Request for full-screen was denied because at least one of the document's containing iframes does not have an "allowfullscreen" attribute.
> +FullScreenDeniedContainerNotAllowed=Request for full-screen was denied because at least one of the document's containing element is not iframe or does not have an "allowfullscreen" attribute.
> 
> +++ b/webapprt/locales/en-US/webapprt/overrides/dom.properties
> -FullScreenDeniedIframeNotAllowed=Request for full-screen was denied because at least one of the document's containing iframes does not have an "allowfullscreen" attribute.
> +FullScreenDeniedContainerNotAllowed=Request for full-screen was denied because at least one of the document's containing element is not iframe or does not have an "allowfullscreen" attribute.

I noticed this in both dom.properties files during l10n:
"… one of the containing element is not iframe"

Shouldn’t this be
"… one of the containing elements is not an iframe"?

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5d3e1ee51053e15337f5fa363098a3f606adb8
Bug 1212299 followup - Fix minor grammar issue in locale text. DONTBUILD

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: followup locale text fix

Approval Request Comment
[Feature/regressing bug #]: this bug
[User impact if declined]: may see warning with grammar mistakes
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: there's a risk that the sentences are still wrong :)
[String/UUID change made/needed]: string changes

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c5d3e1ee510

Comment 14

[Francesco Lodolo [:flod]]

Personally I'd be in favor of this riding the trains with 45: it's going to create unnecessary noise in a string frozen repository, and I'm not convinced it's actually improving clarity (and for that it would need a new string ID).

> Request for full-screen was denied because at least one of the document's 
> containing elements is not an iframe or does not have an "allowfullscreen" attribute.

e.g. is it correct for an object to have multiple containers? I think it would be a lot easier to follow as "… one of the elements containing the document is not…".

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Francesco Lodolo [:flod] from comment #14)
> Personally I'd be in favor of this riding the trains with 45: it's going to
> create unnecessary noise in a string frozen repository, and I'm not
> convinced it's actually improving clarity (and for that it would need a new
> string ID).

I'm not quite familiar with the rules there, and just wanted to try and see what would happen. If that would add much trouble, you can a- this request. I'm fine either way.

> > Request for full-screen was denied because at least one of the document's 
> > containing elements is not an iframe or does not have an "allowfullscreen" attribute.
> 
> e.g. is it correct for an object to have multiple containers? I think it
> would be a lot easier to follow as "… one of the elements containing the
> document is not…".

There could be nested container, e.g. a document inside an <iframe> whose document is inside a <frame>.

Comment 16

[Francesco Lodolo [:flod]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> I'm not quite familiar with the rules there, and just wanted to try and see
> what would happen. If that would add much trouble, you can a- this request.
> I'm fine either way.

There are 2 basic rules:
* All branches besides mozilla-central are string frozen. Release drivers are in charge of approving such changes, and that's why you need to specify if there string changes.
* You shouldn't change existing strings without using a new ID
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

In case of doubt, ask before landing. The change you already landed on m-c is kind of border-line, but I would have been better to double check.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Well, I think this fix should be fine because the meaning is effectively unchanged, just a grammar fix :)

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688824 [details] [diff] [review]
followup locale text fix

As Flod pointed out, we don't accept string changes during Aurora/Beta cycles unless there is a strong marketing/PR/end-user impact/justification. Please let this one ride the train.

Comment 19

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullScreen
and
https://developer.mozilla.org/en-US/Firefox/Releases/44#Miscellaneous