Bug 99285 - [EFL][WK2] Add support for a zoom level setting to MiniBrowser
Summary: [EFL][WK2] Add support for a zoom level setting to MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: KyungTae Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-14 20:07 PDT by KyungTae Kim
Modified: 2012-12-05 23:13 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.98 KB, patch)
2012-10-14 21:34 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.36 KB, patch)
2012-10-15 00:32 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2012-11-25 19:25 PST, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2012-12-02 22:52 PST, KyungTae Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KyungTae Kim 2012-10-14 20:07:08 PDT
EFL MiniBrowser does not currently support zoom-in, zoom-out.
Comment 1 KyungTae Kim 2012-10-14 21:34:55 PDT
Created attachment 168621 [details]
Patch
Comment 2 Gyuyoung Kim 2012-10-14 21:39:45 PDT
Comment on attachment 168621 [details]
Patch

Attachment 168621 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14296569
Comment 3 Chris Dumez 2012-10-14 21:49:18 PDT
Comment on attachment 168621 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:194
> +    } else if (!strcmp(ev->key, "F7")) {

Can you check which shortcut is used by browsers usually and use the same? It is confusing to use different shortcuts.

> Tools/MiniBrowser/efl/main.c:199
> +        zoom_level_set(obj, currentZoomLevel + 1);

There is usually a shortcut to reset the zoom level to its default value as well.
Comment 4 KyungTae Kim 2012-10-14 22:07:47 PDT
Christophe Dumez: 
I assigned 'F7' and 'F8' because the EWebLauncher uses them as zoom-in, zoom-out.

In many browsers, the 'Ctrl +' and 'Ctrl -' are used for zoom-in and zoom-out, and the 'Ctrl 0' is used for resetting zoom level.

I can add them, but I think 'F7' and 'F8' are also needed because the consistency with EWebLauncher is important, too.
Comment 5 Chris Dumez 2012-10-14 22:46:30 PDT
(In reply to comment #4)
> Christophe Dumez: 
> I assigned 'F7' and 'F8' because the EWebLauncher uses them as zoom-in, zoom-out.
> 
> In many browsers, the 'Ctrl +' and 'Ctrl -' are used for zoom-in and zoom-out, and the 'Ctrl 0' is used for resetting zoom level.
> 
> I can add them, but I think 'F7' and 'F8' are also needed because the consistency with EWebLauncher is important, too.

Then I think EWebLauncher should be fixed as well. We don't have any menu in the UI to do this, only keyboard shortcuts. If we use non-standard keyboard shortcuts then people don't know that they can zoom in/out.

I already made a similar patch recently to use CTRL+n to open a new window instead of Fx. People are used to some shortcuts. I don't think we should try to be compatible here with EWebLauncher because I don't think it was done properly in EWebLauncher. As I said before, keeping compatibility with WK1 is a good idea, unless we can do better. And here, I believe we can do better.
Comment 6 Ryuan Choi 2012-10-14 23:55:32 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Christophe Dumez: 
> > I assigned 'F7' and 'F8' because the EWebLauncher uses them as zoom-in, zoom-out.
> > 
> > In many browsers, the 'Ctrl +' and 'Ctrl -' are used for zoom-in and zoom-out, and the 'Ctrl 0' is used for resetting zoom level.
> > 
> > I can add them, but I think 'F7' and 'F8' are also needed because the consistency with EWebLauncher is important, too.
> 
> Then I think EWebLauncher should be fixed as well. We don't have any menu in the UI to do this, only keyboard shortcuts. If we use non-standard keyboard shortcuts then people don't know that they can zoom in/out.
> 
> I already made a similar patch recently to use CTRL+n to open a new window instead of Fx. People are used to some shortcuts. I don't think we should try to be compatible here with EWebLauncher because I don't think it was done properly in EWebLauncher. As I said before, keeping compatibility with WK1 is a good idea, unless we can do better. And here, I believe we can do better.

I think that we can decide it,
because I believe that almost users of EWebLauncher would monitor the changes.

ctrl +/- looks better to me.
Comment 7 Ryuan Choi 2012-10-15 00:04:52 PDT
Comment on attachment 168621 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:68
> +    Evas_Coord ox, oy, mx, my, cx, cy;
> +    evas_pointer_canvas_xy_get(evas_object_evas_get(webview), &mx, &my);
> +    evas_object_geometry_get(webview, &ox, &oy, NULL, NULL);
> +    cx = mx - ox;
> +    cy = my - oy;
> +
> +    Eina_Bool result = ewk_view_scale_set(webview, factor, cx, cy);

By the way, is it correct?

I think that we can not use ewk_view_scale_set well.
At least, cx,cy should be a contents coordination.

But, we did not contribute the APIs related to scroll behavior.

I hope that we land this after getting correct behavior.
Comment 8 KyungTae Kim 2012-10-15 00:32:04 PDT
Created attachment 168638 [details]
Patch
Comment 9 Gyuyoung Kim 2012-10-15 00:54:15 PDT
Comment on attachment 168638 [details]
Patch

Attachment 168638 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14298639
Comment 10 Yael 2012-10-18 13:47:57 PDT
I don't think we should go forward with this method of zooming.
Most mobile browsers zoom via gestures such as pinch or double-tap, and not via a setting or shortcut.
What do you think?
Comment 11 Gyuyoung Kim 2012-10-18 19:09:39 PDT
(In reply to comment #10)
> I don't think we should go forward with this method of zooming.
> Most mobile browsers zoom via gestures such as pinch or double-tap, and not via a setting or shortcut.
> What do you think?

Yes, I think so for mobile browser. However, is this for MiniBrowser which is simple test browser for PC ? I think this is useful only for test PC browser. Tizen mobile browser supports to zoom via gestures, double-tap and so on.
Comment 12 Yael 2012-10-19 05:29:28 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > I don't think we should go forward with this method of zooming.
> > Most mobile browsers zoom via gestures such as pinch or double-tap, and not via a setting or shortcut.
> > What do you think?
> 
> Yes, I think so for mobile browser. However, is this for MiniBrowser which is simple test browser for PC ? I think this is useful only for test PC browser. Tizen mobile browser supports to zoom via gestures, double-tap and so on.

Sorry, what I meant to ask is if we want to keep MiniBrowser as test desktop browser, or make it more like mobile browser. That is what Qt did with their MiniBrowser and I thought we wanted to go in the same direction. (Yes they also have a desktop browser which is less maintained ).
Comment 13 Gyuyoung Kim 2012-11-24 18:20:45 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > I don't think we should go forward with this method of zooming.
> > > Most mobile browsers zoom via gestures such as pinch or double-tap, and not via a setting or shortcut.
> > > What do you think?
> > 
> > Yes, I think so for mobile browser. However, is this for MiniBrowser which is simple test browser for PC ? I think this is useful only for test PC browser. Tizen mobile browser supports to zoom via gestures, double-tap and so on.
> 
> Sorry, what I meant to ask is if we want to keep MiniBrowser as test desktop browser, or make it more like mobile browser. That is what Qt did with their MiniBrowser and I thought we wanted to go in the same direction. (Yes they also have a desktop browser which is less maintained ).

I think it would be good if MiniBrowser supports shortcut keys to change zoom level when we test some websites in MiniBrowser. 

KyungTae, any update ?
Comment 14 KyungTae Kim 2012-11-25 19:25:35 PST
Created attachment 175914 [details]
Patch
Comment 15 Ryuan Choi 2012-11-25 20:37:21 PST
Comment on attachment 175914 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:51
> +static int currentZoomLevel = DEFAULT_ZOOM_LEVEL; 

When ewk_view is more than one, each view should have own zoom level
Comment 16 Chris Dumez 2012-11-25 22:02:32 PST
Comment on attachment 175914 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:50
> +#define DEFAULT_ZOOM_LEVEL 5 // set defeault zoom level to 100% (index 5 on zoomLevels)

"default", comments in WebKit usually end with a period (.)

>> Tools/MiniBrowser/efl/main.c:51
>> +static int currentZoomLevel = DEFAULT_ZOOM_LEVEL; 
> 
> When ewk_view is more than one, each view should have own zoom level

Yes, Ryuan is right. You should move this to _Browser_Window struct probably.

> Tools/MiniBrowser/efl/main.c:52
> +// the zoom values are chosen to be like in Mozilla Firefox 3

Usually ends with a period.

> Tools/MiniBrowser/efl/main.c:53
> +static int zoomLevels[] = {30, 50, 67, 80, 90, 100, 110, 120, 133, 150, 170, 200, 240, 300};

could be const.

> Tools/MiniBrowser/efl/main.c:65
> +    cx = mx - ox;

Maybe a comment to explain what you're doing here?
Comment 17 KyungTae Kim 2012-12-02 22:52:39 PST
Created attachment 177185 [details]
Patch
Comment 18 Gyuyoung Kim 2012-12-02 23:38:49 PST
Comment on attachment 177185 [details]
Patch

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

Looks good to me now.

> Tools/MiniBrowser/efl/main.c:59
> +    if (level < 0  || level >= sizeof(zoomLevels) / sizeof(int))

Don't you need to add a fail message ?
Comment 19 KyungTae Kim 2012-12-02 23:54:07 PST
Comment on attachment 177185 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:59
>> +    if (level < 0  || level >= sizeof(zoomLevels) / sizeof(int))
> 
> Don't you need to add a fail message ?

I thought it's OK because there's a message for zoom level setting, and user can see the fail because the zoom level was not changed.
If there needs a fail message, I think it needs to be added on on_key_down, but I didn't add it because it can make code too complicate.
Comment 20 WebKit Review Bot 2012-12-02 23:59:33 PST
Comment on attachment 177185 [details]
Patch

Clearing flags on attachment: 177185

Committed r136369: <http://trac.webkit.org/changeset/136369>
Comment 21 WebKit Review Bot 2012-12-02 23:59:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Kenneth Rohde Christiansen 2012-12-03 01:08:17 PST
Comment on attachment 177185 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:71
> +
> +    float factor = ((float) zoomLevels[level]) / 100.0;
> +    Evas_Coord ox, oy, mx, my, cx, cy;
> +    evas_pointer_canvas_xy_get(evas_object_evas_get(webview), &mx, &my); // Get current mouse position on window.
> +    evas_object_geometry_get(webview, &ox, &oy, NULL, NULL); // Get webview's position on window.
> +    cx = mx - ox; // current x position = mouse x position - webview x position
> +    cy = my - oy; // current y position = mouse y position - webview y position
> +
> +    Eina_Bool result = ewk_view_scale_set(webview, factor, cx, cy);
> +    return result;
> +}

This is pretty broken with interaction with viewport etc, and with deviceScaleFactor etc the way you get the position is probably also broken.

Anyway, I don't even get what you are trying to implement here? Some way to scale the content, or a scale factor (leaving things to fit into the viewport - that is what firefox does). If that is the case, using devicescalefactor instead makes more sense.

I dislike that people review these things without good explanation and without knowledge of the area.
Comment 23 Kenneth Rohde Christiansen 2012-12-03 02:08:26 PST
Argh! This even allows people to scale beyond valid bounds!
Comment 24 Gyuyoung Kim 2012-12-03 02:12:39 PST
(In reply to comment #22)
> (From update of attachment 177185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177185&action=review
> 
> > Tools/MiniBrowser/efl/main.c:71
> > +
> > +    float factor = ((float) zoomLevels[level]) / 100.0;
> > +    Evas_Coord ox, oy, mx, my, cx, cy;
> > +    evas_pointer_canvas_xy_get(evas_object_evas_get(webview), &mx, &my); // Get current mouse position on window.
> > +    evas_object_geometry_get(webview, &ox, &oy, NULL, NULL); // Get webview's position on window.
> > +    cx = mx - ox; // current x position = mouse x position - webview x position
> > +    cy = my - oy; // current y position = mouse y position - webview y position
> > +
> > +    Eina_Bool result = ewk_view_scale_set(webview, factor, cx, cy);
> > +    return result;
> > +}
> 
> This is pretty broken with interaction with viewport etc, and with deviceScaleFactor etc the way you get the position is probably also broken.
> 
> Anyway, I don't even get what you are trying to implement here? Some way to scale the content, or a scale factor (leaving things to fit into the viewport - that is what firefox does). If that is the case, using devicescalefactor instead makes more sense.
> 
> I dislike that people review these things without good explanation and without knowledge of the area.

As Yael and I talked about this issue in comment #10 ~ #13, I thought MiniBrowser is just test PC browser. If MiniBrowser should support viewport meta tag and deviceScaleFactor, I needed to ask you this patch. But, there was no comments or objections about this anymore. So, I thought that this patch could be landed from test PC browser perspective. Do you think MiniBrowser should support viewport meta tag or mobile zooming and so on ? If so, let's decide how many things MiniBrowser support. Anyway, I'm sorry that this review displease you. BTW, it would be good if you watch EFL port directories. It will help to avoid similar problem next time.
Comment 25 Kenneth Rohde Christiansen 2012-12-03 02:16:34 PST
> As Yael and I talked about this issue in comment #10 ~ #13, I thought MiniBrowser is just test PC browser.

Is it also supposed to work 100% as it is what we use for development testing. It is a very important tool.

> If MiniBrowser should support viewport meta tag and deviceScaleFactor, I needed to ask you this patch. 

It already does.
Comment 26 Ryuan Choi 2012-12-03 02:26:32 PST
(In reply to comment #22)
> (From update of attachment 177185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177185&action=review
> 
> > Tools/MiniBrowser/efl/main.c:71
> > +
> > +    float factor = ((float) zoomLevels[level]) / 100.0;
> > +    Evas_Coord ox, oy, mx, my, cx, cy;
> > +    evas_pointer_canvas_xy_get(evas_object_evas_get(webview), &mx, &my); // Get current mouse position on window.
> > +    evas_object_geometry_get(webview, &ox, &oy, NULL, NULL); // Get webview's position on window.
> > +    cx = mx - ox; // current x position = mouse x position - webview x position
> > +    cy = my - oy; // current y position = mouse y position - webview y position
> > +
> > +    Eina_Bool result = ewk_view_scale_set(webview, factor, cx, cy);
> > +    return result;
> > +}
> 
> This is pretty broken with interaction with viewport etc, and with deviceScaleFactor etc the way you get the position is probably also broken.
> 
> Anyway, I don't even get what you are trying to implement here? Some way to scale the content, or a scale factor (leaving things to fit into the viewport - that is what firefox does). If that is the case, using devicescalefactor instead makes more sense.
> 
> I dislike that people review these things without good explanation and without knowledge of the area.


Sorry, I can not understand your point about scale factor and device scale factor.

Although I am wrong, we are using scale_set for user interaction such as pinch zoom and device scale factor for viewport (1.5 for 160 dpi normally)
Why this is broken with viewport?

At least, we need to make this clear for application developers.
Almost developers, who I know, are using scale_set for scaling contents.

By the way, position logic was really wrong because scale_set requires scrolled position.
If we called this after scrolled, it will be broken. :(
Comment 27 Kenneth Rohde Christiansen 2012-12-03 02:34:17 PST
> Sorry, I can not understand your point about scale factor and device scale factor.
> 
> Although I am wrong, we are using scale_set for user interaction such as pinch zoom and device scale factor for viewport (1.5 for 160 dpi normally)
> Why this is broken with viewport?
> 
> At least, we need to make this clear for application developers.
> Almost developers, who I know, are using scale_set for scaling contents.
> 
> By the way, position logic was really wrong because scale_set requires scrolled position.
> If we called this after scrolled, it will be broken. :(


This patch seems to try to emulate desktop zoom factor which scales all contents by a factor as well as divides the layout viewport with the same factor. It seems to do that with the fixed scale values, firefox references etc.

But that is not really what it does! It just scales like a pinch zoom! Making all of that pretty irrelevant and confusing.


This patch scales like a pinch zoom, which means

1) it is not the behavior that was advertised - this is more like just a pinch zoom, which makes the zoom levels useless.
2) Pinch zoom etc need to check valid boundaries. Qt does this in PageViewportController[Client]Qt and their implementation allows a bit of overscale (20% maybe) and then bounces back. This implementation doesn't even consider valid boundaries.

The patch breaks viewport because it doesn't even consider current scale (meta and viewport @rule can define default scale etc). So if you want to emulate pinch zoom:

1) Need to check boundaries!
2) Just increase/decrease current scale with a fixed value. Say scale by 200% or so.
Comment 28 Ryuan Choi 2012-12-03 03:33:09 PST
(In reply to comment #27)
> > Sorry, I can not understand your point about scale factor and device scale factor.
> > 
> > Although I am wrong, we are using scale_set for user interaction such as pinch zoom and device scale factor for viewport (1.5 for 160 dpi normally)
> > Why this is broken with viewport?
> > 
> > At least, we need to make this clear for application developers.
> > Almost developers, who I know, are using scale_set for scaling contents.
> > 
> > By the way, position logic was really wrong because scale_set requires scrolled position.
> > If we called this after scrolled, it will be broken. :(
> 
> 
> This patch seems to try to emulate desktop zoom factor which scales all contents by a factor as well as divides the layout viewport with the same factor. It seems to do that with the fixed scale values, firefox references etc.
> 
> But that is not really what it does! It just scales like a pinch zoom! Making all of that pretty irrelevant and confusing.
> 
> 
> This patch scales like a pinch zoom, which means
> 
> 1) it is not the behavior that was advertised - this is more like just a pinch zoom, which makes the zoom levels useless.
> 2) Pinch zoom etc need to check valid boundaries. Qt does this in PageViewportController[Client]Qt and their implementation allows a bit of overscale (20% maybe) and then bounces back. This implementation doesn't even consider valid boundaries.
> 
> The patch breaks viewport because it doesn't even consider current scale (meta and viewport @rule can define default scale etc). So if you want to emulate pinch zoom:
> 
> 1) Need to check boundaries!
> 2) Just increase/decrease current scale with a fixed value. Say scale by 200% or so.

OK, I understood your concern. thank you.

This logic is really confusing. I agree with you.

By the way, I think that anyone can call scale_set without considering viewport restriction.
Comment 29 KyungTae Kim 2012-12-05 23:13:13 PST
(In reply to comment #27)
> The patch breaks viewport because it doesn't even consider current scale (meta and viewport @rule can define default scale etc). So if you want to emulate pinch zoom:
> 
> 1) Need to check boundaries!
> 2) Just increase/decrease current scale with a fixed value. Say scale by 200% or so.
I uploaded a new patch by your advice: bug 104215. Thank you.