This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 26568 - Nothing stopping mis-nested fullscreen in iframes?
Summary: Nothing stopping mis-nested fullscreen in iframes?
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: Fullscreen (show other bugs)
Version: unspecified
Hardware: PC Linux
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-13 10:55 UTC by Philip Jägenstedt
Modified: 2014-08-15 11:40 UTC (History)
2 users (show)

See Also:


Attachments

Description Philip Jägenstedt 2014-08-13 10:55:28 UTC
Let's say we have two sibling iframes, iframe1 and iframe2, and that they're same-origin. First enter fullscreen with an element inside iframe1. Then request fullscreen for an element inside iframe2. Since iframe2's document's fullscreen element stack is empty, it seems like nothing's stopping it from going fullscreen.

You will end up with a cross-document set of fullscreen elements that isn't a simple chain, but a tree, which seems weird. Some check on ancestor browsing contexts is probably needed in the fullscreen element ready check.
Comment 1 Anne 2014-08-13 11:34:31 UTC
Additional bullet point:

<li><p>If /element/ has a browser context container and /element/'s browser context container's node document's fullscreen element stack is non-empty, /element/'s browser context container's node document's fullscreen element stack top element is /element/'s browser context container.
<!-- cross-process -->
Comment 2 Philip Jägenstedt 2014-08-13 11:54:10 UTC
It also has do deal with nested iframes, so you'd have consider all ancestor browsing contexts. Also, if the element in question isn't an iframe itself, does it make sense to say "If /element/ has a browser context container" or does it have to be something like "If /element/'s node documents's something or another"?
Comment 3 Anne 2014-08-13 13:26:32 UTC
For any /element/'s ancestor browsing context's document with a non-empty fullscreen element stack the top element of that fullscreen element stack is the ancestor browsing context's nested browsing context's browsing context container.
Comment 4 Philip Jägenstedt 2014-08-13 15:40:52 UTC
I just meant that you may need to traverse up several nested document before getting to a document with a non-empty fullscreen element stack. When you get there, basically the fullscreen element ready check should hold true for the iframe that's going to be push to the stack later.
Comment 5 Anne 2014-08-13 15:59:26 UTC
I'll just keep trying. Sorry :-(

<li><p>If /element/'s browsing context has a browser context container, the fullscreen element ready check returns true for /element/'s browsing context's browser context container.
<!-- cross-process, recursive -->
Comment 6 Philip Jägenstedt 2014-08-13 19:51:07 UTC
Yeah, that looks promising. A case I forgot about is when the relevant iframe element is already fullscreen, in which case the element ready check would fail, but that iframe's document's fullscreen element stack isn't supposed to change.

So I guess one should walk the ancestor iframes, doing the ready check for them, and stop at the first one (if any) that's at the top of its document's fullscreen element stack?

This stuff is complicated :(
Comment 7 Anne 2014-08-14 09:14:52 UTC
So how about I make step 9.3, step 9.2 (seems to be required anyway). And then I change ancestor in "element's node document's fullscreen element stack is either empty or its top element is an ancestor of element" to inclusive ancestor. In addition to adding the point from comment 5.
Comment 8 Philip Jägenstedt 2014-08-14 09:52:24 UTC
What does "make step 9.3, step 9.2" mean? It might be easier if you just make a spec change and I can review that :)
Comment 10 Philip Jägenstedt 2014-08-14 11:51:53 UTC
I've tried implementing the change and it looks good.

A test regression caused by this is when requesting fullscreen twice on the same element. Previously it would fire a fullscreenerror event, now it does nothing, so anyone waiting for an event will wait forever. I'm not sure if it's a real-world problem, but it would be nice if a call to requestFullscreen() always resulted in an event.

Nit: Can an element have a browsing context? Everywhere else in the spec it's documents which have them, and that's also where I have to get it from in Blink.

Nit 2: Rephrase the last point to something that always evaluates to true or false, as opposed to an if with no else branch?
Comment 11 Anne 2014-08-14 15:05:01 UTC
I think this might actually be nicer. This way only state changes or failure to make state changes trigger events. But if you wanted fullscreen and you already are, there's no need to do anything.
Comment 13 Philip Jägenstedt 2014-08-14 21:18:04 UTC
(In reply to Anne from comment #11)
> I think this might actually be nicer. This way only state changes or failure
> to make state changes trigger events. But if you wanted fullscreen and you
> already are, there's no need to do anything.

OK, let's try doing it this way, it's likely this edge case doesn't matter for site compat.
Comment 14 Philip Jägenstedt 2014-08-14 21:27:43 UTC
(In reply to Anne from comment #12)
> https://github.com/whatwg/fullscreen/commit/
> d1b6a381a37354b65f63d81692dd7a15ba512732

The oldNode rewrite has a problem: In "Otherwise, remove element from its node document's fullscreen element stack" the element may not be in the fullscreen element stack at all, need to make it conditional.

The fullscreen element ready check fix looks pedantically correct :)
Comment 15 Philip Jägenstedt 2014-08-14 21:40:17 UTC
(In reply to Philip Jägenstedt from comment #14)
> (In reply to Anne from comment #12)
> > https://github.com/whatwg/fullscreen/commit/
> > d1b6a381a37354b65f63d81692dd7a15ba512732
> 
> The oldNode rewrite has a problem: In "Otherwise, remove element from its
> node document's fullscreen element stack" the element may not be in the
> fullscreen element stack at all, need to make it conditional.

That step also still says /element/ instead of /oldNode/.
Comment 17 Philip Jägenstedt 2014-08-15 11:40:15 UTC
Great, I have nothing further, will try to implement and report any issues.