[GitHub] wvengen commented on issue #338: Improve beforeload event

[GitBox <gi...@apache.org>]

wvengen commented on issue #338: Improve beforeload event
URL: https://github.com/apache/cordova-plugin-inappbrowser/issues/338#issuecomment-434584017
 
 
   [Feedback](https://github.com/apache/cordova-plugin-inappbrowser/pull/276#issuecomment-405571972) 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](https://github.com/apache/cordova-plugin-inappbrowser/pull/276#issuecomment-409489649):
   > Reading up on HTML's `beforeunload` event, I do see `preventDefault()`, e.g. on [MDN](https://developer.mozilla.org/en-US/docs/Web/Events/beforeunload), [W3C HTML 5.0](https://www.w3.org/TR/html50/browsers.html#unloading-documents) (suggesting `e.returnValue`) and [WHATWG](https://html.spec.whatwg.org/#prompt-to-unload-a-document) (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).

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org