Bug 136854 - [EFL][WK2] Minibrowser : Fix the 'Escape' button issue to exit fullscreen
Summary: [EFL][WK2] Minibrowser : Fix the 'Escape' button issue to exit fullscreen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-16 05:34 PDT by Rohit
Modified: 2014-09-25 23:24 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.10 KB, patch)
2014-09-16 05:38 PDT, Rohit
no flags Details | Formatted Diff | Diff
Patch for landing (2.48 KB, patch)
2014-09-22 02:25 PDT, Rohit
no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2014-09-23 06:37 PDT, Rohit
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rohit 2014-09-16 05:34:54 PDT
Escape button does not work when try to exit Minibrowser fullscreen mode.
Comment 1 Rohit 2014-09-16 05:38:50 PDT
Created attachment 238176 [details]
Patch
Comment 2 Gyuyoung Kim 2014-09-17 22:51:24 PDT
Comment on attachment 238176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238176&action=review

> Tools/MiniBrowser/efl/main.c:-565
> -            ewk_view_fullscreen_exit(ewk_view);

Doesn't this function work now ?
Comment 3 Rohit 2014-09-18 05:12:31 PDT
(In reply to comment #2)
> (From update of attachment 238176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238176&action=review
> 
> > Tools/MiniBrowser/efl/main.c:-565
> > -            ewk_view_fullscreen_exit(ewk_view);
> 
> Doesn't this function work now ?

This function doesn't work now. I think reason behind this is that the Webview does not know that it has gone full screen.
We use elm_win_fullscreen_set to toggle full screen and elm_window has fullscreen state information, and not the ewk_view. There is no specific API calls to enter full screen for ewk_view. So ewk_view_fullscreen_exit doesn't work as ewk_view doesn't have information about current fullscreen state.
Comment 4 Gyuyoung Kim 2014-09-18 23:00:55 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 238176 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=238176&action=review
> > 
> > > Tools/MiniBrowser/efl/main.c:-565
> > > -            ewk_view_fullscreen_exit(ewk_view);
> > 
> > Doesn't this function work now ?
> 
> This function doesn't work now. I think reason behind this is that the Webview does not know that it has gone full screen.
> We use elm_win_fullscreen_set to toggle full screen and elm_window has fullscreen state information, and not the ewk_view. There is no specific API calls to enter full screen for ewk_view. So ewk_view_fullscreen_exit doesn't work as ewk_view doesn't have information about current fullscreen state.

If so, I think we should remove the APIs, Probably new bug ?
Comment 5 Gyuyoung Kim 2014-09-18 23:09:42 PDT
Comment on attachment 238176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238176&action=review

Basically patch looks good. r=me. Please update ChangeLog before landing.

> Tools/ChangeLog:7
> +

It would be nicer if you mention what is problem, how to fix this problem.
Comment 6 Rohit 2014-09-22 02:25:22 PDT
Created attachment 238468 [details]
Patch for landing
Comment 7 Gyuyoung Kim 2014-09-22 03:05:53 PDT
Comment on attachment 238468 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=238468&action=review

> Tools/ChangeLog:6
> +        Reviewed by Gyuyoung Kim.

Add a new line.
Comment 8 Ryuan Choi 2014-09-22 04:14:38 PDT
Comment on attachment 238468 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=238468&action=review

> Tools/ChangeLog:9
> +        ewk_view_fullscreen_exit(ewk_view) doesn't work as ewk_view does not know that it has gone fullscreen.
> +        We use elm_win_fullscreen_set to toggle fullscreen and elm_window has fullscreen state information, and not the ewk_view.
> +        There is no specific API calls to enter fullscreen for ewk_view. So either we need to add API for enter fullscreen or remove ewk_view_fullscreen_exit.

Sorry for late comment.

But, I think that this comment looks wrong.

This issue is the bug of MiniBrowser.
ewk_view_fullscreen_exit() is to notify the changes of state to ewk_view when user cancelled HTML5's fullscreen API.
For example, w3c fullscreen API can make specific element as full screen (ex, video element).
So, ewk_view should know the exit to restore the web contents from specific element to whole contents when user cancelled requests of w3c fullscreen.

I think that MiniBrowser should distinguish which events started current full screen status, w3c's fullscreen API or shortcut(F11)
Comment 9 Rohit 2014-09-23 06:23:23 PDT
(In reply to comment #8)
> (From update of attachment 238468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238468&action=review
> 
> > Tools/ChangeLog:9
> > +        ewk_view_fullscreen_exit(ewk_view) doesn't work as ewk_view does not know that it has gone fullscreen.
> > +        We use elm_win_fullscreen_set to toggle fullscreen and elm_window has fullscreen state information, and not the ewk_view.
> > +        There is no specific API calls to enter fullscreen for ewk_view. So either we need to add API for enter fullscreen or remove ewk_view_fullscreen_exit.
> 
> Sorry for late comment.
> 
> But, I think that this comment looks wrong.
> 
> This issue is the bug of MiniBrowser.
> ewk_view_fullscreen_exit() is to notify the changes of state to ewk_view when user cancelled HTML5's fullscreen API.
> For example, w3c fullscreen API can make specific element as full screen (ex, video element).
> So, ewk_view should know the exit to restore the web contents from specific element to whole contents when user cancelled requests of w3c fullscreen.
> 
> I think that MiniBrowser should distinguish which events started current full screen status, w3c's fullscreen API or shortcut(F11)

Thanks for the insight. It makes sense. On observing behaviours for other broswers, I found that 'Esc'is used only for exiting fullscreen when a specific element is fullscreen and not in case of window fullscreen. In case when browser window is fullscreen, F11 is used to exit fullscreen. However, F11 can be used to exit fullscreen even when a window element has gone fullscreen as specific element makes window go fullscreen along with it. I will submit the modified patch based on this observation.
Comment 10 Rohit 2014-09-23 06:37:54 PDT
Created attachment 238531 [details]
Patch
Comment 11 Ryuan Choi 2014-09-24 05:02:41 PDT
Looks good to me.
Comment 12 Gyuyoung Kim 2014-09-25 22:49:21 PDT
Comment on attachment 238531 [details]
Patch

LGTM too.
Comment 13 WebKit Commit Bot 2014-09-25 23:24:08 PDT
Comment on attachment 238531 [details]
Patch

Clearing flags on attachment: 238531

Committed r173997: <http://trac.webkit.org/changeset/173997>
Comment 14 WebKit Commit Bot 2014-09-25 23:24:13 PDT
All reviewed patches have been landed.  Closing bug.