Bug 99285 – [EFL][WK2] Add support for a zoom level setting to MiniBrowser

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KyungTae Kim]

EFL MiniBrowser does not currently support zoom-in, zoom-out.

[KyungTae Kim]

Created attachment 168621 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 168621 [details]
Patch

Attachment 168621 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14296569

[Chris Dumez]

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.

[KyungTae Kim]

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.

[Chris Dumez]

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

[Ryuan Choi]

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

[Ryuan Choi]

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.

[KyungTae Kim]

Created attachment 168638 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 168638 [details]
Patch

Attachment 168638 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14298639

[Yael]

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?

[Gyuyoung Kim]

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

[Yael]

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

[Gyuyoung Kim]

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

[KyungTae Kim]

Created attachment 175914 [details]
Patch

[Ryuan Choi]

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

[Chris Dumez]

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?

[KyungTae Kim]

Created attachment 177185 [details]
Patch

[Gyuyoung Kim]

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 ?

[KyungTae Kim]

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.

[WebKit Review Bot]

Comment on attachment 177185 [details]
Patch

Clearing flags on attachment: 177185

Committed r136369: <http://trac.webkit.org/changeset/136369>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

Argh! This even allows people to scale beyond valid bounds!

[Gyuyoung Kim]

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

[Kenneth Rohde Christiansen]

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

[Ryuan Choi]

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

[Kenneth Rohde Christiansen]

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

[Ryuan Choi]

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

[KyungTae Kim]

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