Bug 137775 – [GTK] Minibrowser : Add window fullscreen support for Minibrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Rohit]

Add support for window fullscreen toggling in Minibrowser using keyboard F11 key.

[Rohit]

Created attachment 239941 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 239941 [details]
Patch

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

> Tools/MiniBrowser/gtk/BrowserWindow.c:575
> +    if (!window->fullScreenIsEnabled) {
> +        gtk_window_fullscreen(GTK_WINDOW(window));
> +        gtk_widget_hide(window->toolbar);
> +        window->fullScreenIsEnabled = TRUE;
> +    } else {
> +        gtk_window_unfullscreen(GTK_WINDOW(window));
> +        gtk_widget_show(window->toolbar);
> +        window->fullScreenIsEnabled = FALSE;
> +    }

We already have code to enter/leave fullscreen when requested by wk, maybe we can reuse that code to make it work also when requested by the user, because there are some inconsistencies. The other code shows a message explaining the user how to leave fullscreen mode, and the keybindings are f or ESC, not F11. Also the other code takes care of showing/hide the findbar.

[Rohit]

(In reply to comment #2)
> Comment on attachment 239941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > +    if (!window->fullScreenIsEnabled) {
> > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > +        gtk_widget_hide(window->toolbar);
> > +        window->fullScreenIsEnabled = TRUE;
> > +    } else {
> > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > +        gtk_widget_show(window->toolbar);
> > +        window->fullScreenIsEnabled = FALSE;
> > +    }
> 
> We already have code to enter/leave fullscreen when requested by wk, maybe
> we can reuse that code to make it work also when requested by the user,
> because there are some inconsistencies. The other code shows a message
> explaining the user how to leave fullscreen mode, and the keybindings are f
> or ESC, not F11. Also the other code takes care of showing/hide the findbar.

I think the code to enter/leave fullscreen is for HTML element/webview fullscreen and not for the minibrowser window itself. We don't need message and title for window to be displayed on fullscreen in this case. Most of the browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 239941 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > 
> > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > +    if (!window->fullScreenIsEnabled) {
> > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > +        gtk_widget_hide(window->toolbar);
> > > +        window->fullScreenIsEnabled = TRUE;
> > > +    } else {
> > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > +        gtk_widget_show(window->toolbar);
> > > +        window->fullScreenIsEnabled = FALSE;
> > > +    }
> > 
> > We already have code to enter/leave fullscreen when requested by wk, maybe
> > we can reuse that code to make it work also when requested by the user,
> > because there are some inconsistencies. The other code shows a message
> > explaining the user how to leave fullscreen mode, and the keybindings are f
> > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> 
> I think the code to enter/leave fullscreen is for HTML element/webview
> fullscreen and not for the minibrowser window itself.

Right.

> We don't need message
> and title for window to be displayed on fullscreen in this case. Most of the
> browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.

Yes, Epiphany also uses F11, but also shows a message. In any case, my point is that the code that enters/leaves fullscreen could be common in both cases. Move the common logic to a function, and call it from both places.

[Rohit]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 239941 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > 
> > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > +    if (!window->fullScreenIsEnabled) {
> > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > +        gtk_widget_hide(window->toolbar);
> > > > +        window->fullScreenIsEnabled = TRUE;
> > > > +    } else {
> > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > +        gtk_widget_show(window->toolbar);
> > > > +        window->fullScreenIsEnabled = FALSE;
> > > > +    }
> > > 
> > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > we can reuse that code to make it work also when requested by the user,
> > > because there are some inconsistencies. The other code shows a message
> > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > 
> > I think the code to enter/leave fullscreen is for HTML element/webview
> > fullscreen and not for the minibrowser window itself.
> 
> Right.
> 
> > We don't need message
> > and title for window to be displayed on fullscreen in this case. Most of the
> > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> 
> Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> is that the code that enters/leaves fullscreen could be common in both
> cases. Move the common logic to a function, and call it from both places.

There is no common code in BrowserWindow.c file for webkitView and window fullscreen except findbar hide. Are you suggesting to implement it in WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC and F key handle?

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 239941 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > 
> > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > +        gtk_widget_hide(window->toolbar);
> > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > +    } else {
> > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > +        gtk_widget_show(window->toolbar);
> > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > +    }
> > > > 
> > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > we can reuse that code to make it work also when requested by the user,
> > > > because there are some inconsistencies. The other code shows a message
> > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > 
> > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > fullscreen and not for the minibrowser window itself.
> > 
> > Right.
> > 
> > > We don't need message
> > > and title for window to be displayed on fullscreen in this case. Most of the
> > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > 
> > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > is that the code that enters/leaves fullscreen could be common in both
> > cases. Move the common logic to a function, and call it from both places.
> 
> There is no common code in BrowserWindow.c file for webkitView and window
> fullscreen except findbar hide. Are you suggesting to implement it in
> WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> and F key handle?

Oh, you are right, sorry, I thought we were also doing the fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by the web view. So, yes, it's enough to move the searchbar visibility code to their own functions and call those from both places. Something like browserWindowHideSearchBarForFullscreen() and browserWindowShowSearchBarAfterFullscreen(), for example

[Rohit]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > Comment on attachment 239941 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > > 
> > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > > +        gtk_widget_hide(window->toolbar);
> > > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > > +    } else {
> > > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > > +        gtk_widget_show(window->toolbar);
> > > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > > +    }
> > > > > 
> > > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > > we can reuse that code to make it work also when requested by the user,
> > > > > because there are some inconsistencies. The other code shows a message
> > > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > > 
> > > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > > fullscreen and not for the minibrowser window itself.
> > > 
> > > Right.
> > > 
> > > > We don't need message
> > > > and title for window to be displayed on fullscreen in this case. Most of the
> > > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > > 
> > > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > > is that the code that enters/leaves fullscreen could be common in both
> > > cases. Move the common logic to a function, and call it from both places.
> > 
> > There is no common code in BrowserWindow.c file for webkitView and window
> > fullscreen except findbar hide. Are you suggesting to implement it in
> > WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> > and F key handle?
> 
> Oh, you are right, sorry, I thought we were also doing the
> fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by
> the web view. So, yes, it's enough to move the searchbar visibility code to
> their own functions and call those from both places. Something like
> browserWindowHideSearchBarForFullscreen() and
> browserWindowShowSearchBarAfterFullscreen(), for example

Do you mean to say hide 'toolbar' in place of searchbar? We dont need to hide 'searchbar' as this beahviour is not needed in case of window fullscreen and needed in case of webview fullscreen.(checked in firefox and chrome).

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > (In reply to comment #2)
> > > > > > Comment on attachment 239941 [details]
> > > > > > Patch
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > > > 
> > > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > > > +        gtk_widget_hide(window->toolbar);
> > > > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > > > +    } else {
> > > > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > > > +        gtk_widget_show(window->toolbar);
> > > > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > > > +    }
> > > > > > 
> > > > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > > > we can reuse that code to make it work also when requested by the user,
> > > > > > because there are some inconsistencies. The other code shows a message
> > > > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > > > 
> > > > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > > > fullscreen and not for the minibrowser window itself.
> > > > 
> > > > Right.
> > > > 
> > > > > We don't need message
> > > > > and title for window to be displayed on fullscreen in this case. Most of the
> > > > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > > > 
> > > > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > > > is that the code that enters/leaves fullscreen could be common in both
> > > > cases. Move the common logic to a function, and call it from both places.
> > > 
> > > There is no common code in BrowserWindow.c file for webkitView and window
> > > fullscreen except findbar hide. Are you suggesting to implement it in
> > > WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> > > and F key handle?
> > 
> > Oh, you are right, sorry, I thought we were also doing the
> > fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by
> > the web view. So, yes, it's enough to move the searchbar visibility code to
> > their own functions and call those from both places. Something like
> > browserWindowHideSearchBarForFullscreen() and
> > browserWindowShowSearchBarAfterFullscreen(), for example
> 
> Do you mean to say hide 'toolbar' in place of searchbar? We dont need to
> hide 'searchbar' as this beahviour is not needed in case of window
> fullscreen and needed in case of webview fullscreen.(checked in firefox and
> chrome).

I don't know why we hide the searchbar when entering fs TBH, I proposed it for consistency.

[Rohit]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (In reply to comment #3)
> > > > > > (In reply to comment #2)
> > > > > > > Comment on attachment 239941 [details]
> > > > > > > Patch
> > > > > > > 
> > > > > > > View in context:
> > > > > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > > > > 
> > > > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > > > > +        gtk_widget_hide(window->toolbar);
> > > > > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > > > > +    } else {
> > > > > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > > > > +        gtk_widget_show(window->toolbar);
> > > > > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > > > > we can reuse that code to make it work also when requested by the user,
> > > > > > > because there are some inconsistencies. The other code shows a message
> > > > > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > > > > 
> > > > > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > > > > fullscreen and not for the minibrowser window itself.
> > > > > 
> > > > > Right.
> > > > > 
> > > > > > We don't need message
> > > > > > and title for window to be displayed on fullscreen in this case. Most of the
> > > > > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > > > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > > > > 
> > > > > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > > > > is that the code that enters/leaves fullscreen could be common in both
> > > > > cases. Move the common logic to a function, and call it from both places.
> > > > 
> > > > There is no common code in BrowserWindow.c file for webkitView and window
> > > > fullscreen except findbar hide. Are you suggesting to implement it in
> > > > WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> > > > and F key handle?
> > > 
> > > Oh, you are right, sorry, I thought we were also doing the
> > > fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by
> > > the web view. So, yes, it's enough to move the searchbar visibility code to
> > > their own functions and call those from both places. Something like
> > > browserWindowHideSearchBarForFullscreen() and
> > > browserWindowShowSearchBarAfterFullscreen(), for example
> > 
> > Do you mean to say hide 'toolbar' in place of searchbar? We dont need to
> > hide 'searchbar' as this beahviour is not needed in case of window
> > fullscreen and needed in case of webview fullscreen.(checked in firefox and
> > chrome).
> 
> I don't know why we hide the searchbar when entering fs TBH, I proposed it
> for consistency.

I think we need to hide searchbar while entering fs because otherwise searchbar would be visible on top of fullscreen element.

[WebKit Commit Bot]

Comment on attachment 239941 [details]
Patch

Clearing flags on attachment: 239941

Committed r175371: <http://trac.webkit.org/changeset/175371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.