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

Improve beforeload event #338

Open
wvengen opened this issue Oct 31, 2018 · 7 comments
Open

Improve beforeload event #338

wvengen opened this issue Oct 31, 2018 · 7 comments

Comments

@wvengen
Copy link
Contributor

wvengen commented Oct 31, 2018

In #276 (CB-14188) the beforeload event was added.
This event currently uses the same callback mechanism as other events, but it is different in one important aspect: instead of just notifying that something happened, it expects an action to be called. It would be good to allow a synchronous callback instead.

@wvengen
Copy link
Contributor Author

wvengen commented Oct 31, 2018

Feedback of @brodybits on this was:

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

A synchronous callback would be preferable, but that needs to be technically possible in Android's and iOS's WebView implementation, because the callback may take some time (because it involves a Javascript invocation).

And then, regarding beforeload, @wvengen mentioned:

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

@benvallack
Copy link

I'm struggling to see how to use this with Ionic 4. How does one call the callback function in the beforeload event handler when using the new .subscribe style way of dealing with events?

browser.on("beforeload") .subscribe( (ev: InAppBrowserEvent) => {
console.log(ev.url);
browser._loadAfterBeforeload(ev.url);
}, err => { });

The event is being fired as I see the url in the console - but the above wont build - I just guessed with the _loadAfterBeforeload function but it doesn't work.

How do I call the callback to continue loading the url in this event?

@wvengen
Copy link
Contributor Author

wvengen commented Sep 19, 2019

In the example's beforeLoadCallback you can see that a second parameter to the callback is a function you can call to load the url.

@benvallack
Copy link

Thanks - but where do I reference this second parameter in the above code?

@wvengen
Copy link
Contributor Author

wvengen commented Sep 19, 2019

Assuming you're using typescript, I would suppose it to be something like:

browser.
  on("beforeload").
  subscribe( (ev: InAppBrowserEvent, callback: (url: string) => void ) => {
    console.log(ev.url);
    callback(ev.url);
  }, err => { });

For people using typescript, I guess it would be useful to annonate the type for this callback.

@benvallack
Copy link

Thanks so much for this, I think we're close! - unfortunately I see this error now:

Argument of type '(ev: InAppBrowserEvent, callback: (url: string) => void) => void' is not assignable to parameter of type '(value: InAppBrowserEvent) => void'.

Any ideas?

@benvallack
Copy link

Still not managing to get this working. There is no mention of it in the types.d.ts file which I think is why it's triggering the error.

If I use // @ts-ignore it bypasses the error but then I get a javascript error in the console when running in the simulator.

I'm wondering if the beforeload callback is working in the current version? Many thanks.

@benvallack benvallack mentioned this issue Oct 28, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants