WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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.
Top of Page
Format For Printing
XML
Clone This Bug