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

CB-14234: (ios) Don't call handleOpenURL for system URLs #278

Merged
merged 1 commit into from Aug 22, 2018

Conversation

dpolivy
Copy link
Contributor

@dpolivy dpolivy commented Aug 10, 2018

When calling .open() with a target of _system, the InAppBrowser on iOS is both launching the URL in the system browser AND also broadcasting to open the URL within the app (calling handleOpenURL). The latter behavior is problematic in many circumstances (e.g. when you want to explicitly open a link in a browser which is a universal link handled by the app).

This commit attempts to address this by checking the return value from openURL -- if it does not open the URL successfully, then (and only then) the code falls back to broadcasting the event within the app to handleOpenURL.

Platforms affected

iOS

What does this PR do?

Fixes CB-14234; it checks the return value of the call to openURL, and only broadcasts the handleOpenURL event within the app if openURL fails to open the URL in the system.

What testing has been done on this change?

I've done manual testing on iOS

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.

@jcesarmobile
Copy link
Member

Looks good to me, but remove the comment, if somebody want to know why it's changed they can check the git log/blame and the commit already have the issue id, so it's easy to spot

When calling `.open()` with a target of `_system`, the InAppBrowser on iOS is both launching the URL in the system browser AND also broadcasting to open the URL within the app (calling handleOpenURL). The latter behavior is problematic in many circumstances (e.g. when you want to explicitly open a link in a browser which is a universal link handled by the app).

This commit attempts to address this by checking the return value from openURL -- if it does not open the URL successfully, then (and only then) the code falls back to broadcasting the event within the app to handleOpenURL.
@dpolivy
Copy link
Contributor Author

dpolivy commented Aug 22, 2018

@jcesarmobile First time I've been asked to remove comments from code 😄 Done, and commit/PR updated. Let me know if there's anything else needed to merge this.

@jcesarmobile jcesarmobile merged commit cf58b04 into apache:master Aug 22, 2018
@jcesarmobile
Copy link
Member

Yeah, sometimes comments are good, but for so small change and being the issue and the PR linked to the commit, it's really easy to see why this was done, so no need of extra comments. Thanks for the PR!

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.

None yet

2 participants