Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-24982] Android/iOS: Add WebView zoomLevel #9226

Merged
merged 38 commits into from May 15, 2018
Merged

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Jul 19, 2017

JIRA: https://jira.appcelerator.org/browse/TIMOB-24982

var win = Ti.UI.createWindow();
var webView = Ti.UI.createWebView({
	url: "http://test.de"
});
var btn = Ti.UI.createButton({
	title: "Zoom 1",
	bottom: 0
});
var btn2 = Ti.UI.createButton({
	title: "Zoom 2",
	bottom: 45
});
var btn3 = Ti.UI.createButton({
	title: "Zoom 4",
	bottom: 90
});
var btn4 = Ti.UI.createButton({
	title: "get zoom",
	bottom: 135
});

btn.addEventListener("click", function(e) {
	webView.zoomLevel = 1;
	Ti.API.info(webView.zoomLevel);
});
btn2.addEventListener("click", function(e) {
	webView.setZoomLevel(2);
	Ti.API.info(webView.getZoomLevel());
});
btn3.addEventListener("click", function(e) {
	webView.setZoomLevel(4);
	Ti.API.info(webView.zoomLevel);
});
btn4.addEventListener("click", function(e) {
	Ti.API.info(webView.zoomLevel);
});
win.add(webView);
win.add(btn);
win.add(btn2);
win.add(btn3);
win.add(btn4);
win.open();

@m1ga m1ga changed the title Android: Add WebView zoomBy [AC-5096] Android/iOS: Add WebView zoomBy Jul 19, 2017
@hansemannn hansemannn changed the title [AC-5096] Android/iOS: Add WebView zoomBy [TIMOB-24982] Android/iOS: Add WebView zoomBy Jul 19, 2017
@m1ga m1ga changed the title [TIMOB-24982] Android/iOS: Add WebView zoomBy [TIMOB-24982] Android/iOS: Add WebView zoomLevel Jul 19, 2017
type: Number
default: undefined. Behaves as no zoom applied.
platforms: [android, iphone, ipad]
since: "6.2.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a "description" that states how to use the zoomLevel numbers such as 1.0 being a 1x scale, 2.0 being a 2x scale, etc. (Versus 100 for a 100% zoom scale and 200 for a 200% scale. We don't want this to be ambiguous.)

And should this property be null by default? Wouldn't it be better to default it to 1.0? You're zooming based on whatever the default WebView scale is, so, it's relative scaling from there. For example, the HTML file used to be able set a "target-densitydpi" meta tag (I think it's deprecated now) that might not match the DPI of the device.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point with the default null value. I would guess iOS currently returns an empty string by default, we could detect that and map it to 1.0. The problem is that unless the user manually sets the zoom-level, he could still pinch-to-zoom, which would not change the value. We should be aware of that behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do decide to update the "zoomLevel" property based on user interaction (pinch zoom or tapping zoom UI buttons), then on Android, we would need to override the onScaleChanged() method...
https://developer.android.com/reference/android/webkit/WebViewClient.html#onScaleChanged(android.webkit.WebView,%20float,%20float)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zoomLevel should multiply the numbers all the time. So if you do setZoomLevel(2);setZoomLevel(2); it should return 4 and you need to set setZoomLevel(0.25); to go back to zoomLevel 1. zooming by pinch should set the value to a proper level. Have to do some testing how to get this to work

@@ -491,6 +495,8 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP
}
} else if (TiC.PROPERTY_DISABLE_CONTEXT_MENU.equals(key)) {
disableContextMenu = TiConvert.toBoolean(newValue);
} else if (TiC.PROPERTY_ZOOM_LEVEL.equals(key)) {
getWebView().zoomBy(TiConvert.toFloat(newValue));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WebView.zoomBy() method is only available on API Level 21 (Android 5.0) and higher. So, calling this method on older Android OS versions will cause a crash.
https://developer.android.com/reference/android/webkit/WebView.html#zoomBy(float)

We also need to constrain the zoom level between 0.1 to 100.0 to avoid an IllegalArgumentException.
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/webkit/WebView.java#L2111

I recommend that you guard the higher API level call as shown below. Putting the higher API level call within a private inner Java class avoids the opcode warning and is how Google does it via their support libraries. Read Google's blog post here for details on this concept.

} else if (TiC.PROPERTY_ZOOM_LEVEL.equals(key)) {
	if (Build.VERSION.SDK_INT >= 21) {
		ApiLevel21.zoomBy(getWebView(), TiConvert.toFloat(newValue, 1.0f));
	}
}


private static class ApiLevel21
{
	private ApiLevel21() {}

	public void zoomBy(WebView webView, float scale)
	{
		if (webView != null) {
			if (scale < 1.0f) {
				scale = 1.0f;
			} else if (scale >= 100.0f) {
				scale = 100.0f;
			}
			webView.zoomBy(scale);
		}
	}
}

The only bummer here is with Android 4.x which does not support the zoomBy() method. The older OS versions only support zoomIn() and zoomOut() methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we benefit much from adding these inner classes to prevent logcat warnings on lower Android versions. Not all support classes do this either:

I/dalvikvm: Could not find method android.widget.FrameLayout.startActionModeForChild, referenced from method android.support.v7.widget.ActionBarContainer.startActionModeForChild
W/dalvikvm: VFY: unable to resolve virtual method 21612: Landroid/widget/FrameLayout;.startActionModeForChild (Landroid/view/View;Landroid/view/ActionMode$Callback;I)Landroid/view/ActionMode;
D/dalvikvm: VFY: replacing opcode 0x6f at 0x0002
W/dalvikvm: VFY: unable to find class referenced in signature (Landroid/graphics/drawable/Icon;)
I/dalvikvm: Could not find method android.widget.ImageButton.setImageIcon, referenced from method android.support.v7.widget.AppCompatImageButton.setImageIcon
W/dalvikvm: VFY: unable to resolve virtual method 21646: Landroid/widget/ImageButton;.setImageIcon (Landroid/graphics/drawable/Icon;)V
D/dalvikvm: VFY: replacing opcode 0x6f at 0x0000
E/dalvikvm: Could not find class 'android.graphics.drawable.RippleDrawable', referenced from method android.support.v7.widget.AppCompatImageHelper.hasOverlappingRendering
/dalvikvm: VFY: unable to resolve instanceof 205 (Landroid/graphics/drawable/RippleDrawable;) in Landroid/support/v7/widget/AppCompatImageHelper;
D/dalvikvm: VFY: replacing opcode 0x20 at 0x000c

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'll have a look at this

@m1ga
Copy link
Contributor Author

m1ga commented Sep 10, 2017

Update the example app in the first posting.

The default zoomLevel is depending on the DPI of the device (https://stackoverflow.com/a/25787425/5193915). In my case it is 2.0 (fully zoomed out). It gets set by onScaleChanged() so it will work with pinching and setZoomLevel().

User story:

  • zoomLevel at start: 2.0
  • webView.setZoomLevel(2); (button zoom in)
  • zoomLevel: 4.0
  • webView.setZoomLevel(2); (button zoom in)
  • zoomLevel: 8.0
  • webView.setZoomLevel(0.25); (button zoom out)
  • zoomLevel: 2.0 (full page visible)

{
if (webView != null) {
if (scale < 0.0f) {
scale = 0.0f;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimum scale needs to be floored to 0.1f. A value less than this will cause an exception to be thrown.

{
zoomLevel = value;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a public setZoomLevel() method that doesn't actually change the WebView's zoom level is a confusing API. I understand why you're doing this, but there is a better way.

I recommend that you remove the setZoomLevel() method and the zoomLevel member variable from the TiUIWebView class. You should instead add a zoomLevel member variable and getZoomLevel() method to the TiWebViewClient class and update the member variable there via its onScaleChanged() listener. Your TiUIWebView.getZoomLevel() method can then simply return the client's zoom level like this...

public float getZoomLevel()
{
	return (this.client != null) ? this.client.getZoomLevel() : 1.0f;
}


public void zoomBy(float scale)
{
zoomBy(getWebView(), scale);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This public zoomBy() method is missing an API Level 21 check like how we handle the zoom level property. Either add the API Level check here too or move all the API Level checks to the private zoomBy() method.

@hansemannn
Copy link
Collaborator

You are right! Also, the userAgent property was merged into the if-statement of the new property, so I cleaned both up. Diff looks much smaller now. Once Jenkins passes and @jquick-axway + @lokeshchdhry give their go, we should be fine here.

@m1ga
Copy link
Contributor Author

m1ga commented May 9, 2018

@hansemannn Thanks!

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented May 9, 2018

@m1ga , I am seeing the following with the test code above I don't see the zoom level being 2x for Zoom 2 but for Zoom 4 for Android.
On android I see the following logs:

  1. Android 8.0:
[INFO] :   [Nexus 6P] 3.5    -- Zoom 1
[INFO] :   [Nexus 6P] 3.5    -- Zoom 2
[INFO] :   [Nexus 6P] 7      -- Zoom 4
  1. Android 6.0.1:
[INFO] :   [Nexus 5] 3    -- Zoom 1
[INFO] :   [Nexus 5] 3    -- Zoom 2
[INFO] :   [Nexus 5] 6    -- Zoom 4

3.Android4.1:
Zooming does not work at all.

[INFO] :   [Android SDK built for x86] 2    -- Zoom 1
[INFO] :   [Android SDK built for x86] 2    -- Zoom 2
[INFO] :   [Android SDK built for x86] 2    -- Zoom 4

IOS works as it should:

[INFO] :   1
[INFO] :   2
[INFO] :   4

@m1ga
Copy link
Contributor Author

m1ga commented May 9, 2018

@lokeshchdhry I'll have have a look right now. There is a check:
if (Build.VERSION.SDK_INT >= 21 && webView != null)
so it won't work with 4.1

@m1ga
Copy link
Contributor Author

m1ga commented May 9, 2018

Ok, the output is just one off. So if you press a button twice you'll see the correct zoom level, but not the absolute value. Fixing it right now

@m1ga
Copy link
Contributor Author

m1ga commented May 9, 2018

Return values are correct now and I've change the example in the first post so there is a 4th button that will return the zoomValue when you e.g. zoom by hand

@hansemannn hansemannn merged commit cf80e34 into tidev:master May 15, 2018
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@m1ga m1ga deleted the zoomBy branch December 10, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants