Full screen will exit during a provisional load of a non-ancestor iframe.
Created attachment 139537 [details] Patch
Comment on attachment 139537 [details] Patch Surely it must be possible to make a test for this, given that the functionality is exposed to WKTR. If it's not testable, please explain why not.
Okay, writing up a test case now.
I'm hitting upon a difficult problem in writing a layout test: because both DRT and WKTR intercept the requests to enter and exit full screen, as far as they are concerned, there is no full screen controller present when the didStartProvisionalLoadForFrame message comes through. Therefore they do not attempt to exit full screen at that point. So I can craft a test which works in a browser (i.e. Safari), but not in DRT or WKTR.
There's the additional problem that the behavior in the bug wasn't reproducing in DRT or WKTR in the first place. So it will be impossible to craft a test which fails in the current build, but passes with the patch applied.
OK, I'll buy that. Can you add that explanation to the ChangeLog and repost for review?
Created attachment 140114 [details] Patch Added a testcase, which while it will not fail when run against older builds, will at least catch future regressions.
<rdar://problem/11213173>
Comment on attachment 140114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140114&action=review > Source/WebKit/mac/ChangeLog:15 > + Move WebKitFullScreenListener into its own file: Why mix this into the same patch with a bug fix? This obscures the question of whether you made any code changes to WebKitFullScreenListener. Why not move it into its own file separately, first? > Source/WebKit/mac/WebView/WebView.mm:1858 > + WebKitFullScreenListener* listener = [[WebKitFullScreenListener alloc] initWithElement:element]; The * is positioned wrong here. > Source/WebKit/mac/WebView/WebView.mm:1863 > + WebFullScreenController* controller = _private->newFullscreenController; > + [controller close]; The * is positioned wrong here and I’m not sure why we’re using a local variable here when we did not use one on the line above.
Comment on attachment 140114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140114&action=review >> Source/WebKit/mac/ChangeLog:15 >> + Move WebKitFullScreenListener into its own file: > > Why mix this into the same patch with a bug fix? This obscures the question of whether you made any code changes to WebKitFullScreenListener. > > Why not move it into its own file separately, first? Only for expediency. Two dependent bugs & patches makes the EWS bots sad. That said, I can pull this out into a separate patch & bug, since it's already passed the EWS bots. >> Source/WebKit/mac/WebView/WebView.mm:1858 >> + WebKitFullScreenListener* listener = [[WebKitFullScreenListener alloc] initWithElement:element]; > > The * is positioned wrong here. Ack. Changed. >> Source/WebKit/mac/WebView/WebView.mm:1863 >> + [controller close]; > > The * is positioned wrong here and I’m not sure why we’re using a local variable here when we did not use one on the line above. Ditto. And no good reason. I'll eliminate the local variable.
Gah, webkit-patch upload tried to upload to the wrong bug, and obseleted the patch that darin r+'d. Sigh, i'll upload a new patch that's dependent on https://bugs.webkit.org/show_bug.cgi?id=85640.
Created attachment 140271 [details] Patch
Created attachment 140276 [details] Patch
Created attachment 140277 [details] Patch
Comment on attachment 140277 [details] Patch Attachment 140277 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12565208
Comment on attachment 140277 [details] Patch r=me
Committed r116165: <http://trac.webkit.org/changeset/116165>