Bug 93525 – webkitfullscreenchange not fired properly in iframe

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Bill Budge]

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/)

[Bill Budge]

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>

[Bill Budge]

Possible duplicate of 90704, 90708.

[Bill Budge]

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.

[Bill Budge]

I need to re-do the code that checks for duplicate events in Document::fullScreenChangeDelayTimerFired.

[Bill Budge]

Created attachment 157593 [details]
Proposed patch

In the meantime, here's the fix for webkitCancelFullScreen, so it will dispatch events when webkitDidExitFullScreen is called.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bill Budge]

Created attachment 158107 [details]
pro

[Bill Budge]

Created attachment 158108 [details]
Proposed patch

[Bill Budge]

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.

[Jer Noble]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bill Budge]

Created attachment 158364 [details]
Proposed patch

[Bill Budge]

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.

[Jer Noble]

Looks good. It just needs a LayoutTest.

[Bill Budge]

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.

[Bill Budge]

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.

[Jer Noble]

Thanks for clarifying.  Looks good, but I can give only an unofficial  r+.

[Adam Barth]

Comment on attachment 159244 [details]
Proposed patch

I don't fully understand this patch, but rs=me based on Jer Noble's LGTM.

[WebKit Review Bot]

Comment on attachment 159244 [details]
Proposed patch

Clearing flags on attachment: 159244

Committed r126043: <http://trac.webkit.org/changeset/126043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 159244 [details]
Proposed patch

Why no test case?

[Bill Budge]

(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.