Bug 93525 - webkitfullscreenchange not fired properly in iframe
Summary: webkitfullscreenchange not fired properly in iframe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-08 14:24 PDT by Bill Budge
Modified: 2012-08-21 09:48 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (6.34 KB, patch)
2012-08-09 16:51 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed patch (3.03 KB, patch)
2012-08-09 17:38 PDT, Bill Budge
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (647.18 KB, application/zip)
2012-08-09 19:01 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-05 (304.80 KB, application/zip)
2012-08-09 19:58 PDT, WebKit Review Bot
no flags Details
pro (3.40 KB, text/plain)
2012-08-13 14:39 PDT, Bill Budge
no flags Details
Proposed patch (3.40 KB, patch)
2012-08-13 14:41 PDT, Bill Budge
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (314.68 KB, application/zip)
2012-08-13 16:10 PDT, WebKit Review Bot
no flags Details
Proposed patch (4.19 KB, patch)
2012-08-14 10:31 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed patch (5.07 KB, patch)
2012-08-17 17:07 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed patch (5.07 KB, patch)
2012-08-17 17:46 PDT, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Budge 2012-08-08 14:24:37 PDT
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/)
Comment 1 Bill Budge 2012-08-08 14:25:28 PDT
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>
Comment 2 Bill Budge 2012-08-08 14:27:19 PDT
Possible duplicate of 90704, 90708.
Comment 3 Bill Budge 2012-08-09 16:51:32 PDT
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.
Comment 4 Bill Budge 2012-08-09 17:04:37 PDT
I need to re-do the code that checks for duplicate events in Document::fullScreenChangeDelayTimerFired.
Comment 5 Bill Budge 2012-08-09 17:38:02 PDT
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 6 WebKit Review Bot 2012-08-09 19:01:07 PDT
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
Comment 7 WebKit Review Bot 2012-08-09 19:01:10 PDT
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 8 WebKit Review Bot 2012-08-09 19:58:36 PDT
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
Comment 9 WebKit Review Bot 2012-08-09 19:58:40 PDT
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
Comment 10 Bill Budge 2012-08-13 14:39:09 PDT
Created attachment 158107 [details]
pro
Comment 11 Bill Budge 2012-08-13 14:41:37 PDT
Created attachment 158108 [details]
Proposed patch
Comment 12 Bill Budge 2012-08-13 14:58:23 PDT
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 13 Jer Noble 2012-08-13 16:02:19 PDT
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 14 WebKit Review Bot 2012-08-13 16:10:35 PDT
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
Comment 15 WebKit Review Bot 2012-08-13 16:10:40 PDT
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
Comment 16 Bill Budge 2012-08-14 10:31:58 PDT
Created attachment 158364 [details]
Proposed patch
Comment 17 Bill Budge 2012-08-14 10:43:16 PDT
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.
Comment 18 Jer Noble 2012-08-16 10:33:19 PDT
Looks good. It just needs a LayoutTest.
Comment 19 Bill Budge 2012-08-17 17:07:22 PDT
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.
Comment 20 Bill Budge 2012-08-17 17:46:04 PDT
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.
Comment 21 Jer Noble 2012-08-17 17:58:00 PDT
Thanks for clarifying.  Looks good, but I can give only an unofficial  r+.
Comment 22 Adam Barth 2012-08-19 21:38:38 PDT
Comment on attachment 159244 [details]
Proposed patch

I don't fully understand this patch, but rs=me based on Jer Noble's LGTM.
Comment 23 WebKit Review Bot 2012-08-20 11:11:38 PDT
Comment on attachment 159244 [details]
Proposed patch

Clearing flags on attachment: 159244

Committed r126043: <http://trac.webkit.org/changeset/126043>
Comment 24 WebKit Review Bot 2012-08-20 11:11:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Darin Adler 2012-08-20 18:17:34 PDT
Comment on attachment 159244 [details]
Proposed patch

Why no test case?
Comment 26 Bill Budge 2012-08-21 09:48:35 PDT
(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.