CB-14187: (ios) Change the InAppBrowser to allow custom schemes #274

wvengen · 2018-07-03T11:50:17Z

Platforms affected

iOS

What does this PR do?

Implements CB-14187, which adds support for the AllowedSchemes preference and customscheme event for iOS.

What testing has been done on this change?

Tested on an iPhone 5SE running iOS 11.4.

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: as though CB-14013 (CB-14013: (android) Change the InAppBrowser to allow custom schemes for oAuth #263) did not include additional tests, ~~I expect it to be unnecessary here as well~~, I added a manual test (as it needs editing config.xml).

brodycj · 2018-07-17T13:54:40Z

The proposed changes definitely seem to be consistent with the corresponding functionality for Android from GH-263 (CB-14013). A couple more things that could help:

a quick and easy app that I or someone else could use to test on both iOS and Android
document the custom scheme support for both Android and iOS

wvengen · 2018-07-18T15:00:07Z

Thanks for your review, @brodybits! I've added a test and some documentation.
I opted for a manual test only, since this needs a config.xml preference (even though I added that to the CONTRIBUTING document). It might be useful to add the preference to the regular Cordova documentation at some point in the future (not sure if that is desirable or not).

brodycj · 2018-07-19T01:44:15Z

Thanks @wvengen. Unfortunately I had some real trouble getting a test demo to work on iOS. A quick demo project that shows this functionality working the same way on both Android and iOS (as closely as possible) would be really helpful.

wvengen · 2018-07-19T07:06:50Z

Thanks for testing. Apparently I didn't test it enough, sorry. Still, for me it works on iOS, except that sometimes the alert Result verified is shown twice. That is because, apparently, the loadstop event is being fired for customscheme as well. I don't see how that can be avoided, as in webViewDidFinishLoad (nor the originating method) there is no context about the URL that caused it.

My proposal:

Mention loadstop being called for customscheme in the documentation (an extra iOS quirks in addEventListener).
In the test, only navigate to the custom URL at the first time it happens.

brodycj · 2018-07-20T22:10:13Z

I gotta say that I am still not able to get it to work on either Android or iOS in my test project at https://github.com/brodybits/cordova-iab-custom-scheme-test-wip (with 1 test link and 3 button tests). I suspect that I am doing something wrong; it would be best if you can issue a PR to correct me.

If I would add the proposed version of this plugin using the following command:

cordova plugin add https://github.com/q-m/cordova-plugin-inappbrowser#feature/allowedschemes-ios

then add iOS platform and then run from Xcode, I would get the following console log message on iOS when I click the test link or any test button:

2018-07-20 17:35:38.166670-0400 HelloCordova[12282:282185] Failed to load webpage with error: The URL can’t be shown

I get the following console log output if I click the test 3 button:

2018-07-20 18:05:31.146655-0400 HelloCordova[12987:301089] NSURLConnection finished with error - code -1002
2018-07-20 18:05:31.155721-0400 HelloCordova[12987:299914] Failed to load webpage with error: The URL can’t be shown

Test does not work for me on Android either.

If another Cordova member with more experience with this plugin can review and test I would be grateful.

wvengen · 2018-07-31T13:29:24Z

It took me some experimentation to find the cause, but the page you load in the whitelist (local page), it is loaded in the Cordova WebView instead of InAppBrowser. Make it cordova.InAppBrowser.open('iabpage.html', '_blank') and the example should work.

Some small corrections in: wvengen/cordova-iab-custom-scheme-test-wip@5917542 (should work without) and wvengen/cordova-iab-custom-scheme-test-wip@821652d (makes link work).

brodycj · 2018-07-31T13:35:02Z

Thanks @wvengen I will take another look today.

brodycj · 2018-07-31T20:50:13Z

tests/tests.js

+            }
+        });
+        ref.addEventListener('customscheme', function (e) {
+            if (e && e.url === "custom://test") {


I just discovered today that npm run eslint has a few failures:

/Users/brodybits/Documents/cordova/cordova-plugin-inappbrowser/tests/tests.js 714:32 error Strings must use singlequote quotes 715:1 error Expected indentation of 16 spaces but found 14 indent 715:15 error 'alert' is not defined no-undef 717:1 error Expected indentation of 16 spaces but found 14 indent 717:15 error 'alert' is not defined no-undef 722:13 error 'alert' is not defined no-undef ✖ 6 problems (6 errors, 0 warnings) 3 errors, 0 warnings potentially fixable with the `--fix` option.

I can think of 2 possible solutions for 'alert' is not defined:

use // eslint-disable-line no-undef

preferred: use window.alert (less ugly, more likely to work on Windows platform)

A nice bonus would be to fix alert --> window.alert throughout tests.js.

Thanks! I went for the bonus in 93077ef (hoping it will help motivate you to complete the review ;) ). Tested it still works (Android).

brodycj · 2018-07-31T23:57:49Z

I pulled the changes from wvengen/cordova-iab-custom-scheme-test-wip@821652d (master branch) into my master branch, doesn't seem to help on either Android or iOS. Gotta say I am getting ready to give up on this one. I just pushed one more change to https://github.com/brodybits/cordova-iab-custom-scheme-test-wip to use your q-m:feature/allowedschemes-ios branch for now, it would be great if you could fix the sample for the sake of the user community.

If you want another expert to take a look I suggest you send a request to: mailto:dev@cordova.apache.org

wvengen · 2018-08-01T06:22:55Z

@brodybits thanks for checking again. The important change is adding _blank to window.open, which was not present on my fork's master yet (but added to the docs in this PR). I'll create a PR for your test app with all changes needed to make it work.

Also, I'll look into the eslint output.

brodycj · 2018-08-03T07:24:44Z

Thanks @wvengen for the update, fixing the eslint warnings, and fixing the alert calls. I will probably need 1-2 weeks to take another look. Apologies for the extra delays.

wvengen · 2018-08-03T08:03:28Z

No problem, looking forward to getting this merged, somewhere in the coming weeks if all works out. :)

wvengen · 2018-08-29T10:25:16Z

Hi @brodybits just a reminder, if you see an opportunity to take another look, please don't forget :)

janpio · 2018-10-02T19:43:45Z

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

brodycj

Looks OK to me, merging

brodycj · 2018-10-03T22:50:31Z

@wvengen can you help resolve the conflicts?

wvengen · 2018-10-05T12:08:08Z

Thanks! I'll be happy to rebase on master once #276 is sorted out.

janpio · 2018-10-30T19:28:25Z

#276 is merged, so this can be taken care of now I think.

wvengen · 2018-10-31T06:54:43Z

I will look at it, thanks.

wvengen · 2018-10-31T09:10:05Z

Rebased. Tested on Android (7) and iOS (iPhone 8) emulator.

jonathanli2 · 2019-02-19T20:05:29Z

it looks like this change is only for uiwebview.
Currently wkwebiew does not handle customer url scheme, except for the app store url. Please refer to CDVWKInappbrowser.m's below method

(BOOL)webView:(WKWebView*)theWebView decidePolicyForNavigationAction:(NSURLRequest*)request;

Should the change also handle custom url for wkwebview?

Thanks
Jonathan

timbru31 · 2019-04-24T12:50:27Z

@wvengen could you possibly rebase your change, resolve the conflicts and see if the code has to be migrated/copied to the UIWebView and WKWebView? Thanks a lot!

janpio · 2019-04-24T14:20:37Z

We can take care of (trivial) merge conflicts as this ourselves @timbru31 via the GitHub UI, no need to put the work on the original committer. Just did so.

timbru31 · 2019-04-24T14:47:23Z

I believe it’s not just merge conflict, but adding the code to the WKWebView, too?

…

Sent from my iPhone

On 24. Apr 2019, at 4:20 PM, Jan Piotrowski ***@***.***> wrote: We can take care of (trivial) merge conflicts as this ourselves @timbru31 via the GitHub UI, no need to put the work on the original committer. Just did so. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

janpio · 2019-04-24T15:15:09Z

Yes, that part of course has to be evaluated by @wvengen.
(Maybe @dpa99c can help out there as well?)

wvengen · 2019-06-13T07:23:09Z

Any help is appreciated here, as I'm not at all familiar with WKWebView, and I don't have that much time currently to look into that.

timbru31

Like I've already said, WKWebView is currently missing. - to prevent any accidental merge (since there is already one approval) I'm adding a "request changes".

No worries @wvengen - maybe someone else can finish this PR. Thanks a lot for your contributions!

timbru31 · 2019-06-17T09:52:49Z

src/ios/CDVUIInAppBrowser.m

@@ -439,6 +439,21 @@ - (BOOL)isValidCallbackId:(NSString *)callbackId
    return NO;
 }

+- (BOOL)isAllowedScheme:(NSString*)scheme


This need to be added to the WKWebView, too.

brodycj reviewed Jul 31, 2018

View reviewed changes

wvengen mentioned this pull request Aug 1, 2018

Fix opening InAppBrowser brodycj/cordova-iab-custom-scheme-test-wip#1

Merged

janpio added platform: ios enhancement labels Sep 16, 2018

wvengen force-pushed the feature/allowedschemes-ios branch from 93077ef to 238693b Compare October 1, 2018 14:37

brodycj approved these changes Oct 3, 2018

View reviewed changes

CB-14187: (ios) Change the InAppBrowser to allow custom schemes

0ca4dd7

wvengen force-pushed the feature/allowedschemes-ios branch from 238693b to 0ca4dd7 Compare October 31, 2018 09:06

boynet mentioned this pull request Nov 27, 2018

documenting the custom schemes handling #356

Open

janpio added the topic: custom-url-schemes label Nov 27, 2018

boynet mentioned this pull request Nov 28, 2018

documenting the custom schemes handling #357

Open

3 tasks

Merge branch 'master' into feature/allowedschemes-ios

c6f4f31

timbru31 requested changes Jun 17, 2019

View reviewed changes

NiklasMerz mentioned this pull request Dec 30, 2019

Update InAppBrowser.java to allow custom URI schemes and intents #551

Open

1 task

NiklasMerz force-pushed the master branch from 849b31c to 82bbe29 Compare January 4, 2020 07:55

madhums mentioned this pull request May 2, 2023

Upgrade all dependencies and create a new build for iOS q-m/questionmark-app#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-14187: (ios) Change the InAppBrowser to allow custom schemes #274

CB-14187: (ios) Change the InAppBrowser to allow custom schemes #274

wvengen commented Jul 3, 2018 •

edited

Loading

brodycj commented Jul 17, 2018

wvengen commented Jul 18, 2018

brodycj commented Jul 19, 2018

wvengen commented Jul 19, 2018

brodycj commented Jul 20, 2018

wvengen commented Jul 31, 2018 •

edited

Loading

brodycj commented Jul 31, 2018

brodycj Jul 31, 2018

wvengen Aug 1, 2018

brodycj commented Jul 31, 2018

wvengen commented Aug 1, 2018 •

edited

Loading

brodycj commented Aug 3, 2018

wvengen commented Aug 3, 2018

wvengen commented Aug 29, 2018

janpio commented Oct 2, 2018

brodycj left a comment

brodycj commented Oct 3, 2018

wvengen commented Oct 5, 2018

janpio commented Oct 30, 2018

wvengen commented Oct 31, 2018

wvengen commented Oct 31, 2018

jonathanli2 commented Feb 19, 2019

timbru31 commented Apr 24, 2019

janpio commented Apr 24, 2019

timbru31 commented Apr 24, 2019 via email

janpio commented Apr 24, 2019

wvengen commented Jun 13, 2019

timbru31 left a comment

timbru31 Jun 17, 2019

CB-14187: (ios) Change the InAppBrowser to allow custom schemes #274

Are you sure you want to change the base?

CB-14187: (ios) Change the InAppBrowser to allow custom schemes #274

Conversation

wvengen commented Jul 3, 2018 • edited Loading

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

brodycj commented Jul 17, 2018

wvengen commented Jul 18, 2018

brodycj commented Jul 19, 2018

wvengen commented Jul 19, 2018

brodycj commented Jul 20, 2018

wvengen commented Jul 31, 2018 • edited Loading

brodycj commented Jul 31, 2018

brodycj Jul 31, 2018

Choose a reason for hiding this comment

wvengen Aug 1, 2018

Choose a reason for hiding this comment

brodycj commented Jul 31, 2018

wvengen commented Aug 1, 2018 • edited Loading

brodycj commented Aug 3, 2018

wvengen commented Aug 3, 2018

wvengen commented Aug 29, 2018

janpio commented Oct 2, 2018

brodycj left a comment

Choose a reason for hiding this comment

brodycj commented Oct 3, 2018

wvengen commented Oct 5, 2018

janpio commented Oct 30, 2018

wvengen commented Oct 31, 2018

wvengen commented Oct 31, 2018

jonathanli2 commented Feb 19, 2019

timbru31 commented Apr 24, 2019

janpio commented Apr 24, 2019

timbru31 commented Apr 24, 2019 via email

janpio commented Apr 24, 2019

wvengen commented Jun 13, 2019

timbru31 left a comment

Choose a reason for hiding this comment

timbru31 Jun 17, 2019

Choose a reason for hiding this comment

wvengen commented Jul 3, 2018 •

edited

Loading

wvengen commented Jul 31, 2018 •

edited

Loading

wvengen commented Aug 1, 2018 •

edited

Loading