Bug 182174 - [GTK] Zooming gesture incorrectly uses scale instead of zoom
Summary: [GTK] Zooming gesture incorrectly uses scale instead of zoom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-26 09:49 PST by Jan-Michael Brummer
Modified: 2018-01-30 01:47 PST (History)
5 users (show)

See Also:


Attachments
Browser using scaling (208.85 KB, image/png)
2018-01-26 09:50 PST, Jan-Michael Brummer
no flags Details
Browser using zooming (292.89 KB, image/png)
2018-01-26 09:50 PST, Jan-Michael Brummer
no flags Details
Firefox after zooming (180.36 KB, image/png)
2018-01-26 09:52 PST, Jan-Michael Brummer
no flags Details
Patch changing scale to zoom (2.46 KB, patch)
2018-01-26 10:21 PST, Jan-Michael Brummer
no flags Details | Formatted Diff | Diff
Patch changing scale to zoom - V2 (2.65 KB, patch)
2018-01-29 08:40 PST, Jan-Michael Brummer
mcatanzaro: review+
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Patch changing scale to zoom - V3 (2.64 KB, patch)
2018-01-29 10:09 PST, Jan-Michael Brummer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan-Michael Brummer 2018-01-26 09:49:27 PST
Currently zooming gesture is implemented using pageScale. Unfortuantely this isn't the typical expectation of a user:
A user wants to zoom the page. Therefore page zoom must be used. Additionally this fixes the async of epiphany after
zooming through gesture and the zoom option within the hamburger menu. Otherwise the user doesn't have a possibility
to revert to the "default" zoom level.
Comment 1 Jan-Michael Brummer 2018-01-26 09:50:15 PST
Created attachment 332376 [details]
Browser using scaling
Comment 2 Jan-Michael Brummer 2018-01-26 09:50:38 PST
Created attachment 332377 [details]
Browser using zooming
Comment 3 Jan-Michael Brummer 2018-01-26 09:51:43 PST
Attached two image showing the difference between those two function. FWIW: In addition i will attach a image of firefox after zooming gesture.
Comment 4 Jan-Michael Brummer 2018-01-26 09:52:09 PST
Created attachment 332379 [details]
Firefox after zooming
Comment 5 Jan-Michael Brummer 2018-01-26 10:21:53 PST
Created attachment 332382 [details]
Patch changing scale to zoom
Comment 6 Michael Catanzaro 2018-01-26 10:42:27 PST
Comment on attachment 332382 [details]
Patch changing scale to zoom

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

> Source/WebKit/UIProcess/gtk/GestureController.cpp:258
> +    g_object_notify(G_OBJECT(m_page.viewWidget()), "zoom-level");

Notifying for another object's property feels pretty fragile. I guess if it's the only place outside WebKitWebView where the zoom level needs to be changed, it's not the end of the world. But can you use webkit_web_view_set_zoom_level() instead? Then the gesture would additionally respect the text-only zoom setting.
Comment 7 Jan-Michael Brummer 2018-01-29 08:40:29 PST
Created attachment 332545 [details]
Patch changing scale to zoom - V2

Use webkit_web_view_set_zoom_level ().
Comment 8 Michael Catanzaro 2018-01-29 09:55:16 PST
Comment on attachment 332545 [details]
Patch changing scale to zoom - V2

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

> Source/WebKit/UIProcess/gtk/GestureController.cpp:33
> +#include "WebKitWebViewPrivate.h"

Better to #include "WebKitWebView.h" since you don't need any of the private bits.
Comment 9 Jan-Michael Brummer 2018-01-29 10:09:14 PST
Created attachment 332549 [details]
Patch changing scale to zoom - V3
Comment 10 WebKit Commit Bot 2018-01-29 11:29:13 PST
Comment on attachment 332549 [details]
Patch changing scale to zoom - V3

Clearing flags on attachment: 332549

Committed r227744: <https://trac.webkit.org/changeset/227744>
Comment 11 WebKit Commit Bot 2018-01-29 11:29:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Carlos Garcia Campos 2018-01-30 01:47:48 PST
Comment on attachment 332549 [details]
Patch changing scale to zoom - V3

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

> Source/WebKit/UIProcess/gtk/GestureController.cpp:33
> +#include "WebKitWebView.h"

This is wrong. We shouldn't include WebKitWebView here, is kind of a layering violation, see below.

> Source/WebKit/UIProcess/gtk/GestureController.cpp:257
> +    webkit_web_view_set_zoom_level(WEBKIT_WEB_VIEW(m_page.viewWidget()), m_scale);

We can't assume viewWidget will be a WebKitWebView, it can be a WebKitWebViewBase. This will probably crash if you try to zoom the web inspector, for example. Anyway, I'll fix this as part of bug #182224.