Bug 138090 – [GTK] Minibrowser: Add support for zoom using Control Key + Mouse scroll

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

[Tanay]

Supporting the following behaviour in GTK Minibrowser
1) Control key + Mouse scroll up -- Zoom in
2) Control key + Mouse scroll down -- Zoom out

[Tanay]

Created attachment 240474 [details]
Patch

[Gyuyoung Kim]

CC'ing Martin

[Martin Robinson]

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

[Tanay]

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

[Philippe Normand]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Tanay]

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

[Carlos Garcia Campos]

(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

[Tanay]

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

[Tanay]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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)

[Tanay]

Created attachment 241552 [details]
Patch

[Tanay]

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

[Tanay]

Reminder to review

[Carlos Garcia Campos]

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.

[Tanay]

Created attachment 242191 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 242191 [details]
Patch

Ok, thanks.

[WebKit Commit Bot]

Comment on attachment 242191 [details]
Patch

Clearing flags on attachment: 242191

Committed r176539: <http://trac.webkit.org/changeset/176539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.