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

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Bug 1212299 part 1 - Forbid documents inside elements other than iframe from requesting fullscreen.
Attachment #8671328 - Flags: review?(bugs)
(Assignee)

Comment 2

4 years ago
Bug 1212299 part 2 - Rewrite fullscreen-denied test to have a clearer structure.
Attachment #8671329 - Flags: review?(bugs)
(Assignee)

Comment 3

4 years ago
Bug 1212299 part 3 - Add test for requesting fullscreen from doc inside frame/object.
Attachment #8671330 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Assignee: nobody → quanxunzhen
Attachment #8671328 - Flags: review?(bugs) → review+
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.
Attachment #8671329 - Flags: review?(bugs) → review+
Comment on attachment 8671330 [details]
MozReview Request: Bug 1212299 part 3 - Add test for requesting fullscreen from doc inside frame/object.

rs+
Attachment #8671330 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
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 10

4 years ago
(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"?
(Assignee)

Comment 12

4 years ago
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
Attachment #8688824 - Flags: approval-mozilla-aurora?
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…".
(Assignee)

Comment 15

4 years ago
(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>.
(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.
(Assignee)

Comment 17

4 years ago
Well, I think this fix should be fine because the meaning is effectively unchanged, just a grammar fix :)
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.
Attachment #8688824 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.