Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Algorithm of how fullscreen enabled flag is handled is not web-compatible #1481

Closed
upsuper opened this issue Jul 1, 2016 · 8 comments
Closed

Algorithm of how fullscreen enabled flag is handled is not web-compatible #1481

upsuper opened this issue Jul 1, 2016 · 8 comments
Labels
compat Standard is not web compatible or proprietary feature needs standardizing

Comments

@upsuper
Copy link
Member

upsuper commented Jul 1, 2016

Recently, I changed Gecko to only set fullscreen enabled flag when the document is loaded based on the algorithm in the HTML spec. But this seems to break some websites.

We received a bug report that videos opened in IMDB's homepage cannot go fullscreen after this change, and I suspect this would not be the only affected site.

It seems all other browsers have the same behavior as what Gecko had previously, which dynamically detect the presence of allowfullscreen attribute on the iframe element. I think the spec should probably reflect that behavior rather than binding fullscreen enabled flag with sandbox flags.

What I'm not sure is, what should we do with the "sandboxed fullscreen browsing context flag"? It seems no browser (except Gecko with the recent fix) follows the algorithm of that flag. Should we remove it to match implementations as well? Or do we think this wouldn't break web compability? Or probably we just don't care any compability issue caused by a stronger sandbox?

cc @annevk @bzbarsky @foolip
@upsuper
Copy link
Member Author

upsuper commented Jul 1, 2016

There is a testcase which shows the difference of behavior in that bug. And I worte a test in web-platform-tests for the current spec behavior when I did the change.

@annevk
Copy link
Member

annevk commented Jul 1, 2016

Is this basically #1240? If not, what is the difference?

@upsuper
Copy link
Member Author

upsuper commented Jul 1, 2016

No, they are different. #1240 is about updating sandbox flags for the browsing context, this is about the fullscreen enabled flag on document.

@annevk annevk mentioned this issue Jul 1, 2016
Closed
@upsuper
Copy link
Member Author

upsuper commented Jul 3, 2016

I think, the spec should be changed to that, allowfullscreen attribute should be detected dynamically when fullscreenEnabled is queried or requestFullscreen is called.

I guess it might be fine to leave sandbox thing as is currently. Probably we should still introduce an independent keyword to sandbox for fullscreen, so that other specs like CSP can use it.

@annevk
Copy link
Member

annevk commented Jul 4, 2016

You mean something like allow-fullscreen?

@upsuper
Copy link
Member Author

upsuper commented Jul 4, 2016

Yeah, I guess something like that, given there are outside reference to that property... It doesn't make sense that when sandbox from CSP is applied, fullscreen can never be allowed.

@annevk
Copy link
Member

annevk commented Jul 4, 2016

I guess that's a reasonable solution given that it was never implemented properly. It's a little sad that it works differently from how sandboxing does in the normal case, but I guess we have to live with that.

@annevk annevk added the compat Standard is not web compatible or proprietary feature needs standardizing label Jul 4, 2016
@mikewest
Copy link
Member

mikewest commented Jul 4, 2016

Blink does not implement a fullscreen sandbox flag (see https://blink.lc/chromium/tree/third_party/WebKit/Source/core/dom/SandboxFlags.h). Skimming the code, it looks like we're doing pure runtime checks when executing things that want to go fullscreen, which seems to match @upsuper's comments above. I don't have a strong opinion as to whether or not fullscreen ought to be controlled by the sandbox attribute (or the corresponding sandbox CSP directive). If we do add that in, we would need an allow-fullscreen token, and I guess require both that token and the allowfullscreen attribute? That seems complicated.

Given that it's already opt-in on the frame, it might be a better idea to just drop the sandbox flag entirely and leave things as a runtime check. shrug CCing @foolip who's doing work on fullscreen (I think?), and might have stronger opinions one way or the other.

annevk added a commit that referenced this issue Jul 4, 2016
@annevk
Make allowfullscreen and allowusermedia match implementations
adf25f5 
Fullscreen sandboxing was never implemented and it appears there are no
immediate plans for it by either Google or Mozilla. So let’s remove it.
Fixes #1240.

Furthermore, the way allowfullscreen works in implementations is by
dynamically checking the attribute. That requires moving to an “allowed
to use” algorithm rather than using a fullscreen enabled flag. (Aside,
the fullscreen enabled flag logic got broken in
688df43 but that no longer matters.)
Fixes #1481.

This change also aligns the recently introduced allowusermedia with
that model since it seems unlikely implementers want to do something
different there.
@annevk annevk mentioned this issue Jul 4, 2016
Merged
annevk added a commit that referenced this issue Jul 5, 2016
@annevk
Make allowfullscreen and allowusermedia match implementations
f6a4c1c 
Fullscreen sandboxing was never implemented and it appears there are no
immediate plans for it by either Google or Mozilla. So let’s remove it.
Fixes #1240.

This also removes sandboxing for getUserMedia() as that meant to follow
the example set by fullscreen. A feature that can be allowed in nested
browsing contexts, is not by default, and does not require sandboxing
to be enabled for it to be allowed.

Furthermore, the way allowfullscreen works in implementations is by
dynamically checking the attribute. That requires moving to an “allowed
to use” algorithm rather than using a fullscreen enabled flag. The
fullscreen enabled flag was determined upon creation of the document
and frozen afterwards. This new model allows setting the attribute at
any point. (Note that this is different from how sandboxing works,
which is frozen upon creation, but now they are no longer tied that
matters less.) Fixes #1481.

(Aside, the fullscreen enabled flag logic got broken in
688df43, but that no longer matters.)

This change also aligns the recently introduced allowusermedia with
that model since implementers likely want that to remain matching
allowfullscreen.
domenic pushed a commit that referenced this issue Jul 6, 2016
@annevk @domenic
Make allowfullscreen and allowusermedia match implementations
9f6b91c 
Fullscreen sandboxing was never implemented and it appears there are no
immediate plans for it by either Google or Mozilla. So let’s remove it.
Fixes #1240.

This also removes sandboxing for getUserMedia() as that meant to follow
the example set by fullscreen. A feature that can be allowed in nested
browsing contexts, is not by default, and does not require sandboxing
to be enabled for it to be allowed.

Furthermore, the way allowfullscreen works in implementations is by
dynamically checking the attribute. That requires moving to an “allowed
to use” algorithm rather than using a fullscreen enabled flag. The
fullscreen enabled flag was determined upon creation of the document
and frozen afterwards. This new model allows setting the attribute at
any point. (Note that this is different from how sandboxing works,
which is frozen upon creation, but now they are no longer tied that
matters less.) Fixes #1481.

(Aside, the fullscreen enabled flag logic got broken in
688df43, but that no longer matters.)

This change also aligns the recently introduced allowusermedia with
that model since implementers likely want that to remain matching
allowfullscreen.
foolip added a commit that referenced this issue Jul 7, 2016
@foolip
Editorial: drop reference to the user media enabled flag
b73d375 
This is unused since commit 9f6b91c.

Follow-up to #1481.
@foolip foolip mentioned this issue Jul 7, 2016
Merged
annevk pushed a commit that referenced this issue Jul 7, 2016
@foolip @annevk
Editorial: drop reference to the user media enabled flag
37b83e9 
This is unused since commit 9f6b91c.

Follow-up to #1481.
@travisleithead travisleithead mentioned this issue Oct 19, 2016
Closed
@foolip foolip mentioned this issue Dec 15, 2016
Closed
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
@annevk
Make allowfullscreen and allowusermedia match implementations
d44dfe2 
Fullscreen sandboxing was never implemented and it appears there are no
immediate plans for it by either Google or Mozilla. So let’s remove it.
Fixes whatwg#1240.

This also removes sandboxing for getUserMedia() as that meant to follow
the example set by fullscreen. A feature that can be allowed in nested
browsing contexts, is not by default, and does not require sandboxing
to be enabled for it to be allowed.

Furthermore, the way allowfullscreen works in implementations is by
dynamically checking the attribute. That requires moving to an “allowed
to use” algorithm rather than using a fullscreen enabled flag. The
fullscreen enabled flag was determined upon creation of the document
and frozen afterwards. This new model allows setting the attribute at
any point. (Note that this is different from how sandboxing works,
which is frozen upon creation, but now they are no longer tied that
matters less.) Fixes whatwg#1481.

(Aside, the fullscreen enabled flag logic got broken in
688df43, but that no longer matters.)

This change also aligns the recently introduced allowusermedia with
that model since implementers likely want that to remain matching
allowfullscreen.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
@foolip
Editorial: drop reference to the user media enabled flag
0cc6d9f 
This is unused since commit 9f6b91c.

Follow-up to whatwg#1481.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compat Standard is not web compatible or proprietary feature needs standardizing
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mikewest @upsuper @annevk