127064 – [GTK] youtube HTML5 videos in fullscreen, after <Esc>, can't go fullscreen again

RESOLVED FIXED 127064

[GTK] youtube HTML5 videos in fullscreen, after <Esc>, can't go fullscreen again

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

Summary [GTK] youtube HTML5 videos in fullscreen, after <Esc>, can't go fullscreen again

Víctor M. Jáquez L.

Reported 2014-01-15 13:47:36 PST

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

Attachments
testcase (1.39 KB, text/html) 2014-01-17 10:04 PST, Víctor M. Jáquez L.	no flags	Details
Patch (6.48 KB, patch) 2014-01-20 08:35 PST, Víctor M. Jáquez L.	no flags	Details Formatted Diff Diff
Patch (6.70 KB, patch) 2014-01-20 09:31 PST, Víctor M. Jáquez L.	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (455.82 KB, application/zip) 2014-01-20 10:28 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (488.22 KB, application/zip) 2014-01-20 10:53 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (539.00 KB, application/zip) 2014-01-20 11:44 PST, Build Bot	no flags	Details
Patch (3.83 KB, patch) 2014-01-21 08:16 PST, Víctor M. Jáquez L.	no flags	Details Formatted Diff Diff
Patch (3.73 KB, patch) 2014-01-24 13:11 PST, Víctor M. Jáquez L.	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Víctor M. Jáquez L.

Comment 1 2014-01-15 13:48:26 PST

I still don't discard that it may be a bug in the youtube's player. I'll do that tomorrow.

Philippe Normand

Comment 2 2014-01-15 22:41:04 PST

Is this with their HTML5 or Flash player?

Víctor M. Jáquez L.

Comment 3 2014-01-15 23:55:02 PST

(In reply to comment #2) > Is this with their HTML5 or Flash player? HTML5

Víctor M. Jáquez L.

Comment 4 2014-01-16 08:26:45 PST

oddly enough, the bad behavior is only shown in webkit2, not in webkit1 with GtkLaunch. Using GtkLaunch the behavoir is as expected.

Víctor M. Jáquez L.

Comment 5 2014-01-17 10:04:00 PST

Created attachment 221475 [details] testcase this test web page is to corner the bug

Víctor M. Jáquez L.

Comment 6 2014-01-17 10:06:03 PST

The error is simple: when pressing <Esc> to cancel fullscreen, the event webkitfullscreenchange is not emitted. Again, this only happens in webkit2.

Víctor M. Jáquez L.

Comment 7 2014-01-17 11:11:44 PST

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?

Víctor M. Jáquez L.

Comment 8 2014-01-20 08:35:33 PST

Created attachment 221661 [details] Patch

WebKit Commit Bot

Comment 9 2014-01-20 08:37:59 PST

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

Víctor M. Jáquez L.

Comment 10 2014-01-20 08:38:57 PST

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.

Sergio Villar Senin

Comment 11 2014-01-20 09:07:59 PST

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.

Víctor M. Jáquez L.

Comment 12 2014-01-20 09:31:31 PST

Created attachment 221666 [details] Patch

Víctor M. Jáquez L.

Comment 13 2014-01-20 09:32:34 PST

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

Víctor M. Jáquez L.

Comment 14 2014-01-20 09:33:28 PST

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

Build Bot

Comment 15 2014-01-20 10:28:34 PST

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

Build Bot

Comment 16 2014-01-20 10:28:36 PST

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

Build Bot

Comment 17 2014-01-20 10:53:44 PST

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

Build Bot

Comment 18 2014-01-20 10:53:47 PST

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

Martin Robinson

Comment 19 2014-01-20 10:59:13 PST

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?

Víctor M. Jáquez L.

Comment 20 2014-01-20 11:12:12 PST

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.

Martin Robinson

Comment 21 2014-01-20 11:29:41 PST

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

Build Bot

Comment 22 2014-01-20 11:44:32 PST

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

Build Bot

Comment 23 2014-01-20 11:44:35 PST

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

Víctor M. Jáquez L.

Comment 24 2014-01-20 12:36:55 PST

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.

Víctor M. Jáquez L.

Comment 25 2014-01-21 02:17:40 PST

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

Víctor M. Jáquez L.

Comment 26 2014-01-21 08:16:01 PST

Created attachment 221748 [details] Patch

Martin Robinson

Comment 27 2014-01-24 09:36:15 PST

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.

Víctor M. Jáquez L.

Comment 28 2014-01-24 13:11:21 PST

Created attachment 222146 [details] Patch

WebKit Commit Bot

Comment 29 2014-01-24 14:00:55 PST

Comment on attachment 222146 [details] Patch Clearing flags on attachment: 222146 Committed r162724: <http://trac.webkit.org/changeset/162724>

WebKit Commit Bot

Comment 30 2014-01-24 14:01:02 PST

All reviewed patches have been landed. Closing bug.

Carlos Garcia Campos

Comment 31 2014-02-17 05:41:07 PST

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

Víctor M. Jáquez L.

Comment 32 2014-02-17 06:18:09 PST

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.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebKitGTK

Assignee

Víctor M. Jáquez L.

Reported

2014-01-15 13:47 PST

Modified

2014-02-17 06:18 PST History

CC List

10 users Show

URL

Keywords

Depends on

Blocks