Steps to reproduce the issue: 1. Go to http://people.igalia.com/vjaquez/yt-page.html (simple youtube's iframe player) 2. Press to toggle button to switch to fullscreen 3. Once in fullscreen mode, press the Escape key to switch back to normal mode Expected behavior: * The fullscreen toggle button should be in ready-to-go-fullscreen state Observed behavoir: * The fullscreen toggle button is in fullscreen state and can't be switched back, it's not usable to go to fullscreen again. Notes: It works in Chromium, in WebKit for Mac and Firefox as expected, but not in WebKitGTK+.
I still don't discard that it may be a bug in the youtube's player. I'll do that tomorrow.
Is this with their HTML5 or Flash player?
(In reply to comment #2) > Is this with their HTML5 or Flash player? HTML5
oddly enough, the bad behavior is only shown in webkit2, not in webkit1 with GtkLaunch. Using GtkLaunch the behavoir is as expected.
Created attachment 221475 [details] testcase this test web page is to corner the bug
The error is simple: when pressing <Esc> to cancel fullscreen, the event webkitfullscreenchange is not emitted. Again, this only happens in webkit2.
The thing is this: When pressing the <Esc>, at the function Document::dispatchFullScreenChangeOrErrorEvent(), the queue is empty, hence the event webkitfullscreenchange is not dispatched. Why is it empty?
Created attachment 221661 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
This odd: running the layout test inside MiniBrowser, GtkLaunch or DTR, it works as expected. But, running it with WKTR, it times out, as if the eventSender doesn't send the keyDown correctly.
Comment on attachment 221661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221661&action=review Looks good to me. Added some nits. > Source/WebKit2/ChangeLog:10 > + This is a nice description of the fix, but I miss some context, like the actual explanation of why it was failing (I understand it should be related to the event emission but it isn't a direct outcome of the description) > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1072 > +} I guess the function does not make sense at all without the FULLSCREEN_API. It might be better to guard it all (after all the caller is already guarded) > LayoutTests/fullscreen/full-screen-escape.html:1 > +<body> It's a good practice to start the file with <!DOCTYPE html> > LayoutTests/fullscreen/full-screen-escape.html:37 > + runWithKeyDown(function(){span.webkitRequestFullScreen()}); Add some spaces between the call and the brackets, also after the parentheses.
Created attachment 221666 [details] Patch
Comment on attachment 221661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221661&action=review >> Source/WebKit2/ChangeLog:10 >> + > > This is a nice description of the fix, but I miss some context, like the actual explanation of why it was failing (I understand it should be related to the event emission but it isn't a direct outcome of the description) done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1072 >> +} > > I guess the function does not make sense at all without the FULLSCREEN_API. It might be better to guard it all (after all the caller is already guarded) I'm following what the other functions related with full screen do: keep the function signature but guard the content. >> LayoutTests/fullscreen/full-screen-escape.html:1 >> +<body> > > It's a good practice to start the file with <!DOCTYPE html> done >> LayoutTests/fullscreen/full-screen-escape.html:37 >> + runWithKeyDown(function(){span.webkitRequestFullScreen()}); > > Add some spaces between the call and the brackets, also after the parentheses. done
(In reply to comment #11) > (From update of attachment 221661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221661&action=review Thanks for the promptly review!
Comment on attachment 221666 [details] Patch Attachment 221666 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6274908631859200 New failing tests: fullscreen/full-screen-escape.html
Created attachment 221673 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 221666 [details] Patch Attachment 221666 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6693187779297280 New failing tests: fullscreen/full-screen-escape.html
Created attachment 221675 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 221666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review > LayoutTests/fullscreen/full-screen-escape.html:16 > + var fullscreenChanged = function(event) > + { > + if (callback) > + callback(event) > + }; Not self-consistent with curly brace placement below. > LayoutTests/fullscreen/full-screen-escape.html:25 > + setTimeout(exitFullScreen, 100); Why the 100 millisecond timeout?
Comment on attachment 221666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review >> LayoutTests/fullscreen/full-screen-escape.html:25 >> + setTimeout(exitFullScreen, 100); > > Why the 100 millisecond timeout? This is an arbitrary value, just to make a pause before exiting the full screen mode.
(In reply to comment #20) > (From update of attachment 221666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review > > >> LayoutTests/fullscreen/full-screen-escape.html:25 > >> + setTimeout(exitFullScreen, 100); > > > > Why the 100 millisecond timeout? > > This is an arbitrary value, just to make a pause before exiting the full screen mode. Why is the pause necessary?
Comment on attachment 221666 [details] Patch Attachment 221666 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5110401599537152 New failing tests: fullscreen/full-screen-escape.html
Created attachment 221679 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 221666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review >>>> LayoutTests/fullscreen/full-screen-escape.html:25 >>>> + setTimeout(exitFullScreen, 100); >>> >>> Why the 100 millisecond timeout? >> >> This is an arbitrary value, just to make a pause before exiting the full screen mode. > > Why is the pause necessary? Actually, it is not necessary, just inlining exitFullScreen in there should be enough. What worries me is this test doesn't work in any platform: in safari doesn't because eventSent.keyDown doesn't work, and in gtk, the same function doesn't work in fullscreen mode (to add the bug is in my todo). Perhaps I should remove the test.
Since none of the platforms implements correctly the sendEvent.keyDown in WKTR when an element is in full screen mode, I'll move out the test into ManualTests
Created attachment 221748 [details] Patch
Comment on attachment 221748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221748&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1071 > + WebFullScreenManagerProxy* fullScreenManagerProxy = webkitWebViewBase->priv->pageProxy->fullScreenManager(); > + fullScreenManagerProxy->requestExitFullScreen(); > +#endif This can be one line.
Created attachment 222146 [details] Patch
Comment on attachment 222146 [details] Patch Clearing flags on attachment: 222146 Committed r162724: <http://trac.webkit.org/changeset/162724>
All reviewed patches have been landed. Closing bug.
Comment on attachment 222146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222146&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:48 > +void webkitWebViewBaseRequestExitFullScreen(WebKitWebViewBase*); Why are you adding private API that is only used in WebKitWebViewBase.cpp? This should be a static method, or you could even avoid the method entirely that is used only once and use priv->pageProxy->fullScreenManager()->requestExitFullScreen(); directly
Comment on attachment 222146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222146&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:48 >> +void webkitWebViewBaseRequestExitFullScreen(WebKitWebViewBase*); > > Why are you adding private API that is only used in WebKitWebViewBase.cpp? This should be a static method, or you could even avoid the method entirely that is used only once and use priv->pageProxy->fullScreenManager()->requestExitFullScreen(); directly I followed the code structure saw in webkitWebViewBaseExitFullScreen(), that's why.