Bug 138090 - [GTK] Minibrowser: Add support for zoom using Control Key + Mouse scroll
Summary: [GTK] Minibrowser: Add support for zoom using Control Key + Mouse scroll
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-10-27 04:09 PDT by Tanay
Modified: 2014-11-25 05:47 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.57 KB, patch)
2014-10-27 04:14 PDT, Tanay
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2014-11-13 21:21 PST, Tanay
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2014-11-25 02:27 PST, Tanay
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tanay 2014-10-27 04:09:56 PDT
Supporting the following behaviour in GTK Minibrowser
1) Control key + Mouse scroll up -- Zoom in
2) Control key + Mouse scroll down -- Zoom out
Comment 1 Tanay 2014-10-27 04:14:30 PDT
Created attachment 240474 [details]
Patch
Comment 2 Gyuyoung Kim 2014-11-12 16:23:32 PST
CC'ing Martin
Comment 3 Martin Robinson 2014-11-12 16:30:07 PST
Is there already a way to zoom in MiniBrowser? I don't think we should be adding features just for the sake of adding features. It should be "mini."
Comment 4 Tanay 2014-11-12 21:54:29 PST
(In reply to comment #3)
> Is there already a way to zoom in MiniBrowser? I don't think we should be
> adding features just for the sake of adding features. It should be "mini."

Yes, there is zoom support in the Minibrowser via keyboard & softkeys. However, scroll with mouse to zoom seems more intuitive way to zoom and is a functionality present in all major browsers, hence it would be a good addition to GTK. 
Also it gives a good way to use/test gdk event API's for scroll direction and state.
Comment 5 Philippe Normand 2014-11-13 00:08:38 PST
If you want to test GTK features in a full blown WebKitGTK browser I suggest one called Epiphany.

MiniBrowser doesn't need new features, it should remain... minimal.
Comment 6 Carlos Garcia Campos 2014-11-13 00:40:11 PST
Comment on attachment 240474 [details]
Patch

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

I don't see the problem of having this in MiniBrowser, I have some comments about the patch, though.

> Tools/MiniBrowser/gtk/BrowserWindow.c:447
> +static void mouseZoomCallback(BrowserWindow *window, const GdkEvent *event)

This is actually the scroll-event callback, that is used to implement zoom, I would call this scrollEventCallback instead. The function is boolean not void.

> Tools/MiniBrowser/gtk/BrowserWindow.c:454
> +    gdk_event_get_scroll_direction(event, &direction);
> +    gdk_event_get_state(event, &state);

You could simply cast to GdkEventScroll or event change the function parameter to receive a GdkEventScroll directly, and then you can use event->state and event->direction directly.

> Tools/MiniBrowser/gtk/BrowserWindow.c:456
> +    if (direction == GDK_SCROLL_UP && state == GDK_CONTROL_MASK) {

Instead of checking the state all the time, we can just return early when the state != GDK_CONTROL_MASK.

> Tools/MiniBrowser/gtk/BrowserWindow.c:459
> +            zoomLevel = webkit_web_view_get_zoom_level(window->webView) * zoomStep;
> +            webkit_web_view_set_zoom_level(window->webView, zoomLevel);

This is duplicated in zoomInCallback, we could move this to a function browser_window_zoom_in() for example and call it from here and zoomInCallback()

> Tools/MiniBrowser/gtk/BrowserWindow.c:466
> +            zoomLevel = webkit_web_view_get_zoom_level(window->webView) / zoomStep;
> +            webkit_web_view_set_zoom_level(window->webView, zoomLevel);

And the same here.

> Tools/MiniBrowser/gtk/BrowserWindow.c:468
> +    }

You should probably handle the other direction, including GDK_SCROLL_SMOOTH, a switch would look much better than ifs, I think

> Tools/MiniBrowser/gtk/BrowserWindow.c:469
> +}

You should return TRUE when you have handled the event and FALSE otherwise to propagate the event.

> Tools/MiniBrowser/gtk/BrowserWindow.c:790
> +    g_signal_connect(window, "scroll_event", G_CALLBACK(mouseZoomCallback), window);

scroll_event -> scroll-event. It doesn't make sense to pass the instance pointer as user data and you are not even handling the user data in the callback. I think it would be better if you implement the scroll_event vmethod of GtkWidgetClass instead of connecting to the signal.
Comment 7 Carlos Garcia Campos 2014-11-13 02:55:56 PST
Comment on attachment 240474 [details]
Patch

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

>> Tools/MiniBrowser/gtk/BrowserWindow.c:790
>> +    g_signal_connect(window, "scroll_event", G_CALLBACK(mouseZoomCallback), window);
> 
> scroll_event -> scroll-event. It doesn't make sense to pass the instance pointer as user data and you are not even handling the user data in the callback. I think it would be better if you implement the scroll_event vmethod of GtkWidgetClass instead of connecting to the signal.

hmm, but we don't want to connect to the window scroll event, but to the web view scroll event instead.
Comment 8 Tanay 2014-11-13 03:10:53 PST
(In reply to comment #7)
> Comment on attachment 240474 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240474&action=review
> 
> >> Tools/MiniBrowser/gtk/BrowserWindow.c:790
> >> +    g_signal_connect(window, "scroll_event", G_CALLBACK(mouseZoomCallback), window);
> > 
> > scroll_event -> scroll-event. It doesn't make sense to pass the instance pointer as user data and you are not even handling the user data in the callback. I think it would be better if you implement the scroll_event vmethod of GtkWidgetClass instead of connecting to the signal.
> 
> hmm, but we don't want to connect to the window scroll event, but to the web
> view scroll event instead.

We are connecting to the scroll event for the window. The zoom API webkit_webview_set_zoom_level() acts on the window->webview. 
As per your review comment I am using the GDKEventScroll so I do not need to pass window as user data and will pass NULL instead. I will incorporate the other review comments and upload a patch.
Comment 9 Carlos Garcia Campos 2014-11-13 03:14:24 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 240474 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240474&action=review
> > 
> > >> Tools/MiniBrowser/gtk/BrowserWindow.c:790
> > >> +    g_signal_connect(window, "scroll_event", G_CALLBACK(mouseZoomCallback), window);
> > > 
> > > scroll_event -> scroll-event. It doesn't make sense to pass the instance pointer as user data and you are not even handling the user data in the callback. I think it would be better if you implement the scroll_event vmethod of GtkWidgetClass instead of connecting to the signal.
> > 
> > hmm, but we don't want to connect to the window scroll event, but to the web
> > view scroll event instead.
> 
> We are connecting to the scroll event for the window. The zoom API
> webkit_webview_set_zoom_level() acts on the window->webview. 
> As per your review comment I am using the GDKEventScroll so I do not need to
> pass window as user data and will pass NULL instead. I will incorporate the
> other review comments and upload a patch.

But you don't want to zoom the view when scrolling in the toolbar for example
Comment 10 Tanay 2014-11-13 03:50:20 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 240474 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=240474&action=review
> > > 
> > > >> Tools/MiniBrowser/gtk/BrowserWindow.c:790
> > > >> +    g_signal_connect(window, "scroll_event", G_CALLBACK(mouseZoomCallback), window);
> > > > 
> > > > scroll_event -> scroll-event. It doesn't make sense to pass the instance pointer as user data and you are not even handling the user data in the callback. I think it would be better if you implement the scroll_event vmethod of GtkWidgetClass instead of connecting to the signal.
> > > 
> > > hmm, but we don't want to connect to the window scroll event, but to the web
> > > view scroll event instead.
> > 
> > We are connecting to the scroll event for the window. The zoom API
> > webkit_webview_set_zoom_level() acts on the window->webview. 
> > As per your review comment I am using the GDKEventScroll so I do not need to
> > pass window as user data and will pass NULL instead. I will incorporate the
> > other review comments and upload a patch.
> 
> But you don't want to zoom the view when scrolling in the toolbar for example

I just tried to connect to the WebKitWebView 'scroll-event' signal and I am only getting a event->direction value of '4' for up and down scroll. This is corresponding to GDK_SMOOTH_SCROLL.
Comment 11 Tanay 2014-11-13 04:04:06 PST
Since I recieve only one scroll direction value in this case (WebKitWebView) it is not possible to differentiate between the events. Connecting to window scroll events does not cause any issues as per my testing. Please do let me know your inputs.
Comment 12 Carlos Garcia Campos 2014-11-13 05:04:44 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > Comment on attachment 240474 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=240474&action=review
> > > > 
> > > > >> Tools/MiniBrowser/gtk/BrowserWindow.c:790
> > > > >> +    g_signal_connect(window, "scroll_event", G_CALLBACK(mouseZoomCallback), window);
> > > > > 
> > > > > scroll_event -> scroll-event. It doesn't make sense to pass the instance pointer as user data and you are not even handling the user data in the callback. I think it would be better if you implement the scroll_event vmethod of GtkWidgetClass instead of connecting to the signal.
> > > > 
> > > > hmm, but we don't want to connect to the window scroll event, but to the web
> > > > view scroll event instead.
> > > 
> > > We are connecting to the scroll event for the window. The zoom API
> > > webkit_webview_set_zoom_level() acts on the window->webview. 
> > > As per your review comment I am using the GDKEventScroll so I do not need to
> > > pass window as user data and will pass NULL instead. I will incorporate the
> > > other review comments and upload a patch.
> > 
> > But you don't want to zoom the view when scrolling in the toolbar for example
> 
> I just tried to connect to the WebKitWebView 'scroll-event' signal and I am
> only getting a event->direction value of '4' for up and down scroll. This is
> corresponding to GDK_SMOOTH_SCROLL.

Yes, that's because the web view has the GDK_SMOOTH_SCROLL_MASK in its event mask. You should handle GDK_SMOOTH_SCROLL as I suggested previously.
Comment 13 Carlos Garcia Campos 2014-11-13 05:05:25 PST
(In reply to comment #11)
> Since I recieve only one scroll direction value in this case (WebKitWebView)
> it is not possible to differentiate between the events. Connecting to window
> scroll events does not cause any issues as per my testing. Please do let me
> know your inputs.

Does not cause any issues doesn't mean it's correct.
Comment 14 Carlos Garcia Campos 2014-11-13 05:09:21 PST
Comment on attachment 240474 [details]
Patch

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

>> Tools/MiniBrowser/gtk/BrowserWindow.c:456
>> +    if (direction == GDK_SCROLL_UP && state == GDK_CONTROL_MASK) {
> 
> Instead of checking the state all the time, we can just return early when the state != GDK_CONTROL_MASK.

The state comparison is also wrong, since state is a bitmask you have to compare with the mask (gtk_accelerator_get_default_mod_mask()) if (state & mask == GDK_CONTROL_MASK)
Comment 15 Tanay 2014-11-13 21:21:37 PST
Created attachment 241552 [details]
Patch
Comment 16 Tanay 2014-11-13 21:26:42 PST
Thanks for the inputs.
Uploaded patch with review comments and changes for: 
1) Connecting to WebView scroll event
2) Using the GdkEventScroll -> delta_y values to determine scroll direction
Comment 17 Tanay 2014-11-24 22:59:21 PST
Reminder to review
Comment 18 Carlos Garcia Campos 2014-11-24 23:38:55 PST
Comment on attachment 241552 [details]
Patch

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

> Tools/MiniBrowser/gtk/BrowserWindow.c:448
> +static void browser_window_zoom_in(BrowserWindow *window)

Private methods should follow the WebKit coding style, this should be browserWindowZoomIn

> Tools/MiniBrowser/gtk/BrowserWindow.c:456
> +static void browser_window_zoom_out(BrowserWindow *window)

Ditto.

> Tools/MiniBrowser/gtk/BrowserWindow.c:470
> +        return isEventHandled;

This is the only place where we return FALSE, so I think we don't need the local variable, simply return FALSE here directly and TRUE after the if

> Tools/MiniBrowser/gtk/BrowserWindow.c:480
> +    return isEventHandled;

This is always TRUE at this point due to the early return. But the event was actually handled only if the zoom happened. You could make zoom in/out functions bool and return False when the view can't zoom, and return that value directly here.
Comment 19 Tanay 2014-11-25 02:27:32 PST
Created attachment 242191 [details]
Patch
Comment 20 Carlos Garcia Campos 2014-11-25 03:47:13 PST
Comment on attachment 242191 [details]
Patch

Ok, thanks.
Comment 21 WebKit Commit Bot 2014-11-25 05:47:52 PST
Comment on attachment 242191 [details]
Patch

Clearing flags on attachment: 242191

Committed r176539: <http://trac.webkit.org/changeset/176539>
Comment 22 WebKit Commit Bot 2014-11-25 05:47:57 PST
All reviewed patches have been landed.  Closing bug.