Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 13 users
Status: Fixed
Owner:
Closed: Aug 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Regression

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
webkitfullscreenchange not fired properly in iframe
Reported by m...@brad.is, Jul 21 2012 Back to list
Chrome Version       : 22.0.1212.0 canary
OS Version: OS X 10.7.4, Windows 7
URLs (if applicable) : http://brad.is/reportingbugs/webkitfullscreenchange/
Other browsers tested:
  Add OK or FAIL after other browsers where you have tested this issue:
     Safari 5: OK
  Firefox 14: OK
     IE 7/8/9: N/A

What steps will reproduce the problem?
1. Go to http://brad.is/reportingbugs/webkitfullscreenchange/
2. Click the full screen button
3. Use esc or click the exit button to exit full screen

What is the expected result?
webkitfullscreenchange should fire once when it enters, and once when it leaves full screen.

What happens instead?
webkitfullscreenchange fires twice when entering, and does not fire when exiting

Please provide any additional information below. Attach a screenshot if
possible.

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.
 
Comment 1 by meh...@chromium.org, Jul 22 2012
Labels: -Area-Undefined Area-UI Feature-FullScreen
Labels: Hotlist-GoogleApps
This is a problem for Google Presentations, that's blocking us from making "Start presentation" automatically open in full screen
Cc: gregsimon@chromium.org
Labels: Action-BisectNeeded
Greg, do you know someone good to look into this?

If this is a regression (looks like it is), it would be good to get a bisect.
Cc: jeremya@chromium.org koz@chromium.org
Koz - is this a regression? Can you please take a look?
Comment 5 by m...@brad.is, Jul 24 2012
I don't know if this is a Webkit or browser thing, but it does work correctly in whatever version of Webkit that is in Safari 5.1. 
Comment 6 by mbollu@chromium.org, Jul 24 2012
OS: ALL

Findings:
Issue reproducible on: 22.0.1216.0 (Official Build 148053) canary, 20.0.1132.57 (Official Build 145807) stable, 21.0.1180.49 (Official Build 147161) beta, 22.0.1215.0 (Official Build 147830) dev

Issue not reproducible on: 19.0.1084.56 (Official Build 140965)

Regression window:
Revision 129378 is [(g)ood/(b)ad/(q)uit]: b
You are probably looking for a change made after 129376 (known good), but no later than 129378 (first known bad).
CHANGELOG URL:
  http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=129376%3A129378

Comment 7 by mbollu@chromium.org, Jul 24 2012
Labels: -OS-Mac -Type-Bug -Action-BisectNeeded OS-All Type-Regression Mstone-22 ReleaseBlock-Beta
Status: Untriaged
Cc: mbollu@chromium.org
The bisect info isn't pointing the right changes. mbollu@: could you try bisecting it again?
Comment 9 by kareng@google.com, Jul 25 2012
Owner: mbollu@chromium.org
Status: Assigned
Here is the regression window from Win7:

You are probably looking for a change made after 129388 (known good), but no lat
er than 129392 (first known bad).
WEBKIT CHANGELOG URL:
  http://trac.webkit.org/log/trunk/?rev=112365&stop_rev=112327&verbose=on&limit=
10000
CHANGELOG URL:
  http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/tru
nk/src&range=129388%3A129392
Owner: ----
Status: Untriaged
Cc: dharani@chromium.org
Now that we have a bisect, is there someone who can own this?
Comment 14 by kareng@google.com, Jul 26 2012
Owner: jam...@chromium.org
Status: Assigned
james can u please take a look? dana and I looked andwe're not sure which change maybe it's this? http://trac.webkit.org/changeset/112338 
Cc: -jeremya@chromium.org jam...@chromium.org
Owner: jeremya@chromium.org
I don't know anything about fullscreen.  Jeremy - can you direct?
Cc: bbudge@chromium.org
bbudge has been a total ninja at fullscreen recently, perhaps he could take a look?

I can't see anything likely-looking in the range mbollu@ posted. Perhaps it's a bit flaky?
Comment 17 Deleted
I looked at this in the debugger. The problem seems to be in WebKit (WebCore/dom/Document.cpp) where webkitCancelFullScreen doesn't send events to the same targets as the code in webkitRequestFullScreenForElement. Darin and I could reproduce the bug in the WebKit nightly build (Safari) so this looks like a WebKit regression.

http://builds.nightly.webkit.org/files/trunk/mac/WebKit-SVN-r123921.dmg

Do you want to file the bug (best, you will be the reporter) or should I?

https://bugs.webkit.org/

Here is a script we used to test and verify the bug:

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

Here is some test HTML:
Comment 19 by k...@google.com, Aug 2 2012
Owner: bbudge@chromium.org
Did the webkit bug get filed?
It's best if you file the bug report, since you are the original reporter here, but if you prefer, I can do it for you.
Can you do it in this case? Sorry, this is an issue the apps team cares about, and they're not familiar with the WebKit bug tracker.
I searched the open bugs at bugs.webkit.org using the terms "fullscreen iframe" and found two existing bugs that seem related:

https://bugs.webkit.org/show_bug.cgi?id=90704
https://bugs.webkit.org/show_bug.cgi?id=90708

I filed a new bug reporting that the events are wrong:

https://bugs.webkit.org/show_bug.cgi?id=93525

I don't know how actively the bugs here are being worked on.
Cc: darin@chromium.org
I looked at the problem today and the code is complex. I put together a "band-aid" patch which makes the example work for me and uploaded it on the webkit bug https://bugs.webkit.org/show_bug.cgi?id=93525. I need to make sure it passes layout tests (and sort out the situation to see if I need additional tests, or just to reenable disabled tests.)

We'll see what the reviewers say.
Status: Started
I uploaded a patch that fixes both the duplicate event problem and the lack of events when canceling fullscreen. I don't know how it will be reviewed for WebKit but it passes all currently passing layout tests and is small enough to be merged to a Chrome branch.
Webkit patch is under review.
Still waiting for another round of reviews (for >24 hours). The patch is small and fixes a number of issues and should be a good candidate for merging into the branch even if review takes a while.
Status: ExternalDependency
Reviewer wants a layout test. I'll try to get that done today.
Comment 30 Deleted
Patch is up with a layout test. Needs an official reviewer to give r+ and then should land.
Comment 32 by k...@google.com, Aug 20 2012
Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Will take this when ready.
Labels: Merge-Requested
Status: Fixed
Change landed in Webkit: http://trac.webkit.org/changeset/126043
Comment 34 by k...@google.com, Aug 20 2012
Labels: -Merge-Requested Merge-Approved
Comment 35 by k...@google.com, Aug 21 2012
Labels: -Merge-Approved Merge-Merged
Project Member Comment 36 by bugdroid1@chromium.org, Oct 13 2012
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member Comment 37 by bugdroid1@chromium.org, Mar 9 2013
Labels: -Area-UI -Type-Regression -Feature-FullScreen -Mstone-22 Type-Bug-Regression M-22 Cr-UI Cr-UI-Browser-FullScreen
Project Member Comment 38 by bugdroid1@chromium.org, Mar 14 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Sign in to add a comment