CB-14188: Add beforeload event, catching navigation before it happens

[wvengen]

Platforms affected

Android, iOS

What does this PR do?

Implements CB-14188: support for a callback that determines whether a URL to be loaded in the inAppBrowser should be loaded or not.

What testing has been done on this change?

On iOS & Android emulators (will test on real devices soon).

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change: I'd be happy to add it once the feature is finished otherwise.
• Added documentation for this change (added checklist item)
• Research whether this is possible asynchronously
• Use a synchronous callback

Example

app = cordova.InAppBrowser.open(url, "_blank", "beforeload=yes")
app.addEventListener("beforeload", function(e, cb) {
  if (e.url.startsWith("https://example.com/")) {
    cb(e.url); // Tell inAppBrowser to open the URL indeed.
  } else {
    alert("Opened external URL: " + e.url);
  }
}, false);

[brodybits]

Looks nice in general. A couple major comments on my part:

The idea of passing an asynchronous callback to an event listener seems really strange to me. While I can definitely understand the logic behind making it asynchronous between native and JavaScript I don't think this really justifies introducing what looks like new paradigm in event listeners. I can think of the following alternative approaches:

• event listener synchronously returns non-null value to indicate that application does not want to load the URL, in a similar fashion to a standard beforeload event handler ref: [1] (preferred)
• event listener throws an exception to override default behavior, in a similar fashion to backbutton event on Windows ref: [2]
• new asynchronous callback mechanism outside the event handler mechanism that the application can use to filter URL load requests

The code also seems to assume that there would never be multiple outstanding load requests, which I would not agree with. What if a user clicks on a couple links in succession, or the webpage JavaScript issues multiple HTTP(S) requests?

[1] https://eloquentjavascript.net/15_event.html#p_nu8/BUQa7r
[2] https://cordova.apache.org/docs/en/latest/cordova/events/events.html#windows-quirks

[wvengen]

Thanks a lot for your review and comments, @brodybits. I agree that the current approach is not ideal, and have struggled with how to add the callback. Indeed, a synchronous callback would be the best option, that would first need some research to whether Android's and iOS's WebView implementation allow this to take a little time (because it involves a Javascript invocation).

If this is not possible (or if this is ported to a platform requiring near-immediate return from its do-I-need-to-load-this-URL-function), for Cordova users it can still be a synchronous function, with the help of a workaround similar to what happens now, but without the concurrency issue you mentioned (I have an idea to do this using a custom URL scheme).

Interesting you mention the beforeunload event, that would be familiar to many. For inAppBrowser, the confirm dialog would not be shown. It also makes sense to skip the callback on first page load, as that is controlled by the inAppBrowser-invoking code already.

I'll delve into this. Since I'll be on holiday for about two weeks, it may take a little time.

[wvengen]

Reading up on HTML's beforeunload event, I do see preventDefault(), e.g. on MDN, W3C HTML 5.0 (suggesting e.returnValue) and WHATWG (preferring e.preventDefault()).

I would suggest to adopt e.returnValue and e.preventDefault() in InAppBrowser's new beforeunload event. This is in-line with current standards, allows using the existing event handling infrastructure, and by updating the event this will also work with multiple event handlers. It does require some changes to the event object passed on to the handlers (so that returnValue can be reported back, and e.preventDefault() is available).

[wvengen]

I'd also suggest to explicitely enable the event (with an option like beforeunload=yes when opening the InAppBrowser), so that we're sure there is no performance penalty (if any). We can always consider removing the option in the future, if it doesn't turn out to be an issue in practice.

[janpio]

Hey, I just fixed the problem that caused Android tests to fail in master. Could you rebase this PR please? This should get rid of the Android failures and possibly fix all test failures for this PR.

[wvengen]

@janpio still failing on Android 7 :(

[janpio]

Yep, you are unfortunately hitting this: #307

[janpio]

And passing :) (context: #307 (comment))

[brodybits]

Looks OK to me, merging

[janpio]

This is already merged now, but can this be added to the tests and checked automatically that it works?

[wvengen]

Oh, wait, wasn't expecting this to be merged already ... I was still coming back to processing the feedback of @brodybits on the callback mechanism. I suppose these points are still valid ... ?

I'm also ok with having this PR included right now (so the feature can be used), and changing the API later (in a next major release), if somebody has a better way of implementing it. Or mark the API as unstable, so it can be changed in a point release. What shall we do?

[purplecabbage]

Why 2 vars that will always store the same value?

[wvengen]

(this is merged already, but perhaps my reply will serve as a reference)
waitForBeforeload is the current state, whether we need to wait a call back from JS to really load next time it is encountered. useBeforeload indicates whether the beforeload event is used at all (see the end of shouldOverrideUrlLoading).

[janpio]

Yes, I agree that the feedback is valid - I have no idea why @brodybits just merged it after giving this feedback himself. Could you please create a new PR and refer to this one so it all gets connected? Thanks.

[brodybits]

I was hoping to unblock forward progress, looks like it wasn't so good. I cannot promise when I will get a chance to follow up again. Sorry for the troubles.

@wvengen I do stand by the feedback I gave before, would like to treat them as minor points in case anyone has a chance to do it better someday.

[purplecabbage]

Not a blocker, just a little nit.
Keep moving forwards.

[wvengen]

Ah, that clears it up. And thanks for merging: with these minor points I think it indeed makes sense to include it already, and if interest in this feature would increase, it can be improved.
I'll open an issue to track this.

Thanks again for your support, @brodybits!

[jezmck]

Sorry for the simple question, is this in the current release (v3.0.0)?
I've not been able to catch this event.