webkitfullscreenchange is not fired when exiting full screen if the element exiting is inside an iframe. The event is fired twice when entering full screen. If you access the iframe page directly (http://brad.is/reportingbugs/webkitfullscreenchange/iframe.html), the event fires properly. When that same page is inside an iframe, the event does not (http://brad.is/reportingbugs/webkitfullscreenchange/)
Here's a test script that exposes the problem. <script> function log(x) { document.getElementById("console").innerText += x + "\n"; } document.onwebkitfullscreenchange = function() { log("fullscreenchange"); } onload = function() { log("load"); } if (location.search.substring(1) != "child") { document.write('<iframe webkitallowfullscreen src="test_initiated_by_subframe.html?child"></iframe>'); document.write("<br>PARENT"); } else { document.write("<br>CHILD"); } </script> <br> <button onclick="document.documentElement.webkitRequestFullScreen()">enter fullscreen</button> <button onclick="document.webkitCancelFullScreen()">exit fullscreen</button> <pre id="console"></pre>
Possible duplicate of 90704, 90708.
Created attachment 157580 [details] Proposed patch Putting this up for general comments on the approach. This is more of a band-aid, but it doesn't unduly complicate the code and fixes several issues that are causing problems in Chromium.
I need to re-do the code that checks for duplicate events in Document::fullScreenChangeDelayTimerFired.
Created attachment 157593 [details] Proposed patch In the meantime, here's the fix for webkitCancelFullScreen, so it will dispatch events when webkitDidExitFullScreen is called.
Comment on attachment 157593 [details] Proposed patch Attachment 157593 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13477042 New failing tests: fullscreen/full-screen-restrictions.html
Created attachment 157610 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 157593 [details] Proposed patch Attachment 157593 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13461740 New failing tests: fullscreen/full-screen-restrictions.html
Created attachment 157619 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 158107 [details] pro
Created attachment 158108 [details] Proposed patch
This is a fix for some problems we see in Chrome when canceling fullscreen in an iframe. There are two change events on the iframe when going to fullscreen, and none when exiting fullscreen. This patch should pass all enabled fullscreen layout tests. It fails the same tests as the current code. I haven't determined what to do about the failing tests, or whether new tests are needed.
Comment on attachment 158108 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=158108&action=review > Source/WebCore/ChangeLog:18 > + * dom/Document.cpp: > + Please specify which functions were modified in the ChangeLog. > Source/WebCore/dom/Document.cpp:-5019 > - if (settings->thirdPartyStorageBlockingEnabled()) > - securityOrigin()->blockThirdPartyStorage(); These seem unrelated. Was this intentional? > Source/WebCore/dom/Document.cpp:5512 > - // 1. Let doc be the context object. (i.e. "this") > + // 1. Let doc be the context object. (i.e. "this") Unnecessary whitespace change. > Source/WebCore/dom/Document.cpp:5742 > // If the element was removed from our tree, also message the documentElement. > - if (!contains(node.get())) > + if (!contains(node.get()) && !node->isFrameOwnerElement()) Please update the comment to explain why the !node->isFrameOwnerElement() is necessary. > Source/WebCore/dom/Document.cpp:5757 > - if (!contains(node.get())) > + if (!contains(node.get()) && !node->isFrameOwnerElement()) Ditto.
Comment on attachment 158108 [details] Proposed patch Attachment 158108 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13481987 New failing tests: http/tests/security/cross-origin-session-storage.html http/tests/security/cross-origin-local-storage.html
Created attachment 158140 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 158364 [details] Proposed patch
I modified some code in webkitExitFullScreen beyond what I did in the last patch. The first change deletes some code that appears to have no effect. The second change adds a continue, as that seems to be the desired behavior according to the comments and spec.
Looks good. It just needs a LayoutTest.
Created attachment 159231 [details] Proposed patch The existing fullscreen/exit-full-screen-iframe.html test seems correct. I made some adjustments to my patch so that this test now passes.
Created attachment 159244 [details] Proposed patch Fixed some wacky lines in the ChangeLog. To clarify my last post, the fix is now tested by an existing layout test, fullscreen/exit-full-screen-iframe.html.
Thanks for clarifying. Looks good, but I can give only an unofficial r+.
Comment on attachment 159244 [details] Proposed patch I don't fully understand this patch, but rs=me based on Jer Noble's LGTM.
Comment on attachment 159244 [details] Proposed patch Clearing flags on attachment: 159244 Committed r126043: <http://trac.webkit.org/changeset/126043>
All reviewed patches have been landed. Closing bug.
Comment on attachment 159244 [details] Proposed patch Why no test case?
(In reply to comment #25) > (From update of attachment 159244 [details]) > Why no test case? The existing 'fullscreen/exit-full-screen-iframe.html' test case covers this. The test now passes.