Bug 18281 – [GTK] add functions to set/get the zoom level

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Marco Barisione]

WebKit gtk is currently missing an API to set/get the zoom level.

I'm going to write a patch to add this feature, using as source of inspiration the win port APIs.

[Marco Barisione]

Created attachment 20315 [details]
Add functions to handle full page/text-only zoom to WebKit gtk

The patch does not include documentation and the ChangeLog entry as I'm not sure if the added API is ok.

The functions names are copied from the ones in the win port, that are in turn a C++-ificated version of the ones in the mac port.

[Marco Barisione]

Created attachment 20316 [details]
Add zoom buttons to the gtk launcher

The first three buttons are for full page zoom, the other three are for text only zoom.

[Marco Barisione]

Created attachment 20330 [details]
Fix some problems with return types

I'm not sure that adding some many functions is the right thing. Full-page and text-only zoom are not meant to be used together, so why not having a property to control wether to use the first or the latter? This way we could have zoom_in instead of zoom_page_in and make_text_larger. The same would apply for the other functions.

[Christian Dywan]

Comment on attachment 20330 [details]
Fix some problems with return types

Nice to see some progress here. Now for my first impression:

>+static const gfloat minimumZoomMultiplier = 0.5f;
>+static const gfloat maximumZoomMultiplier = 3.0f;
>+static const gfloat zoomMultiplierRatio = 1.2f;

Where do these values come from? I would hesitate hardcoding anything like that without good reasing since this what determines good choices can vary much depending on the device and display in use.

>+WEBKIT_API gfloat
>+webkit_web_view_get_text_size_multiplier (WebKitWebView* webView);
>+
>+WEBKIT_API void
>+webkit_web_view_set_text_size_multiplier (WebKitWebView* webView, gfloat multiplier);

I don't recognize from 'multiplier' anywhere else. 'scale' is familiar term to describe this.

>+WEBKIT_API gboolean
>+webkit_web_view_can_make_text_larger (WebKitWebView* webView);
>+
>+WEBKIT_API void
>+webkit_web_view_make_text_larger (WebKitWebView* webView);
>+
>+WEBKIT_API gboolean
>+webkit_web_view_can_make_text_smaller (WebKitWebView* webView);
>+
>+WEBKIT_API void
>+webkit_web_view_make_text_smaller (WebKitWebView* webView);
>+
>+WEBKIT_API gboolean
>+webkit_web_view_can_make_text_standard_size (WebKitWebView* webView);
>+
>+WEBKIT_API void
>+webkit_web_view_make_text_standard_size (WebKitWebView* webView);

This terminology makes no sense other than in an OS X application. However reseting the text size multiplier seems useful functionality wise.

[Marco Barisione]

Full page zoom can be used instead of text-only zoom, not together. So I propose to add a boolean "full-page-zoom" property (or "text-zoom", I don't have better idea atm). If full-page-zoom is set to TRUE then everything is zoomed, else only text size is modified.

Then we need a "zoom-level" float property to set/get the zoom level (with the corresponding webkit_web_view_[gs]et_zoom_level() functions).

webkit_web_view_zoom_in() and webkit_web_view_zoom_out() could be useful too, but what should the scaling factor be? Other ports use 1.2 but hardcoded values are bad. Adding a "zoom-factor" defaulting to 1.2 could solve the problem but I'm not sure we really need to add another property for this.

[Christian Dywan]

(In reply to comment #5)
> Full page zoom can be used instead of text-only zoom, not together. So I
> propose to add a boolean "full-page-zoom" property (or "text-zoom", I don't
> have better idea atm). If full-page-zoom is set to TRUE then everything is
> zoomed, else only text size is modified.

Why is that? I don't concider the two options mutually exclusive from a user's point of view. Of course if this just doesn't work out for technical reasons we can't do much about it.

> Then we need a "zoom-level" float property to set/get the zoom level (with the
> corresponding webkit_web_view_[gs]et_zoom_level() functions).
> 
> webkit_web_view_zoom_in() and webkit_web_view_zoom_out() could be useful too,
> but what should the scaling factor be? Other ports use 1.2 but hardcoded values
> are bad. Adding a "zoom-factor" defaulting to 1.2 could solve the problem but
> I'm not sure we really need to add another property for this.

I agree it must be possible to adjust this value from an application. I think zoom-factor should actually be a property of WebKitWebSettings. I suspect an application global value is the proper way to handle this.

[Marco Barisione]

(In reply to comment #6)
> Why is that? I don't concider the two options mutually exclusive from a user's
> point of view. Of course if this just doesn't work out for technical reasons we
> can't do much about it.

WebCore doesn't allow them to be used together, I don't know if it's a just limitation or if there are good reasons. My idea is that the user should not some UI to handle both and it would be very confusing: "hey why do I have to separate zoom in buttons?".
IMHO if the user always wants bigger fonts it can set them, otherwise he sould just get full page zoom (at least when webkit is used for a web browser).

[Christian Dywan]

(In reply to comment #7)
> (In reply to comment #6)
> > Why is that? I don't concider the two options mutually exclusive from a user's
> > point of view. Of course if this just doesn't work out for technical reasons we
> > can't do much about it.
> 
> WebCore doesn't allow them to be used together, I don't know if it's a just
> limitation or if there are good reasons. My idea is that the user should not
> some UI to handle both and it would be very confusing: "hey why do I have to
> separate zoom in buttons?".
> IMHO if the user always wants bigger fonts it can set them, otherwise he sould
> just get full page zoom (at least when webkit is used for a web browser).

Agreed, the interface for the two means of zooming could be a problem. It might be desirable to get one or two opinions from the mac port crew on this. Or even any more opinions to have a clearer vision before deciding this.

[Marco Barisione]

(In reply to comment #8)
> > WebCore doesn't allow them to be used together, I don't know if it's a just
> > limitation or if there are good reasons. My idea is that the user should not
> > some UI to handle both and it would be very confusing: "hey why do I have to
> > separate zoom in buttons?".
> > IMHO if the user always wants bigger fonts it can set them, otherwise he sould
> > just get full page zoom (at least when webkit is used for a web browser).
> 
> Agreed, the interface for the two means of zooming could be a problem. It might
> be desirable to get one or two opinions from the mac port crew on this. Or even
> any more opinions to have a clearer vision before deciding this.

I talked a bit with some guys working on the mac port and:
1 - WebCore doesn't allow to have full-page zoom and text-only zoom at the same time
2 - They kept two separate sets of APIs for the two modes for backward compatibility.

So I will modify the patch to have a single set of function and a full-page-zoom property (suggestions for better names?)

[Marco Barisione]

Created attachment 20475 [details]
Add functions and properties to handle full-page/text-only zoom APIs to WebKit gtk

The new API as discussed on IRC (zoomFactor is still hard-coded). I think this new API is much nicer than the old one. What do you think?

I think that the zoom level becomes too big/small too quickly. What about increasing the zoom level with + 0.1 instead of multiplying it with 1.2?

[Marco Barisione]

Created attachment 20476 [details]
Add zoom buttons to GtkLauncher

[Michael Emmel]

Created attachment 20717 [details]
Alternative Zoom patch


Here is my version of the zoom patch for comparison.

[Christian Dywan]

Comment on attachment 20475 [details]
Add functions and properties to handle full-page/text-only zoom APIs to WebKit gtk

>+static const gfloat minimumZoomMultiplier = 0.5f;
>+static const gfloat maximumZoomMultiplier = 3.0f;
>+static const gfloat zoomMultiplierRatio = 1.2f;

I still think these should be exposed in the API. For the sake of this bug I would be willing to add them to WebKitWebSettings myself, after this is landed.

>@@ -95,7 +102,9 @@ enum {
>     PROP_PASTE_TARGET_LIST,
>     PROP_EDITABLE,
>     PROP_SETTINGS,
>-    PROP_TRANSPARENT
>+    PROP_TRANSPARENT,
>+    PROP_ZOOM_LEVEL,
>+    PROP_TEXT_ONLY_ZOOM

As for "text-only-zoom" I have two questions:

1. What about calling this "zoom-text-only", to have it align visually with "zoom-level"?

2. Would it make more sense to move this property to WebKitWebSettings? I do not know the answer myself. If anyone has an actual use case I would most appreciate that.

>+    g_object_class_install_property(objectClass, PROP_ZOOM_LEVEL,
>+                                    g_param_spec_float("zoom-level",
>+                                                       "Zoom level",
>+                                                       "The level of zoom of the content",
>+                                                       G_MINFLOAT,
>+                                                       G_MAXFLOAT,
>+                                                       1,
>+                                                       WEBKIT_PARAM_READWRITE));

This is not your fault. But I'm wondering why WebView has a different style when it comes to property installation. I was certainly tempted to criticize you for that =)

[Alp Toker]

Comment on attachment 20717 [details]
Alternative Zoom patch

Thanks for the patch.

We discussed this and we'll go with Barisione's patch, though he is taking an idea or two from yours.

[Marco Barisione]

Created attachment 21360 [details]
Add zoom functions to WebKit gtk

Patch updated with the changes discussed here and on IRC.

I'm not sure if zoom-text-only should be a global settings or not. I think that someone could want different zooming modes for different pages.

Scaled images are ugly because of bug #19266, but Pierre-Luc has a patch for it.

[Pierre-Luc Beaudoin]

Comment on attachment 20717 [details]
Alternative Zoom patch

Obsolete upon request from Marco Barisione

[Marco Barisione]

Created attachment 21407 [details]
Add zoom functions to WebKit gtk

Updated patch with documentation.

[Alp Toker]

Landed in r34249. Please examine the commit since it changes a few things:

Scale stepping in/out is now linear to avoid rounding errors and incorrect behaviour.

Function and property names have changed.

Documentation is cleaned up quite a bit.

WebSettings copying is fixed now to copy this setting too.

I think the new API is awesome ;-)

[Alp Toker]

Comment on attachment 21407 [details]
Add zoom functions to WebKit gtk

r+ with changes as mentioned earlier.