26568 2014-08-13 10:55:28 +0000 Nothing stopping mis-nested fullscreen in iframes? 2014-08-15 11:40:15 +0000 1 1 1 Unclassified WebAppsWG Fullscreen unspecified PC Linux RESOLVED FIXED P2 normal --- 1 philipj annevk mike public-webapps public-webapps-bugzilla oldest_to_newest 110126 0 philipj 2014-08-13 10:55:28 +0000 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. 110128 1 annevk 2014-08-13 11:34:31 +0000 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 --> 110130 2 philipj 2014-08-13 11:54:10 +0000 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"? 110136 3 annevk 2014-08-13 13:26:32 +0000 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. 110141 4 philipj 2014-08-13 15:40:52 +0000 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. 110143 5 annevk 2014-08-13 15:59:26 +0000 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 --> 110157 6 philipj 2014-08-13 19:51:07 +0000 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 :( 110172 7 annevk 2014-08-14 09:14:52 +0000 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. 110173 8 philipj 2014-08-14 09:52:24 +0000 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 :) 110174 9 annevk 2014-08-14 10:00:37 +0000 https://github.com/whatwg/fullscreen/commit/742c24c03407688bd2de60d012409d6c8b7f6051 110176 10 philipj 2014-08-14 11:51:53 +0000 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? 110186 11 annevk 2014-08-14 15:05:01 +0000 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. 110187 12 annevk 2014-08-14 15:12:47 +0000 https://github.com/whatwg/fullscreen/commit/d1b6a381a37354b65f63d81692dd7a15ba512732 110198 13 philipj 2014-08-14 21:18:04 +0000 (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. 110199 14 philipj 2014-08-14 21:27:43 +0000 (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 :) 110200 15 philipj 2014-08-14 21:40:17 +0000 (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/. 110209 16 annevk 2014-08-15 09:41:10 +0000 https://github.com/whatwg/fullscreen/commit/1fe479e54d3fb162127f6d86dce51fa6e5caf8cc Thanks! New issue, new bug? 110211 17 philipj 2014-08-15 11:40:15 +0000 Great, I have nothing further, will try to implement and report any issues.