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

WKWebView Support #105

Merged
merged 22 commits into from Jul 29, 2015
Merged

Conversation

lokimeyburg
Copy link
Collaborator

I've added support for WKWebView which was requested in issue #84.

How does it work:
WKWebView support is opt-in. In order to use WKWebView you need to instantiate the WKWebViewJavascriptBridge. The rest of the WKWebViewJavascriptBridge API is the same as WebViewJavascriptBridge.

  1. Import the header file:
#import "WKWebViewJavascriptBridge.h"
  1. Instantiate WKWebViewJavascriptBridge and with a WKWebView object
WKWebViewJavascriptBridge* bridge = [WKWebViewJavascriptBridge bridgeForWebView:webView handler:^(id data, WVJBResponseCallback responseCallback) {
    // etc
}];

Where to start
Take a look at the WKWebViewJavascriptBridge.m file first as it has most of the logic. The two important differences are that the delegates for WKWebView have been changed and that the response for evaluating Javascript is now returned in a callback.

UIWebView WKWebView Equivalent
didFailLoadWithError didFailNavigation
webViewDidFinishLoad didFinishNavigation
webViewDidStartLoad didStartProvisionalNavigation
shouldStartLoadWithRequest decidePolicyForNavigationAction

How to test this
Pull down the branch and try to run the example application. Try running it in XCode 5 and 6 to test out iOS7 and iOS8 respectively. If you're using Pods, point to this branch and see if it works in one of your existing apps.

Extra

  1. Can someone please test this out on OSX 10.10? I don't have a machine currently running Yosemite.
  2. I'm also currently testing this against a bigger iOS project, Stacker, in the feature/wkwebkit branch to make sure WebViewJavascriptBridge works as advertised.
  3. I think there's lots of room to DRY up the code between WebViewJavascriptBridge and WKWebViewJavascriptBridge however I think that should be left for another PR.

@marcuswestin
Copy link
Owner

Awesome. Thank you!!!

It's too late for me to review this now, but I really look forward to merging.

Awesome stuff dude!

-- while mobile

On Tue, Oct 14, 2014 at 11:49 PM, Loki Meyburg notifications@github.com
wrote:

I've added support for WKWebView which was requested in issue #84.
How does it work:
WKWebView support is op-tin. In order to use WKWebView you need to instantiate the WKWebViewJavascriptBridge. The rest of the WKWebViewJavascriptBridge API is the same as WebViewJavascriptBridge.

  1. Import the header file:
#import "WKWebViewJavascriptBridge.h"
  1. Instantiate WKWebViewJavascriptBridge and with a WKWebView object
    WKWebViewJavascriptBridge* bridge = [WKWebViewJavascriptBridge bridgeForWebView:webView handler:^(id data, WVJBResponseCallback responseCallback) {
    // etc
    }];

Where to start
Take a look at the WKWebViewJavascriptBridge.m file first as it has most of the logic. The two important differences are that the delegates for WKWebView have been changed and that the response for evaluating Javascript is now returned in a callback.
| UIWebView | WKWebView Equivalent |
--- | --- | ---
| didFailLoadWithError | didFailNavigation |
| webViewDidFinishLoad | didFinishNavigation |
| webViewDidStartLoad | didStartProvisionalNavigation |
| shouldStartLoadWithRequest | decidePolicyForNavigationAction |
How to test this
Pull down the branch and try to run the example application. Try running it in XCode 5 and 6 to test out iOS7 and iOS8 respectively. If you're using Pods, point to this branch and see if it works in one of your existing apps.
Extra

  1. Can someone please test this out on OSX 10.10? I don't have a machine currently running Yosemite.
  2. I'm also currently testing this against a bigger iOS project, Stacker, in the feature/wkwebkit branch to make sure WebViewJavascriptBridge works as advertised.
  3. I think there's lots of room to DRY up the code between WebViewJavascriptBridge and WKWebViewJavascriptBridge however I think that should be left for another PR.
    You can merge this Pull Request by running:
    git pull https://github.com/lokimeyburg/WebViewJavascriptBridge wkwebview-support
    Or you can view, comment on it, or merge it online at:
    WKWebView Support #105
    -- Commit Summary --
  • Added the WKWebViewJavascriptBridge class
  • Updated the example app to use WKWebView
  • Improved startup message queue for WKWebView
  • Example app dynamically includes Webkit if iOS 8
  • Added compiler flags to include Yosemite
  • Added README documentation on using WKWebview
  • Added examples on using WKWebView to the README
    -- File Changes --
    M Example Apps/ExampleApp-iOS.xcodeproj/project.pbxproj (8)
    M Example Apps/ExampleApp-iOS/ExampleAppViewController.h (21)
    M Example Apps/ExampleApp-iOS/ExampleAppViewController.m (40)
    M Example Apps/ExampleApp-iOS/main.m (22)
    M Example Apps/ExampleApp.html (1)
    M README.md (18)
    A WebViewJavascriptBridge/WKWebViewJavascriptBridge.h (39)
    A WebViewJavascriptBridge/WKWebViewJavascriptBridge.m (326)
    -- Patch Links --
    https://github.com/marcuswestin/WebViewJavascriptBridge/pull/105.patch
    https://github.com/marcuswestin/WebViewJavascriptBridge/pull/105.diff

    Reply to this email directly or view it on GitHub:
    WKWebView Support #105


- (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
{
NSLog(@"1. Finished navigation");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's delete that little log message :)

@karlbecker
Copy link

Hi @lokimeyburg ,
Great start on this!

I am only using WebViewJavascriptBridge for iOS development, so all my comments are limited to that scope.

Our app is using a WebViewJavascriptBridge to run js business logic in a webview hidden from the user. Using WebViewJavascriptBridge with your branch has this working fine for me, but using WKWebViewJavascriptBridge is not working the same. Check out the most recent commit on these two branches:
https://github.com/karlbecker/ios-js-bizlogic/tree/feature/initial_projects <-- works fine, pops an alert
https://github.com/karlbecker/ios-js-bizlogic/tree/feature/initial_projects_wkwebview <-- doesn't work

I'll keep trying to figure out what's wrong, but any idea quickly looking at my sample project? I think the issue might simply be my use of WebViewJavascriptBridge, but would be happy to have your input if you have a few minutes.

Otherwise, the changes seem pretty straightforward. I want to make sure WKWebViewJavascriptBridge is truly a drop-in replacement for WebViewJavascriptBridge, though, and this sample project is showing it may not quite be that way... or that I am just abusing the bridge in bad ways 😄

Thanks!

@lokimeyburg
Copy link
Collaborator Author

Hi @karlbecker,
Thanks for taking a look! 😃 I'll take a look at your project and see if I can't figure out whats causing the error.

My goal is to have this be a drop-in replacement without having to change your JS. I have it working over on my own project, Stacker, on the feature/wkwebview branch. However, it's by no means an exhaustive example of everything you can do with WebViewJavascriptBridge. So having your project (and hopefully others) test out this pull request will be very helpful in catching all the bugs - thanks!

@karlbecker
Copy link

I have a much bigger and more complicated project using it, too - private repo, though :)
But once this simple example is addressed, I have a feeling the more complicated project will work its way out, too.
Thanks @lokimeyburg !

@karlbecker
Copy link

Note that you'll need to pod install to get that sample app to run - the readme describes what to do.

@lokimeyburg
Copy link
Collaborator Author

🆒 I'll try it out in a few hours.

@lokimeyburg
Copy link
Collaborator Author

Woah, 🆗 , so here's what I've found: javascript alerts aren't working. The problem doesn't seem to be with this pull request or project but rather that I can't seem to get alerts (in wkwebview) working in any of my projects - including yours.

To test try it out just add alert('why wont you work') at the start of your <script> tag in your bridge.html file.

Otherwise, the method that should show the alert is most definitely being called in your code. To make sure I was able to get it to print out some debug logs.

It's probably some simple WKWebView configuration/initializer thingy you and I are missing that isn't allowing WKWebView to show alerts. Feeling pretty stumped.

@marcuswestin
Copy link
Owner

I strongly recommend manipulation the dom to show log output for debugging over using alert. Since the alert UI is synchronous and halts the js execution thread, it adds unnecessary complexity with opaque consequences.

The right place to test wkwebview additions is in the example app in the github repo.

-- while mobile

On Mon, Oct 27, 2014 at 9:16 PM, Loki Meyburg notifications@github.com
wrote:

Woah, 🆗 , so here's what I've found: javascript alerts aren't working. The problem doesn't seem to be with this pull request or project but rather that I can't seem to get alerts (in wkwebview) working in any of my projects - including yours.
To test try it out just add alert('why wont you work') at the start of your <script> tag in your bridge.html file.
Otherwise, the method that should show the alert is most definitely being called in your code. To make sure I was able to get it to print out some debug logs.

It's probably some simple WKWebView configuration/initializer thingy you and I are missing that isn't allowing WKWebView to show alerts. Feeling pretty stumped.

Reply to this email directly or view it on GitHub:
#105 (comment)

@lokimeyburg
Copy link
Collaborator Author

Yup, that's what I did. Here I'm using @karlbecker's project to print DOM elements to the screen to make sure the proper methods are being called.

screen shot 2014-10-27 at 6 20 27 pm

Otherwise, this pull request uses the example project in this repo to make sure everything works as expected (it looks like it does).

@lokimeyburg
Copy link
Collaborator Author

(the 2. initialize function being called output was placed in the method that @karlbecker mentioned wasn't working)

@lokimeyburg
Copy link
Collaborator Author

So the question is ( but isn't the point of this pull request ) does anyone know how to make javascript alerts work in WKWebView?

@lokimeyburg
Copy link
Collaborator Author

@karlbecker
Copy link

Thanks for the feedback!
Indeed @marcuswestin , I agree about avoiding using js alerts to demo things. The reason I had done this was to demo at a presentation that "look, there really is a webview running in this app that you cannot see!" Opening up desktop Safari to show the webpage being changed is good, but seeing the UI inside the app display a simple javascript call drove the point home better I think. The presentation was last night, in fact - a little WebViewJavascriptBridge promotion 😄

And nice find, @lokimeyburg - thanks! I'll test it out a little more, although I have a feeling this will work great now.

@marcuswestin
Copy link
Owner

I've been irresponsibly unresponsive here. I'm sorry guys.

First off: This is a fantastic start! We have a good working reference implementation. Thanks for doing this.

To pull it into the main repo I have 2 big asks, both for maintenance reasons.

  1. WKWebViewJavascriptBridge.{h,m} is largely a copy-paste of WebViewJavascriptBridge.{h,m}. To make it maintainable the WK implementation needs to be moved into the existing files. E.g in WebViewJavascriptBridge.h, there should be a #elif defined supportsWKWebKit section before the #elif defined __IPHONE_OS_VERSION_MAX_ALLOWED section. Similarly, there would probably be a WVJB_PLATFORM_IOS_WKWEBVIEW and appropriate delegate implementation methods in a #if defined WVJB_PLATFORM_IOS_WKWEBVIEW in WebViewJavascriptBridge.m.

  2. Add an ExampleApp-iOS-WKWebView app. Why? Because currently the code is checked by opening and running both ExampleApp-iOS and ExampleApp-OSX. Once we incorporate the WKWebView branch, we'll want to be able to quickly fire up the three different implementations and check that they're all working.

I know this is a lot to ask. If we want WVJB to remain stable and strongly maintainable going forwards, I believe these are necessary improvements. I feel strongly about this, but am also very open to hearing dissenting opinions with alternative constructive suggestions.

Cheers, and thanks again for taking the time to do this!

@lokimeyburg
Copy link
Collaborator Author

Re: WK implementation needs to be moved into the existing files - I tried this but stopped because imho it made the overall code more cluttered and less easy to maintain.

Here's why I thought it made it cluttered:

Whenever you call something from the WebViewJavascriptBridge API, like say [bridge send:@"hello world"] you need to check in the send method whether the instance of bridge is WKWebView or not. This, across a few methods, makes it less clean. Using compiler macros doesn't cut it because, for example, you want to maintain UIWebView & WKWebView support in iOS8.

Also, the methods need to be split up a lot more because WKWebView uses callbacks when evaluating javascript.

Compare:
https://github.com/marcuswestin/WebViewJavascriptBridge/blob/master/WebViewJavascriptBridge/WebViewJavascriptBridge.m#L345
with
https://github.com/lokimeyburg/WebViewJavascriptBridge/blob/wkwebview-support/WebViewJavascriptBridge/WKWebViewJavascriptBridge.m#L247

Personally, if maintainability is what you're after, I would still keep two separate files: WebViewJavascriptBridge and WKWebViewJavascriptBridge and move all the common snippets into a helper class called WebViewJavascriptBridgeBase. This would in my opinion lead to much easier to read and maintain codebase (as opposed to the messiness of trying to force WKWebView support into the same file). Thoughts?

Either way, I'll take another stab at it this week/weekend and see where I get and we can make decision then.

As for: Add an ExampleApp-iOS-WKWebView app - I definitely agree. I was thinking the same thing last night when I was testing out @karlbecker's comment. This would've made it a lot easier.

@karlbecker
Copy link

Note that WKWebView is for both iOS and OS X, so would we want both ExampleApp-iOS-WKWebView and ExampleApp-OSX-WKWebView ?

I like @lokimeyburg 's idea of moving common code into WebViewJavascriptBridgeBase.

UIWebView will likely be deprecated by Apple someday. On that day, we could more simply exclude UIWebView-specific code with this approach.

Due to iOS 8's slow adoption, and the (pretty unlikely) potential for the two different js engines to produce different results on the same iOS version (WKWebView gets Nitro, UIWebView does not), I think we definitely want to keep supporting both UIWebView and WKWebView and allow a user to use one or the other in an iOS 8 app.

@marcuswestin
Copy link
Owner

@lokimeyburg: Splitting into multiple files sounds great, as long as 1: there is very little code duplication, and 2: installation remains as simple as dragging a single WebViewJavascriptBridge folder into your project.

@karlbecker: Yes; either that or make the iOS & OSX example apps both run UIWebView and WKWebView next to each other at the same time. I prefer the latter.

@karlbecker
Copy link

Agreed on ease of installation - and of course, it will all work great with cocoapods (no more dragging folders around).

@marcuswestin I also prefer just two apps, with the different webviews running side by side. For fun, could have basic javascript performance test to demonstrate how fast one is over the other 😄

@lokimeyburg
Copy link
Collaborator Author

Thanks for the feedback guys - I like where things are heading! I'll move duplicate code into a separate WebViewJavascriptBridgeBase class and also tackle having side-by-side webview demos. I'll work on it this week.

@marcuswestin
Copy link
Owner

Looks good loki. iOS app held up well during some quick testing.

Some quick notes:
· There is a compile-time warning in the iOS app. There shouldn't be any.
· There is alot of code duplication. I'd be very happy to merge once OS X is up and running, as long as you're OK with me fixing code duplication and some other small issues I see before releasing a new version.

Nice work dude.

@lokimeyburg
Copy link
Collaborator Author

· as long as you're OK with me fixing code duplication and some other small issues I see before releasing a new version.

I don't have a problem with that 😄

@marcuswestin
Copy link
Owner

Great. As soon as there's a workable OS X demo I'm happy to take it from there.

-- while mobile

On Wed, Nov 12, 2014 at 4:30 PM, Loki Meyburg notifications@github.com
wrote:

· as long as you're OK with me fixing code duplication and some other small issues I see before releasing a new version.

I don't have a problem with that 😄

Reply to this email directly or view it on GitHub:
#105 (comment)

@karlbecker
Copy link

I'll be sure to review this, too (and hopefully contribute a little bit), but I have investigated some additional info about WKWebView lately, and unfortunately it integrates worse than UIWebView into other Cocoa networking APIs. So supporting both is definitely wise, since WKWebView is simply a no-go for some WebViewJavascriptBridge users (myself included), due to WKWebView not supporting NSURLProtocol, among other libraries. More info on StackOverflow, and here's an OpenRadar I would encourage us all to submit to Apple to encourage this pretty important integration.

@marcuswestin
Copy link
Owner

Waaaaat....

Ugh, apple, I love you, but sometimes...

Sigh. Ok. Great research Karl. Thanks for the heads up. It would be super helpful if you could add a note about what you found in the readme to avoid that inevitable gotcha for some people, and have a place to point people when the inevitable related issues happen...

-- while mobile

On Fri, Nov 14, 2014 at 12:03 PM, Karl Becker notifications@github.com
wrote:

I'll be sure to review this, too (and hopefully contribute a little bit), but I have investigated some additional info about WKWebView lately, and unfortunately it integrates worse than UIWebView into other Cocoa networking APIs. So supporting both is definitely wise, since WKWebView is simply a no-go for some WebViewJavascriptBridge users (myself included), due to WKWebView not supporting NSURLProtocol, among other libraries. More info on StackOverflow, and here's an OpenRadar I would encourage us all to submit to Apple to encourage this pretty important integration.

Reply to this email directly or view it on GitHub:
#105 (comment)

@karlbecker
Copy link

You bet, I can add that in.

@lokimeyburg
Copy link
Collaborator Author

Ugh - super lame. Lacking NSURLCache & NSURLProtocol support is a deal breaker for me when it comes to any apps in production. This is most likely because WKWebView is handled in another process (which is why a lot of the methods use callbacks & blocks). Thanks for the research @karlbecker!

Here's a list of other 'gotchas' to watch out for - not sure how reliable the list it is but it's worth a scroll-through: https://github.com/ShingoFukuyama/WKWebViewTips/blob/master/README.md

@karlbecker
Copy link

Great link @lokimeyburg - this one looks really hairy:

Cannot coexist with UIWebView on iOS 7 and below

Will the change in this branch break anyone's submittal?

@lokimeyburg
Copy link
Collaborator Author

I've...

  • Fixed the compiler warning
  • Added an example for OSX
  • Updated the README to warn about missing Cocoa network APIs when using WKWebView

@edgarchu
Copy link

@karlbecker Did you ever find out if that warning is valid?
re:

Cannot coexist with UIWebView on iOS 7 and below

@karlbecker
Copy link

@edgarchu - I haven't tried submitting an app with both UIWebView and WKWebView, so nope, not sure if Apple would reject, if it wouldn't work, or if it would just be a-ok!

@alleus
Copy link

alleus commented Jun 30, 2015

Any updates on this? Would love to use WebViewJavascriptBridge with WKWebView, and this merge request seems well worked trough.

@lokimeyburg
Copy link
Collaborator Author

@alleus Just wanted to add that we've successfully launched a rather large app with both UIWebview and WKWebview without any issue. @marcuswestin what would it take to merge this PR? Make sure it works on iOS 9?

@karlbecker
Copy link

Just wanted to add that we've successfully launched a rather large app with both UIWebview and WKWebview without any issue

👏 👏 👏

And no tricky techniques needed to hide that you used both, correct, @lokimeyburg ?

@lokimeyburg
Copy link
Collaborator Author

Nope no tricks. It's pretty clear in the source code that we're implementing both.

marcuswestin added a commit that referenced this pull request Jul 29, 2015
@marcuswestin marcuswestin merged commit 17fcf7a into marcuswestin:master Jul 29, 2015
@marcuswestin
Copy link
Owner

Sorry for the delay guys. Getting a startup off the ground can be pretty all-consuming :)

@marcuswestin
Copy link
Owner

Wanna take some credit for your hard work and add a PR with updates to README that notes support of WKWebView :)

@marcuswestin
Copy link
Owner

@lokimeyburg You're now a contributor - very well deserved!

Feel free to act on behalf of WVBJ with the best interest of the project in mind :) Keep me in the loop on anything significant though please.

Cheers,
Marcus

@marcuswestin
Copy link
Owner

If you're interested in doing more right now, here's an interesting PR to consider: #133

@marcuswestin
Copy link
Owner

Also looking interesting: #129

@lokimeyburg
Copy link
Collaborator Author

Thanks @marcuswestin! Glad to help! I'll be sure to keep things in order. I'll take a look at those PRs tomorrow.

@marcuswestin
Copy link
Owner

Awesome!

@karlbecker
Copy link

And great job to everyone on an excellent, exciting 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

5 participants