[TIMOB-25277] Android: Implement onLink callback for Ti.UI.WebView

[garymathews]

• Implement onlink callback property for Titanium.UI.WebView
• Callback must return true or false as a condition for loading the link
• Fixed issue where events from new windows would not fire

TEST CASE

var window = Ti.UI.createWindow(),
    webView = Ti.UI.createWebView({
        url: 'https://india.gov.in/website-ministry-commerce-and-industry',
        onlink: function(e) {
            if (e.url.endsWith('.pdf')) {
                alert('PDF: ' + e.url);
                return false;
            }
            return true;
        }
    });

window.add(webView);
window.open();

JIRA Ticket

[garymathews]

@hansemannn Maybe we could add this to iOS for parity?

[hansemannn]

We should call it "link", so without the on. The reason we are doing that is because Alloy would use it as on="onLink" which looks pretty bad. Also, when exactly is that fired? We already have beforeload or blacklisturl which sounds pretty similar to me.

EDIT: Sorry, didn't see it's a property. But even that, we should move all event-related properties to real events, otherwise it confuses the developer and makes it more difficult to use in Alloy (via manual controller reference).

EDIT 2: I saw your comment on the JIRA - that's exactly what I would do. And it's cross-platform already. The wildcard .pdf should also already work for both platforms and they could use the solution today, so I'd vote for that!

[garymathews]

I can't make it an event since it requires an immediate return value (similar to onCreateWindow)

[hansemannn]

But using the existing API's, they should do that already if I got that correct? iOS uses a delegate-method that returns something as well, so checking for the blacklisted URL's changes the return-value. Exposing this as a property would not be possible on iOS and also wouldn't make sense since the above reasons.

[jquick-axway]

It sounds like the ultimate goal here is to provide an opportunity to cancel URL navigation and do something native instead. I'd like to share my experience regarding this.

How this is implemented now, the onLink() callback is hooked up to the Android WebViewClient.onPageStarted(), which unfortunately is invoked after the HTTP request has been sent. So, while you may be able to cancel the page load in-time on a slow Internet connection, in my experience, you'll never be able to cancel it in time when loading a local HTML file. For example, I've worked with mobile app devs who used WebViews and local HTML files more as a fancy UI front-end where the links were unregistered URL schemes to tell the native code to execute an operation (and the navigation needs to be canceled to prevent a 404).

On iOS, our Titanium beforeload event is hooked up to the UIWebView.shouldStartLoadWithRequest which gets invoked before the HTTP request gets sent. That is, UIWebView is asking the app developer for permission to navigate. This event would allow a developer to set up the above functionality mentioned (ie: cancel the HTTP request and perform a native operation instead) and is what we're really after.

On Android, the equivalent to iOS' shouldStartLoadWithRequest method is WebViewClient.onLoadResource() which asks permission before sending the HTTP request... but it has 1 problem. The Android onLoadResource() method does not get invoked when navigating to another URL via JavaScript; it only gets invoked when tapping on a link. That was my experience back when I was trying to do this during the Android 2.x days. Back then, navigating via URLs can only be detected via the onPageStarted() when it is already too late (I've never found a work-around unfortunately).

So... I'm wondering if the real solution here is to tweak our "beforeload" event to provide this functionality. On Android, "beforeload" technically happens when the next URL has already started and not before it loads like how it is on iOS. But that said, I think it was made this way on purpose on Android to work-around JavaScript URL navigation detection mentioned above. However, we can still tweak our Android code to trigger a "beforeload" via the Java onLoadResource() and flag the event as already fired when we reach the Java onPageStarted() to prevent the event from being fired twice.

Anyways, that's my 2 cents.
I'm curious if Android 4.x or newer OS versions has made this any easier.

[garymathews]

The issue with onLoadResource() is that every resource loaded by the page is passed through it (images, css, scripts). Where we only want the link the user has clicked on.

I think in this scenario onPageStarted() will do.

@hansemannn The workaround posted on JIRA, although works, isn't intended for this behavior. It's also not cross-platform as the onCreateWindow property does not exist on iOS. I don't think users should need to implement a workaround like that to have control over the links a user has clicked on. Maybe there's a better way to implement this?

[jquick-axway]

You know what, I think I got my Android methods mixed up. I meant WebViewClient.shouldOverrideUrlLoading(), not onLoadResource.

[garymathews]

Ahh, I do. But I intercept those before they reach shouldOverrideUrlLoading() here.

[build]

	Fails
🚫	🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.
🚫	Tests have failed, see below for more information.

	Messages
📖	💾 Here's the generated SDK zipfile.

Tests:

Classname	Name	Time	Error
android.console	#timeEnd	0	AssertionError: expected undefined to be a function
android.console	#time	0.003	AssertionError: expected undefined to be a function
android.Titanium.Android.Service	createService-foreground-normal	0.002	TypeError: service.foregroundNotify is not a function
android.Titanium.Media.VideoPlayer	endPlaybackTime	0.001	AssertionError: expected undefined to be a number
android.Titanium.Media.VideoPlayer	duration	0.001	AssertionError: expected undefined to be a number
android.Titanium.Media.VideoPlayer	playableDuration	0.003	AssertionError: expected undefined to be a number
android.Titanium.Media.VideoPlayer	autoplay	0	AssertionError: expected undefined to be a boolean
android.Titanium.UI.Layout	twoPins	5.969	Error: timeout of 5000ms exceeded
android.Titanium.UI.Layout	undefinedBottom	5.58	Error: timeout of 5000ms exceeded
android.Titanium.UI.Layout	viewTop	5.33	Error: timeout of 5000ms exceeded
android.Titanium.UI.TableView	Add and remove headerView/footerView	5.004	Error: timeout of 5000ms exceeded
ios.console	#timeEnd	0.002	file:///Users/build/Library/Develo
ios.console	#time	0.002	file:///Users/build/Library/Develo
ios.Error	Native exception surfaced	0.013	file:///Users/build/Library/Developer/CoreSimulator/Devices/B33AB49C-A8CC-403B-9950-1269FC7C660C/data
ios.Titanium.Analytics	.optedOut	0.001	file:///Users/build/Library/Devel
ios.Titanium.Media.VideoPlayer	Release video player and close window (TIMOB-26033)	10.012	file:///Users/build/Librar
ios.Titanium.Media.VideoPlayer	showsControls	0	file:///Users/build/Libr
ios.Titanium.Media.VideoPlayer	volume	0.001	file:///Users/build/
ios.Titanium.UI	#convertUnits() returns 25.4 mm = dpi pixels	0.001	file:///Users/build/Library/Developer/CoreS
ios.Titanium.UI	#convertUnits() converts 1 in = dpi pixels	0	file:///Users/build/Libr
ios.Titanium.UI	#convertUnits() returns Ti.Platform.DisplayCaps.logicalDensityFactory for 1dip to pixels	0.001	file:///Users/build/Library/Developer/
ios.Titanium.UI	#convertUnits() returns 2.54cm = dpi pixels	0	file:///Users/build/Library/Developer/CoreS
ios.Titanium.UI	#convertUnits() converts 100 unspecified units to px	0.001	file:///Users/build/Library/Developer/Core
ios.Titanium.UI.iOS	#constants	0.011	file:///Users/build/Library/Deve
ios.Titanium.UI.WebView	.zoomLevel	10.004	file:///Users/build/Librar

Generated by 🚫 dangerJS

[hansemannn]

@garymathews I've updated the docs to fix the validation error. Let me know if something else should be revisited.

[lokeshchdhry]

@garymathews , @jquick-axway , Can anyone of you please fix the lint errors. Thanks !!

[lokeshchdhry]

Needs to be changed to 7.3.0

[lokeshchdhry]

FR Passed.

onLink callback is called successfully with the url of the pdf.

Studio Ver: 5.1.0.201804230827
SDK Ver: 7.3.0 local build
OS Ver: 10.13.4
Xcode Ver: Xcode 9.3.1
Appc NPM: 4.2.13
Appc CLI: 7.0.3
Daemon Ver: 1.1.1
Ti CLI Ver: 5.1.0
Alloy Ver: 1.12.0
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10
Devices: ⇨ google Nexus 6P --- Android 8.1.0
⇨ google Nexus 5 --- Android 6.0.1
Emulator: ⇨ Android 4.1.2

[lokeshchdhry]

Waiting for CR to pass.

[jquick-axway]

@garymathews, @lokeshchdhry, I'm worried that this might crash if "tiapp.xml" property "run-on-main-thread" is set to false, because it'll invoke the JavaScript callback on the wrong thread.

Edit:
Here's a crazy idea. Maybe this feature should only be supported on the main UI thread? Because there isn't a nice way to handle this in an async manner since the WebView needs immediate feedback. I know iOS has a similar feature where you cancel the link/url that's about to be loaded.

[lokeshchdhry]

@garymathews , @jquick-axway , Are we making any changes to this PR ?

[sgtcoolguy]

Bumping to 7.4.0 to be merged with an equivalent iOS implementation per https://jira.appcelerator.org/browse/TIMOB-26063 cc @hansemannn

Also to be merged in same released as Windows PR: appcelerator/titanium_mobile_windows#1211

[hansemannn]

I would still disagree with the interface design here. We have something very similar with the "blacklisturl" event already, which is cross-platform and works today already. Can't we just get the URL from there, cancel the HTTP request and live a happy life?

[garymathews]

@jquick-axway Could you write a test case that reproduces the threading issue with the onlink callback? I'm having trouble reproducing it

[jquick-axway]

Okay. I've verified that the onlink callback is thread safe when "run-on-main-thread" is set to false. I've verified it by running a brute-force loop in JS while tapping on a link to invoke theonlink callback.

For everyone's info, the KrollFunction.call() will block the current thread and queue the callback to be invoked on the runtime thread when available. This avoids a race condition and prevents the JavaScript stack from getting clobbered. To walk you through it, KrollFunction.call() will call TiMessenger.sendBlockingRuntimeMessage(), which calls TiMessenger.sendBlockingMessage(), and that method will block forever until the callback has been invoked.
https://github.com/appcelerator/titanium_mobile/blob/master/android/runtime/v8/src/java/org/appcelerator/kroll/runtime/v8/V8Function.java#L38
https://github.com/appcelerator/titanium_mobile/blob/master/android/runtime/common/src/java/org/appcelerator/kroll/common/TiMessenger.java#L174
https://github.com/appcelerator/titanium_mobile/blob/master/android/runtime/common/src/java/org/appcelerator/kroll/common/TiMessenger.java#L239

@hansemannn, do we have something similar on iOS?

[jquick-axway]

There's one issue here. The onPageStarted() method gets called after the HTTP request was sent. If you set up the URL to use a customer URL scheme that was intended to be overridden by onlink, then the WebView will display an "unknown url scheme" error page.

I'm able to reproduce this issue with the following:

var window = Ti.UI.createWindow();
var webView = Ti.UI.createWebView({
	html: '<!DOCTYPE html><html><body><a href="myapp:HelloWorld">Tap this link</a></body></html>',
	onlink: function(e) {
		Ti.API.info("@@@ onlink() invoked. url: " + e.url);
		var endTime = new Date();
		endTime.setTime(endTime.getTime() + 1000);
		while (endTime >= new Date()) {}
		return (e.url !== "myapp:HelloWorld");
	},
});
window.add(webView);
window.open();

The above issue can be solved by using the WebViewClient.shouldOverrideUrlLoading() method instead. But the downside to using this method (based on what I remember) is that it only gets called when tapping/clicking on links. It does not get called for URLs invoked in JavaScript within the webpage (and I know iOS' UIWebView does not have this limitation). But this might be okay since we've named the property onlink.

[hansemannn]

@jquick-axway @garymathews Is this PR ready to merge? iOS was just merged.

[jquick-axway]

@hansemannn, there is 1 issue left which I've posted above.

[garymathews]

Updated PR

[build]

	Messages
📖	💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

[lokeshchdhry]

FR Passed.

Studio Ver: 5.1.2.201810080801
SDK Ver: 7.5.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.0
Appc NPM: 4.2.14-2
Appc CLI: 7.0.7-master.9
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.3
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Pixel (Android 9)
Emulator: ⇨ Android 4.1.2