Extend IASKAppSettingsWebViewController with data detectors to recognise links, addresses, phone numbers, etc.

[funnel20]

I have upgraded IASK from version 3.8.0 to 3.8.4 in one of my production apps.
It contained a self made WebViewController with a XIB file with a WkWebView in the storyboard.

This has now been replaced by IASKAppSettingsWebViewController and I noticed a difference:

WkWebView shows hyperlinks at street address and phone numbers ✅:

IASKAppSettingsWebViewController does not ❌:

It appears that it occurs in WkWebView, since the dataDetectorTypes property of WKWebViewConfiguration is by default set to WKDataDetectorTypeAll in the Storyboard file:

Although this is no bug at all, we could easily extend IASKAppSettingsWebViewController with a new feature where these data detector(s) to assist users with convenient navigation. Especially since these links will be handled by system apps.

@futuretap The question is whether you want to add this feature or not. And if so, how to implement it?
I'd like to propose 3 suggestions:

1. Implement all datadetectors by default for IASKAppSettingsWebViewController
2. Add a boolean option to the the plist configuration of IASKAppSettingsWebViewController, which switches between no data detectors (default) and all data detectors.
3. Add an option to the plist configuration of IASKAppSettingsWebViewController where multiple detector types can be added, defaults to WKDataDetectorTypeNone.

I would prefer option 1, since I can't think of a use-case where I should disable these user-oriented conveniences. And also since this is the default WkWebView configuration when adding it to a view in Xcode.
But maybe we can ask other IASK users for advice?

[futuretap]

I agree we should implement option 1 with all data detectors enabled. If users don’t want that, they should subclass IASKAppSettingsWebViewController.

[funnel20]

@futuretap Awesome, I'm working on a PR.

[funnel20]

While adding a WKWebViewConfiguration to loadView of IASKAppSettingsWebViewController, the sample app crashes with EXC_BAD_ACCESS when selecting the WebView.

Searching for this leads to https://stackoverflow.com/a/53939210/2439941, where it appears that creating the web view should be postponed to viewDidLoad instead of loadView.

I've renamed loadView to viewDidLoad and of course called [super viewDidLoad].
Indeed the crash is prevented, and the web view loads properly.

I was not aware of loadView at all, since I always use NIB-files or Storyboard.
After having read the Apple docu of loadView, it becomes clear to me that it only creates the view.
And consequently:

If you want to perform any additional initialization of your views, do so in the viewDidLoad() method.

The current code of loadView already contains quite some additional initialization, which didn't crash yet. Basically according to this Apple docu quote, the code sections should be moved to viewDidLoad .

@futuretap How do you want to approach this?

1. Just rename loadView to viewDidLoad, as it seems to work.
2. Add viewDidLoad and move appropriate code sections from loadView to it.

[funnel20]

I agree we should implement option 1 with all data detectors enabled. If users don’t want that, they should subclass IASKAppSettingsWebViewController.

The data detectors are just 1 example of WKWebViewConfiguration, or WKWebView properties, that are enabled by default when adding a WKWebView to Interface Builder. E.g.:

• Display: Link Preview
• JavaScript: Enabled
• Media: Picture-in-Picture

@futuretap What do we want to do with all these properties?

[futuretap]

Regarding -loadView vs. -viewDidLoad, we should keep both. -loadView should create the views, -viewDidLoad should do the web view configuration as this seems to only work there.

I suggest to enable all properties except "Can Auto-open Windows".

[funnel20]

@futuretap Ok, will move specific WKWebView configuration to viewDidLoad.

To have mutual understanding on properties, what do you mean with "enable all properties"?

• All possible options that WKWebView and WKWebViewConfiguration have, based on the docu of them (it are a lot)
• From the above screenshot of Interface Builder: the entire set of options all enabled (e.g. "Media: Inline Playback").
  NOTE: Interface Builder only shows a subset of all possible options. I don't know what the conditions are how Apple selected this subset, hopefully something like most or common used.
• From the above screenshot of Interface Builder: the set of options that are enabled by default in Xcode (e.g. "Media: Airplay")

[futuretap]

I suggested the second option. Let’s use a reasonable default configuration. We could design it subclass friendly by implementing and calling a -configureWebView method that can be easily overridden.

[funnel20]

@futuretap Forgive me if I keep asking annoying questions, but the Requirement Management is always the hardest part.
The second option contradicts with your previous remark:

I suggest to enable all properties except "Can Auto-open Windows".

Is it still option 2, or option 2 except "Can Auto-open Windows"?

For the subclass feature, I prefer to create another issue, so we can focus on single tasks. Is that ok?

[futuretap]

Is it still option 2, or option 2 except "Can Auto-open Windows"?

Option 2 except "Can Auto-open Windows".

Feel free to create another issue for subclassing.

[funnel20]

@futuretap I still have some questions about option 2, after reading the iOS docu.
For instance the 2 checkboxes at "Interaction: for Audio/Video Playback" refers to property mediaTypesRequiringUserActionForPlayback, where we define the media types that require a user gesture to begin playing.
The Apple default in Xcode with both checkboxes disabled, means WKAudiovisualMediaTypeNone.
When you want to enable them both, it means that auto-start of both video and audio is disabled, and the users needs to make a gesture to start it.
Is this really what you want? I prefer option 3, where Apple somehow made some user-oriented considerations of the default values in Xcode.

UPDATE
After testing with WKAudiovisualMediaTypeNone, this appears extremely annoying. The sample app with your URL -> Blog will automatically start a full-screen video. This convinced me to use WKAudiovisualMediaTypeAll.

[futuretap]

Of course you’re right, I agree. I'm confident you’ll choose a sensible configuration that makes sense for most IASK users!

[funnel20]

@futuretap This is the set that I came up with:

https://github.com/funnel20/InAppSettingsKit/blob/ab1689e19ac975529fc0304dc9e1913f9917a6bb/Sources/InAppSettingsKit/Controllers/IASKAppSettingsWebViewController.m#L150-L176

It has been validated on several pages, to check the data detectors and media behaviour.

Please review and let me know any desired changes, or if I can mage a PR.

[futuretap]

Looks good, thanks. Let’s just extract the web view configuration into a separate method that is included in the .h file, for easy overriding.

[funnel20]

Unfortunately configuration is read-only according to docu:

Use the object in this property to obtain information about your web view’s configuration. Because this property returns a copy of the configuration object, changes you make to that object don’t affect the web view’s configuration.

It can only be assigned as initialiser of WKWebView via:

self.webView = [[WKWebView alloc] initWithFrame:CGRectZero configuration:configuration];

So we should assign a method to the .h file, that's why I wanted to open a new ticket for that.

[futuretap]

That’s why I propose to extract the whole code to assign a config to the web view into a -configureWebView method.

[funnel20]

@futuretap Forgive me my stupidity, but how would an IASK user call configureWebView: on IASKAppSettingsWebViewController? Can you provide a code sample, so I can use that to create and test this functionality?

Background: I'm not a pro in subclassing or other complicated tricks. Especially when I turn off my developer hat and put on my user hat, as a user of 3rd party libs I prefer simplicity and ease of use.

An alternative to a method configureWebView: is a property like webViewConfiguration, where we catch the setter in the *.m file, and re-init the webView upon that event. In that way the user can also read the existing configuration from the property and modify it according to its needs.

[futuretap]

A user would create a subclass of IASKAppSettingsWebViewController and override the -configureWebView: method. Then, the subclass name would be referenced by IASKViewControllerClass in the settings plist. To allow calling super from the subclass it’s required to publish the method signature in the .h file. Does that make sense?

@interface MyWebViewController : IASKAppSettingsWebViewController
@end

@implementation MyWebViewController

- (void) configureWebView:(WKWebView*)webView {
   [super configureWebView:webView];
   // perform our own configuration logic
}
@end

[funnel20]

Still not clear to me. Let's say a user wants to disable JavaScript.
In other words he needs to change the configuration property (which is read-only) of webView by changing:

configuration.defaultWebpagePreferences.allowsContentJavaScript = NO;

How should he do this in this proposed override method and how will that be effectuated inside the *.m file?

[futuretap]

The user would then copy the original implementation of -configureWebView: into the subclass and modify just the needed properties. Then, the call to super is not needed. In the code above, I mixed .h and .m (@interface into .h, @implementation into .m).

[funnel20]

I don't know the opinion from other programmers. But in my 15 year of iOS programming I have never ever done this.
For 1 simple reason; maintainability:

In theory; every time a user would update IASK, he should remember to verify the original implementation of -configureWebView: by copying it from the *.m file, redo the customisation in the subclass and do validations. Here I assume that there is only 1 person maintaining the source code. In practise, often multiple people work on it. Is person A aware of informing person B?

To me, this is an unacceptable and unreliable workflow, where the risks are larger than the benefits. In other words; bad practise.

Can't we just add a property to IASKAppSettingsViewController, like existing showDoneButton or showCreditsFooter, or to another IASK class, which we propagate somehow to IASKAppSettingsWebViewController to update the webView configuration?