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

Fix inappbrowser not opening on iOS 13 by using reusable window #534

Merged
merged 7 commits into from
Sep 24, 2019

Conversation

NiklasMerz
Copy link
Member

Closes #492

Platforms affected

iOS

Motivation and Context

Solution for Issue #492 . InAppBrowser closed instantly after opening when using WKWebView and iOS 13 beta.

Description

Like @jcesarmobile suggested the show method for WKWebView now is similar to UIWebView. The hidden option is checked like UIWebView, too.

Please review if this change is makes sense or the other show method was done like that for some reason.

Testing

Run sample app with hidden yes and no.

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

@NiklasMerz
Copy link
Member Author

NiklasMerz commented Sep 6, 2019

@dpa99c You are probably the right person to review this. I cannot add you as a reviewer.

A very short test with the test harness looked good.

@dpa99c
Copy link
Contributor

dpa99c commented Sep 6, 2019

OK will give it a test when I get a mo

@dpa99c
Copy link
Contributor

dpa99c commented Sep 6, 2019

@NiklasMerz I've given your branch a test but it seems to suffer a from a bug I've previously encountered:
after closing the IAB window, the main Cordova app webview is unresponsive to touch so the page UI elements can't be interacted with.

Here's the steps to reproduce:

  • git clone https://github.com/dpa99c/cordova-plugin-inappbrowser-test && cd cordova-plugin-inappbrowser-test
  • cordova plugin add https://github.com/GEDYSIntraWare/cordova-plugin-inappbrowser.git#fix-show-ios13
  • cordova platform add ios
  • connect a device running iOS 13 beta
  • cordova run ios --device
    • or open project in Xcode and run
  • In the app:
    • Press "Open InappBrowser" button
    • Observe IAB window opens and IAB page UI elements are responsive to touch
    • Press Done to dismiss IAB window
    • Observe UI elements of Cordova app webview are unresponsive to touch

It's been a long time since I encountered this bug but it must be related to some of the code you've removed/changed in this PR. I'm pretty sure the bug will occur in other versions of iOS too - IIRC I encountered it in iOS 11/12.

@NiklasMerz
Copy link
Member Author

Unfortunately I can reproduce this. Do you know why the WKIAB is opened differently than UIWebView? The thing with the reusable window seems to fix the closing issue but causes this issue. I guess I need to debug more.

@jcesarmobile
Copy link
Member

I think the problem is the UIWebView open code has been changed a lot of times reintroducing the mentioned bug, so by making them equal you've propagated the bug to the WKWebView implementation.

They key part of the fix was to have a reference to the tmpWindow, so you can do that in the PR without changing the whole code to match the UIWebView code.

@NiklasMerz
Copy link
Member Author

I reverted some changes from the first version. But it is still not working. That's strange, something else must be different UIIAB.

Removed tmpWindow and controller.
This was introduced to fix IAB with WKWebView before WKIAB existed.
@NiklasMerz
Copy link
Member Author

Now I took a completely different approach, that seems to fix this. I dug through the git history and found this commit: f32917d

It looks like the tmpWindow and tmpController stuff was needed to get IAB working before WKInAppBrowser existed 3 years ago. I tested the code that was used before this changed and looks like it is working. Further testing needed!

@NiklasMerz
Copy link
Member Author

Aaaand I found a bug with this attempt. Running browser.close() in the main app does not work anymore.

Unfortunately I will not be able to work on this for some time. Feel free to push to this branch or create PRs. I will follow the conversation and try my best to contribute this when I have time.

@jcesarmobile
Copy link
Member

That was added because the presented view controller was causing the Cordova WKWebView to get frozen while it was presented, so the IAB events (loadstart, loadend, etc.) weren't received in the Cordova window until the modal was dismissed, making them useless.

Did you check if those events work with your approach? ideally should be tested at least with iOS 10 as Cordova and newer.

@NiklasMerz
Copy link
Member Author

That was added because the presented view controller was causing the Cordova WKWebView to get frozen while it was presented, so the IAB events (loadstart, loadend, etc.) weren't received in the Cordova window until the modal was dismissed, making them useless.

Did you check if those events work with your approach? ideally should be tested at least with iOS 10 as Cordova and newer.

I understand, makes sense. That's why close does not work, too. My new approach does not freeze the main window when closing IAB. The old approach freezes the main window while IAB is open.

@NiklasMerz
Copy link
Member Author

I am still on it but this worked in a short test. I won't promise anything. Just another attempt.

Maybe someone with more Objective C experince could review this. I have no idea if this strongSelf stuff is needed for the background thread. This needs more testing with the IAB test app as well.

Fixes close, hide and toolbar 'done' and makes cordova window responsive
@NiklasMerz
Copy link
Member Author

Now it looks good in my app and the iab test app. I used iOS 12.4 and 13.1 to test this. I guess that is somehow the solution but I am not sure about the Objective-C syntax.

Looking forward to reviews and tests!

mobilestar2015 added a commit to mobilestar2015/cordova-plugin-inappbrowser that referenced this pull request Sep 24, 2019
@dpa99c
Copy link
Contributor

dpa99c commented Sep 24, 2019

@NiklasMerz I'm going to take a look at this now (sorry, been real busy with the day job so OSS has had to come a poor second recently)

@dpa99c
Copy link
Contributor

dpa99c commented Sep 24, 2019

@NiklasMerz I've tested your branch with the latest commits and it's looking good.
Closing the IAB window no longer makes the Cordova Webview unresponsive to touch and opening the IAB as hidden then calling show() also works fine.
Great work - especially considering (like me) Objective-C is not oen of your main programming languages.

I have found one edge case issue: if you open the IAB then hide it (instead of closing it) by calling hide(), the Cordova Webview becomes unresponsive to touch when the IAB window is hidden.
To reproduce this, do the following:

  • git clone https://github.com/dpa99c/cordova-plugin-inappbrowser-test && cd cordova-plugin-inappbrowser-test
  • cordova plugin add https://github.com/GEDYSIntraWare/cordova-plugin-inappbrowser.git#fix-show-ios13
  • cordova platform add ios
  • connect a device running iOS 13 beta
  • cordova run ios --device
    • or open project in Xcode and run
  • In the app:
    • Press "Open InappBrowser" button
    • Observe IAB window opens and IAB page UI elements are responsive to touch
    • Press "Hide" button to hide the IAB window
    • Observe UI elements of Cordova app webview are unresponsive to touch

IMHO this is a fairly rare edge case that can be addressed at a later date - we can raise a separate issue to cover the edge case of hiding (instead of closing) the IAB and address it separately.
My opinion is that it's better to merge your branch as it is now to master to fix the main issue which affects the main use case for this plugin on iOS.

@janpio what's your thoughts on this?

@NiklasMerz
Copy link
Member Author

Thank you @dpa99c for your great review.

Regarding the hide issue that's something I forgot to test. In my last commit I moved the code with tmpWindow.hidden = YES to browser exit. This was necessary to catch the programmatic close from within the class CDVInAppBrowser and the close from CDVBrowserViewController which is called by the "done" button in the toolbar. So to fix this it should do to add this to hide or another fitting event. I will try.

@NiklasMerz
Copy link
Member Author

Yes that should work now. I am glad you spotted this bug.

@dpa99c
Copy link
Contributor

dpa99c commented Sep 24, 2019

Will give it another test

@dpa99c
Copy link
Contributor

dpa99c commented Sep 24, 2019

Yep that fixes the hide() issue and I've found no regressions in other areas, so I reckon this is good to merge.

@dpa99c dpa99c merged commit ba345b0 into apache:master Sep 24, 2019
@cding45
Copy link

cding45 commented Oct 4, 2019

I downloaded the master branch and it did resolved the inappbrowser shows and disappears right away issue.

When will it be pushed so we can get it through npm install?

@jeffjulian
Copy link

Is there an ETA on the release and NPM package deployment?

@davidachin
Copy link

@dpa99c much love if you could npm deploy this bad boy :)

@dpa99c
Copy link
Contributor

dpa99c commented Oct 14, 2019

Sorry I don't have the authority/ability to perform an npm release of this plugin

@NiklasMerz
Copy link
Member Author

This is an official Apache plugin. There is a process to follow. I guess there has to happen a vote and someone has to step up to make a release.

I started a discussion on the mailing list. Let's see how that turn out.

@brodycj
Copy link

brodycj commented Oct 15, 2019

I started a discussion on the mailing list. Let's see how that turn out.

Thanks @NiklasMerz it got my attention. I am personally very reluctant to publish any plugins that I do not actively maintain.

I just sent a private email to get someone else to publish this one.

Please feel free to ping us again if needed.

@jcesarmobile jcesarmobile mentioned this pull request Oct 17, 2019
3 tasks
@tygorill
Copy link

Is there any news about an npm install release?

@NiklasMerz
Copy link
Member Author

It's is still in discussion https://lists.apache.org/thread.html/9cf042a3a62fcad899a18759ed40a72087bfafd6cfe6dd48e77f61d1@%3Cdev.cordova.apache.org%3E

You can always add the plugin from github to use this fix.

@GroupeBEL
Copy link

Hello,
Is there any news ? since 2019/10/21 we are still waiting :/

rorygrandin pushed a commit to rorygrandin/cordova-plugin-inappbrowser that referenced this pull request Nov 21, 2019
@NiklasMerz NiklasMerz added this to the 3.2.0 milestone Dec 27, 2019
@gauravgaikwad777
Copy link

gauravgaikwad777 commented Feb 13, 2020

Please add the latest plugin in your config.xml
plugin name="cordova-plugin-inappbrowser" spec="~3.2.0"
This resolved it for me.
Thanks @NiklasMerz

Rouv pushed a commit to Rouv/cordova-plugin-inappbrowser that referenced this pull request Apr 2, 2020
jcesarmobile added a commit to jcesarmobile/cordova-plugin-inappbrowser that referenced this pull request Apr 27, 2020
rorygrandin pushed a commit to rorygrandin/cordova-plugin-inappbrowser that referenced this pull request Apr 28, 2020
@mujthab-hyb
Copy link

this.iab.create(url,"_self") , not working in iOS , how to solve this

@NiklasMerz
Copy link
Member Author

Is the URL in the whitelist? See https://github.com/apache/cordova-plugin-inappbrowser#cordovainappbrowseropen

Please open a new issue with a detailed description. We don't really monitor closed issues and PRs.

@apache apache locked as off-topic and limited conversation to collaborators Jul 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not work ios13 InAppbrowser select target _blank