Skip to content

(android) Removed waitForBeforeload flag as it prevents beforeLoad from being called on every GET request #755

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PDLMobileApps
Copy link
Contributor

Platforms affected

Android

Motivation and Context

Resolves #686

Link to the issue: #686

Description

We removed waitForBeforeload flag as it prevents beforeLoad from being called on every GET request and it does not seem to have any meaningful use.

Testing

Testing was done manually as I am not sure how to automate it.

The following tests were executed for Android 7, 8, and 10.

Created a new Cordova test app using cordova-plugin-inappbrowser.
Executed:
cordova.InAppBrowser.open("https://www.hannaford.com", "_blank", "beforeload=get");
and verified beforeLoad gets called with every get request.

Executed:
cordova.InAppBrowser.open("mailto:[email protected]", "_blank", "beforeload=get");
and verified beforeLoad gets called with that get request.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

…o be called for every GET request and it does not seem to have any meaningful use
PDLMobileApps pushed a commit to PDLMobileApps/cordova-plugin-inappbrowser that referenced this pull request Jul 31, 2020
@dpa99c
Copy link
Contributor

dpa99c commented Sep 1, 2020

@wvengen originally implemented the beforeload feature and his PR comment indicates that waitForBeforeload is necessary.

have you got an opinion on this @wvengen?

@PDLMobileApps
Copy link
Contributor Author

@wvengen originally implemented the beforeload feature and his PR comment indicates that waitForBeforeload is necessary.

have you got an opinion on this @wvengen?

We saw it was added on purpose and I believe it may had a specific goal. However, now it has the effect of preventing beforeLoad from being called on every GET request, which is quite a big deal.
We removed it and we have been using the code in this PR successfully in production for a week.

@stionic
Copy link

stionic commented Sep 24, 2020

waitForBeforeload is necessary. I think it prevent duplicate event.

for fix problem you maybe want correct it instead of remove.

I have found same problem before and fixed by correct it when onPageFinished. Real problem is when load error it not reset waitForBeforeload.

Could you please check commit?

@PDLMobileApps
Copy link
Contributor Author

PDLMobileApps commented Oct 1, 2020

waitForBeforeload is necessary. I think it prevent duplicate event.

for fix problem you maybe want correct it instead of remove.

I have found same problem before and fixed by correct it when onPageFinished. Real problem is when load error it not reset waitForBeforeload.

Could you please check commit?

We have been using the change in this PR in our app in the Google Play Store for almost two month (500k installs). We have no reports of issues with beforeLoad which is called a lot when the customer uses the app. So, if waitForBeforeload is really necessary, I would expect a completely different situation.

By the way, no one has been able to provide a good explanation on why it's needed. On the other hand, if you have in the code, beforeLoad only happens every other request, which is definitely wrong. That has been reported more than once by many people.

It would really nice to have an good explanation on why it's necessary. By looking at the code, I cannot really understand it.

@NiklasMerz NiklasMerz added this to the 5.next milestone Apr 7, 2021
@dtarnawsky
Copy link

To follow up on this PR, I've found in testing that this PR is necessary for beforeload to function more than once. @stionic: The code for waitForBeforeload doesn't prevent duplicates of the beforeload event occurring and can be safely removed .

@ep-mark
Copy link

ep-mark commented Jul 11, 2022

This PR fixes the issues we found too!

ep-mark pushed a commit to EducationPerfect/web-npm-cordova-plugin-inappbrowser that referenced this pull request Jul 11, 2022
@wunschradio
Copy link

Thank you. This finally fixed this issue for me.

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

Successfully merging this pull request may close these issues.

beforeload event fires only on odd links
7 participants