Skip to content

Conversation

pjcabrera
Copy link
Contributor

We have a requirement in our app, to open in mobile Safari, any http:// and https:// links displayed in a web view. Our web view is not full screen and is loaded with an HTML string from the backend. Displaying external content in that web view is outside of the scope of our app, so we open them in mobile Safari.

I've forked the WebView component and added a callback property, shouldStartLoadWithRequest, and modified the RCTWebView implementation of webView:shouldStartLoadWithRequest:navigationType:
to check if the shouldStartLoadWithRequest property is set.

If the property is set, webView:shouldStartLoadWithRequest:navigationType: passes the URL & navigationType to the callback. The callback is then able to ignore the request, redirect it, open a full screen web view to display the URL content, or even deep link to another app with LinkingIOS.openURL().

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @nicklockwood, @donyu and @vjeux to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 23, 2015
@javache
Copy link
Member

javache commented Oct 23, 2015

Since RN is asynchronous, it's really hard to expose this shouldStartLoadWithRequest API correctly. Is setting HTML/URL really equivalent to returning YES from that delegate call? I would assume that would information such as referrer and break things like the back action in the webview.

@pjcabrera
Copy link
Contributor Author

I would rather have the ability for a JS callback to return a promise to the native delegate, but I didn't see how to do this. Any feedback or help on that is appreciated, as that would be the correct implementation.

In this implementation, setting HTML/URL causes the delegate to fire again, and the _forceReload flag skips the JS callback, and returns YES.

I'll have to check if this messes with the web view history and breaks the back/forward.

@javache
Copy link
Member

javache commented Oct 23, 2015

You could consider blocking the main thread this (I believe the underlying webkit call is actually async), and waiting for the JS side to return. We haven't used that pattern anywhere else yet but I'd be curious to see if it works out.

On onShouldStartLoadWithRequest, create a new NSCondition with a certain identifier and call the JS block passing that identifier. While you wait for the JS to return, block the main thread using [NSCondition waitUntilDate:] (usually not recommended as this will block any interaction). On the JS side you should then call back into the native module, e.g. webView.startLoadWithRequest(identifier, true/false), which passes that information and signals the condition.

@pjcabrera
Copy link
Contributor Author

Tried your suggested approach, but blocking the main thread with NSCondition or NSConditionLock blocks RN.

Here's what I tried:

In RCTWebView.m:

#pragma mark - UIWebViewDelegate methods

- (BOOL)webView:(__unused UIWebView *)webView shouldStartLoadWithRequest:(NSURLRequest *)request
 navigationType:(UIWebViewNavigationType)navigationType
{
  // skip these checks for the JS Navigation handler
  if (![request.URL.scheme isEqualToString:RCTJSNavigationScheme] &&
      _onShouldStartLoadWithRequest) {
    NSMutableDictionary *event = [self baseEvent];
    [event addEntriesFromDictionary: @{
      @"url": (request.URL).absoluteString,
      @"navigationType": @(navigationType)
    }];
    return [self onShouldStartLoadWithRequest:event];
  }
  if (_onLoadingStart) {
    // We have this check to filter out iframe requests and whatnot
    BOOL isTopFrame = [request.URL isEqual:request.mainDocumentURL];
    if (isTopFrame) {
      NSMutableDictionary *event = [self baseEvent];
      [event addEntriesFromDictionary: @{
        @"url": (request.URL).absoluteString,
        @"navigationType": @(navigationType)
      }];
      _onLoadingStart(event);
    }
  }

  // JS Navigation handler
  return ![request.URL.scheme isEqualToString:RCTJSNavigationScheme];
}

- (BOOL)onShouldStartLoadWithRequest:(NSMutableDictionary *)event {
  _conditionLock = [[NSCondition alloc] init];
  [_conditionLock lock];
  _onShouldStartLoadWithRequest(event);
  [_conditionLock waitUntilDate:[NSDate dateWithTimeIntervalSinceNow:10]];
  return _startLoadWithRequestResult;
}

- (void)startLoadWithRequest:(BOOL)result {
  [_conditionLock signal];
  _startLoadWithRequestResult = result;
}

In RCTWebViewManager.m:

RCT_EXPORT_METHOD(startLoadWithRequest:(nonnull NSNumber *)reactTag result:(BOOL)result)
{
  [self.bridge.uiManager addUIBlock:^(__unused RCTUIManager *uiManager, RCTSparseArray *viewRegistry) {
    RCTWebView *view = viewRegistry[reactTag];
    if (![view isKindOfClass:[RCTWebView class]]) {
      RCTLogError(@"Invalid view returned from registry, expecting RCTWebView, got: %@", view);
    } else {
      [view startLoadWithRequest:result];
    }
  }];
}

I put a breakpoint in RCTWebViewManager.m, and [view startLoadWithRequest:result]; is only called after waitUntilDate: times out. I tried firing the JS callback on a different thread, but this didn't seem to have any effect.

@javache
Copy link
Member

javache commented Oct 24, 2015

self.bridge.uiManager addUIBlock: will dispatch_async on the main thread, and will be blocked on the -[NSCondition wait]. To make this work, the invoked native module method will have to work off the main thread.

@pjcabrera pjcabrera force-pushed the exposed-callback-for-webview-shouldStartLoadWithRequest branch from 9b8ae4d to 694af52 Compare October 25, 2015 07:38
@facebook-github-bot
Copy link
Contributor

@pjcabrera updated the pull request.

@pjcabrera
Copy link
Contributor Author

@javache I replaced the call to self.bridge.uiManager addUIBlock: with [[NSNotificationCenter defaultCenter] postNotificationName:object:userInfo:] in RCTWebViewManager.m and used [[NSNotificationCenter defaultCenter] addObserverForName:object:queue:usingBlock:] in RCTWebView.m. Works like a charm.

@pjcabrera pjcabrera force-pushed the exposed-callback-for-webview-shouldStartLoadWithRequest branch from 694af52 to ec8bccb Compare October 25, 2015 20:51
@facebook-github-bot
Copy link
Contributor

@pjcabrera updated the pull request.

@pjcabrera
Copy link
Contributor Author

@javache @nicklockwood any interest in further review of this PR? I made the changes that were suggested.

@javache
Copy link
Member

javache commented Oct 30, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/189349541404275/int_phab to review.

@javache
Copy link
Member

javache commented Oct 30, 2015

Thanks @pjcabrera. I iterated on this a bit more, and we're now discussing internally if this is a good way forward for implementing synchronous API's.

@javache javache closed this in 0a290e2 Nov 4, 2015
MattFoley pushed a commit to skillz/react-native that referenced this pull request Nov 9, 2015
Summary: public

Original github title:  Exported a callback for native webview delegate method shouldStartLoadWithRequest

We have a requirement in our app, to open in mobile Safari, any http:// and https:// links displayed in a web view. Our web view is not full screen and is loaded with an HTML string from the backend. Displaying external content in that web view is outside of the scope of our app, so we open them in mobile Safari.

I've forked the WebView component and added a callback property, shouldStartLoadWithRequest, and modified the RCTWebView implementation of `webView:shouldStartLoadWithRequest:navigationType:`
to check if the shouldStartLoadWithRequest property is set.

If the property is set, `webView:shouldStartLoadWithRequest:navigationType:` passes the URL & navigationType to the callback. The callback is then able to ignore the request, redirect it, open a full screen web view to display the URL content, or even deep link to another app with LinkingIOS.openURL().

Original author: PJ Cabrera <[email protected]>
Closes facebook#3643

Reviewed By: nicklockwood

Differential Revision: D2600371

fb-gh-sync-id: 14dfdb3df442d899d9f2af831bbc8d695faefa33
Crash-- pushed a commit to Crash--/react-native that referenced this pull request Dec 24, 2015
Summary: public

Original github title:  Exported a callback for native webview delegate method shouldStartLoadWithRequest

We have a requirement in our app, to open in mobile Safari, any http:// and https:// links displayed in a web view. Our web view is not full screen and is loaded with an HTML string from the backend. Displaying external content in that web view is outside of the scope of our app, so we open them in mobile Safari.

I've forked the WebView component and added a callback property, shouldStartLoadWithRequest, and modified the RCTWebView implementation of `webView:shouldStartLoadWithRequest:navigationType:`
to check if the shouldStartLoadWithRequest property is set.

If the property is set, `webView:shouldStartLoadWithRequest:navigationType:` passes the URL & navigationType to the callback. The callback is then able to ignore the request, redirect it, open a full screen web view to display the URL content, or even deep link to another app with LinkingIOS.openURL().

Original author: PJ Cabrera <[email protected]>
Closes facebook#3643

Reviewed By: nicklockwood

Differential Revision: D2600371

fb-gh-sync-id: 14dfdb3df442d899d9f2af831bbc8d695faefa33
@yedidyak
Copy link
Contributor

Are there any plans to match this API on Android?

@aqnaruto
Copy link

i think you need not to add this method ,if you want to shouldStartLoadWithRequest before you open a webview , you can open a html with js request, then you got response and redirect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants