Skip to content
This repository has been archived by the owner on Apr 2, 2018. It is now read-only.

Swizzle it just a little bit #197

Merged
merged 11 commits into from May 10, 2016

Conversation

tony--
Copy link
Contributor

@tony-- tony-- commented Apr 26, 2016

Fixes #179

This approach amounts to a simplified version of hackishlyHidesInputAccessoryView, but removes all instances of the string UIWebBrowserView (as I believe this string is what was causing the App Store rejections).

This version retains classname checking but only checks for the "UIWeb" prefix in order to avoid referencing the string UIWebBrowserView.
The classname check is needed to avoid swizzling every view in webView.scrollView.subviews (originally I thought I would be able to limit this via respondsToSelector: @selector(inputAccessoryView), but then I realized that every UIView responds to @selector(inputAccessoryView)).

According to this SO post, this classname check will not cause problems with app store.
However, if it is still a concern, the classname check could be removed but this might have a performance impact on some apps that have a lot of subviews (e.g., apps with a lot of images).

@tony-- tony-- changed the title Swizzle it just a little bit Swizzle it just a little bit #179 Apr 26, 2016
@tony-- tony-- changed the title Swizzle it just a little bit #179 Swizzle it just a little bit Apr 26, 2016
@tlancina
Copy link
Contributor

Thanks for the PR! I'm wondering if just replacing the inputAccessoryView method on the existing UIWebBrowserView class is less intrusive that replacing the class entirely? Looking at something like https://github.com/cjpearson/cordova-plugin-keyboard/blob/master/src/ios/CDVKeyboard.m#L118-L146, I feel like it's less code and fails pretty gracefully. Also it's potentially less of a red flag to Apple if at runtime the class itself doesn't change, although from what I can tell they are just searching for string matches.

In the interest of transparency it might make sense to pull this out into its own plugin. So if you do want to hide the bar, you are explicitly opting in to potential future rejection, considering there is no way to do this with public APIs at the moment. Any thoughts?

@tony--
Copy link
Contributor Author

tony-- commented May 3, 2016

I thought about just replacing the method, but wasn't sure how to do it. Now that I see the example, it looks pretty straightforward! Personally, I don't think it will be less of a red flag - I think the rejections are based on string matching, but I like that it is simpler.

I see where you are coming from in wanting to shield users from possible rejection.
Personally, I think avoiding strings that match non-public API and class names will be sufficient to avoid rejections, but isolating this functionality might be reassuring for some users.

Feel free to close this if you want to go with cjpearson's simpler approach and/or want to pull this out into a separate plugin.
Also let me know if you'd like me to update this PR to swizzle in the method only (instead of swapping classes).

@tlancina
Copy link
Contributor

tlancina commented May 3, 2016

Yeah the reason I thought there might be more than just string matching was
because there were no reports (that I know of) of the other plugin, which
had the same string, causing rejections. But I suppose that could be
explained by the difference in usage numbers.

I'll admit to probably overthinking this. If apps were to start getting
rejected again (meaning they are detecting it with more than just string
matching) I think we would be justified in asking our users to open Radars
to prod Apple into exposing, at a minimum, inputAccessoryView for
UI/WKWebView.

If you don't mind updating the PR to only swizzle the method I'll merge it
in and release 2.2.0. Thanks!

On Tue, May 3, 2016 at 12:06 PM Tony Homer notifications@github.com wrote:

I thought about just replacing the method, but wasn't sure how to do it.
Now that I see the example, it looks pretty straightforward! Personally, I
don't think it will be less of a red flag - I think the rejections are
based on string matching, but I like that it is simpler.

I see where you are coming from in wanting to shield users from possible
rejection.
Personally, I think avoiding strings that match non-public API and class
names will be sufficient to avoid rejections, but isolating this
functionality might be reassuring for some users.

Feel free to close this if you want to go with cjpearson's simpler
approach and/or want to pull this out into a separate plugin.

Also let me know if you'd like me to update this PR to swizzle in the
method only (instead of swapping classes).


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#197 (comment)

@tony--
Copy link
Contributor Author

tony-- commented May 4, 2016

The one thing I don't like about this approach is the inclusion of the obfuscated class name strings.
I experimented with some other approaches, but they all had side-effects.
In the end I came back to this approach, including the obfuscated class name strings, but I tweaked it a bit to fix some potential issues and/or (in my view) clean it up.
Specifically, I moved the static IMP references to member variables (and added Method member variables in order to avoid looking them up multiple times) and I moved the initialization to pluginInitialize. I wanted to avoid the possibility of overwriting the original IMP on multiple calls to hideKeyboardAccessoryBar.

@jacquesdev
Copy link

Hi @tony-- and @tlancina, really sorry to be such a pain, but I just wanted to check if you can resolve the conflict and merge it with 2.2.0? I am only asking because it seems the fix is imminent, and I would love to get this back in to our app.

* master:
  Update keyboard.js
  chore(): update to 2.1.0
  added mock for browser platform

# Conflicts:
#	www/ios/keyboard.js
@tony--
Copy link
Contributor Author

tony-- commented May 10, 2016

@jacquesdev Thanks for the comment! I merged the recent changes from master into my PR branch to resolve the conflict.

@tlancina
Copy link
Contributor

@tony-- looks great! Could you remove the swizzled const? I think it's a relic of the first implementation. Or if it's a hassle I can merge it in right now and can remove it as well.

@jacquesdev for future reference you can install any plugin from github at any time: cordova plugin add https://github.com/tony--/ionic-plugin-keyboard.git#swizzle-it-just-a-little-bit.

@tony--
Copy link
Contributor Author

tony-- commented May 10, 2016

@tlancina - right you are, sorry about that! I removed it; also removed the commented out references to UIWebViewExtension from plugin.xml.

@tlancina tlancina merged commit d2ff48e into ionic-team:master May 10, 2016
@tlancina
Copy link
Contributor

@tony-- thank you for your contribution and patience!

@tony--
Copy link
Contributor Author

tony-- commented May 10, 2016

@tlancina glad to have an opportunity to help out a bit!

@jacquesdev
Copy link

Thanks to both of you much appreciated :)

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.

App Rejected due to UIWebBrowserView Non Public API Usage
3 participants