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

(ios) feat: navigationAction in shouldOverrideLoadWithRequest #1333

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ollm
Copy link
Contributor

@ollm ollm commented May 31, 2023

Platforms affected

iOS

Motivation and Context

Currently shouldOverrideLoadWithRequest only returns the request and navigationType, the plugin I'm working with needs to check if navigationAction.sourceFrame is null or not to decide whether to open the url in the browser, currently I can't do that (far as I know).

Description

Added a new shouldOverrideLoadWithRequest:navigationAction selector to have full access to the navigationAction object.

Testing

I have kept the old implementation so that plugins that already use it are not affected, with the change both the old and the new one work correctly.

I don't know if there is a better way to implement this, since my knowledge of Objective-C is basic, or if it would be better to replace completely with the new one, to avoid redundancy.

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

@dpogue
Copy link
Member

dpogue commented May 31, 2023

The problem with this approach is that it exposes WebKit-specific API (by exposing WKNavigationAction) as part of Cordova's API, and we try not to do that so that we don't end up with breaking API changes when Apple decides to change something. We had a bunch of issues in the past due to the deprecation of UIWebView impacting our plugin API and breaking a bunch of 3rd party plugins.

What information about the frame do you need specifically? I'm guessing the security origin?

@ollm
Copy link
Contributor Author

ollm commented May 31, 2023

Currently I only check if sourceFrame is null, in the plugin I'm working on, when clicked on links inside iframes they have to open in the browser, and some website iframes in particular (It happens to me in Youtube Video Iframe and AdSense Ad), when clicked the navigationType it's in .other and not in .linkActivated, I can't tell it apart from other requests like loading external scripts, the only difference I've noticed is that those have the sourceFrame set to null and the others don't.

For my part, just passing the sourceFrame or indicating if it is null would be enough for me.

@ollm
Copy link
Contributor Author

ollm commented May 31, 2023

As an example, this is the code that I am currently testing in the plugin.

@objc func shouldOverrideLoadWithRequest(_ request: URLRequest, navigationAction: WKNavigationAction) -> Bool {

    var allowNavigationsPass = true

    if let url = request.url, url.scheme == "http" || url.scheme == "https" {

        if navigationAction.sourceFrame == nil {
            allowNavigationsPass = false
        }

        switch navigationAction.navigationType {
            case .linkActivated: // This is not triggered in some iframes
                allowNavigationsPass = false
            case .other:
                let range = url.absoluteString.range(of: "utm_content")
                if range != nil {
                    allowNavigationsPass = false
                }
            default:
                break
        }

        if !allowNavigationsPass {
            UIApplication.shared.open(url)
        }
    }

    return allowNavigationsPass
}

@ollm
Copy link
Contributor Author

ollm commented Jun 2, 2023

What do you think about something similar to this? This should avoid the problem of exposing the WebKit API and should allow you to extend it if a plugin needs other information in the future.

@interface CDVWebViewOverrideLoadWithRequest : NSObject

@property(nonatomic, readwrite) NSURLRequest * request;
@property(nonatomic, readwrite) int navigationType;
@property(nonatomic, readwrite) Boolean sourceFrameIsNil;
//@property(nonatomic, readwrite) WKFrameInfo * sourceFrame;
// ...

@end

@implementation CDVWebViewOverrideLoadWithRequest

@end

and

SEL selector2 = NSSelectorFromString(@"shouldOverrideLoadWithRequest:overrideLoadWithRequest:");
if ([plugin respondsToSelector:selector2]) {
    anyPluginsResponded = YES;
    CDVWebViewOverrideLoadWithRequest *overrideLoadWithRequest = [[CDVWebViewOverrideLoadWithRequest alloc] init];
    overrideLoadWithRequest.request = navigationAction.request;
    overrideLoadWithRequest.navigationType = (int)navigationAction.navigationType;
    overrideLoadWithRequest.sourceFrameIsNil = (navigationAction.sourceFrame == nil) ? YES : NO;
    //...
    shouldAllowRequest = (((BOOL (*)(id, SEL, id, id))objc_msgSend)(plugin, selector2, navigationAction.request, overrideLoadWithRequest));
    if (!shouldAllowRequest) {
        break;
    }
}

Sorry if the code is not quite correct, I'm still learning how Objective-C works.

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

2 participants