Add prop + event for blacklisting URL-schemes in WebView #10654

cbrevik · 2016-10-31T13:33:52Z

Motivation
See #10055. As the issue says, a non-handled custom URL-scheme will throw an error on Android.
The motivation behind this PR is to make blocking URL-schemes more customizable.

In my specific use-case I do not want the URL to load, but I still need to intercept it. I've added a rejectedUrlSchemes array on WebView, which will let you set which schemes to block from loading, and a onUrlSchemeRejected event to notify you when an URL has been blocked.

On iOS you can normally handle it by implementing onShouldStartLoadWithRequest. But, what I've found is that onShouldStartLoadWithRequest can fail because it depends on an async callback from your JS code. If the code or device is a bit slow for whatever reason, it might not return true or false fast enough for onShouldStartLoadWithRequest.

So, this PR is an attempt to instead provide a synchronous alternative for blocking certain URL-schemes on both Android and iOS.

Test plan

<WebView
  source={{ uri: "customscheme://hello/2" }}
  onUrlSchemeRejected={(event) => console.log(event.nativeEvent)}
  rejectedUrlSchemes={["customscheme"]}
  />

The above should blacklist customscheme and block the above URL from loading. It should also log the given event which has an url and a scheme property.

facebook-github-bot · 2016-10-31T13:34:54Z

By analyzing the blame information on this pull request, we identified @jacobp100 and @spicyj to be potential reviewers.

jacobp100

Seems like a good way to handle this.

jacobp100 · 2016-10-31T14:28:38Z

React/Views/RCTWebView.m

  });
+
+  if (!isJSNavigation && _urlSchemeBlacklist) {
+    for (id scheme in _urlSchemeBlacklist) {


Can we use [_urlSchemeBlacklist containsObject:request.URL.scheme]?

Sure will do!

jacobp100 · 2016-10-31T14:31:07Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

  }

+  @ReactProp(name = "urlSchemeBlacklist")
+  public void setUrlSchemeBlacklist(WebView view, @Nullable ReadableArray urlSchemeBlacklistArray) {


Will this work when changing the urlBlacklistScheme from one non-null value to another non-null value?

If it's from a non-null to another non-null, then the previous value will just be overwritten, right?
But, if you mean from a non-null value to a null value; yeah that would fail here. Will fix.

jacobp100 · 2016-10-31T14:31:52Z

Libraries/Components/WebView/WebView.android.js

+
+    /**
+     * An array defining blacklisted URL-schemes.
+     */


Could we call this something like rejectedUrlSchemes and onUrlSchemeRejected?

As I'm not a native English speaker I was quite unsure about naming here. If "rejected" sounds better I will change it! 👍

cbrevik · 2016-10-31T18:56:06Z

On further testing I've found that the provided testplan only seems to work on iOS. For Android the rejectedUrlSchemes check only kicks in on redirects. Setting an inital source/uri to that custom scheme will throw an error:

Not sure if there is any way around that, because it might depend on a "race" between when the source prop is set, and when the rejectedUrlSchemes prop is set? Also I'm unsure if shouldOverrideUrlLoading is called for the first URL?

cbrevik · 2016-11-01T13:45:12Z

Is the above comment a showstopper @jacobp100?

An alternative could be to refactor the Android rejectedUrlSchemes logic into its own method, and call that in setSource (as well as shouldOverrideUrlLoading) to know wether the URL should be loaded?

jacobp100 · 2016-11-01T16:04:16Z

It’s important that things work as a user would expect, and consistently between platforms, so this does sound like something that needs to be resolved. I’m not too familiar with Android, so I can’t really offer any help. Go with what you think is best!

satya164 · 2016-11-06T05:40:31Z

@cbrevik

On further testing I've found that the provided testplan only seems to work on iOS. For Android the rejectedUrlSchemes check only kicks in on redirects. Setting an inital source/uri to that custom scheme will throw an error

Probably a manual check can be added to handle this before setting the initial URL

cc @javache

cbrevik · 2016-11-06T09:10:10Z

@satya164
I refactored the logic a bit to do that in setSource: https://github.com/facebook/react-native/pull/10654/files#diff-7af4740d627e117c5dbc593e92f4be71R430

jacobp100 · 2016-11-06T19:45:17Z

@cbrevik Somebody opened up a PR at #10772. Could you comment on whether this PR would achieve what you were wanting to with your PR?

rseemann · 2016-11-06T21:01:26Z

@jacobp100 I would say it would. Checking for schema is one of the things that one can achieve with my implementation

cbrevik · 2016-11-07T08:35:56Z

@rseemann @jacobp100 I've commented in the other issue.

hramos · 2016-11-11T19:43:07Z

I'm going through open PRs to see what can be merged. Looks like @10772 is still open, so I'm going to leave this one open as well.

mkonicek · 2016-11-23T16:52:45Z

You might have to rebase on #10903 which just landed.

mkonicek · 2016-11-23T16:53:20Z

Libraries/Components/WebView/WebView.ios.js

     */
    scalesPageToFit: PropTypes.bool,

+    /**


This comment doesn't add any info to the name onUrlSchemeRejected below.

Can you please explain what "rejected" means?

mkonicek · 2016-11-23T16:53:46Z

Libraries/Components/WebView/WebView.ios.js

+    onUrlSchemeRejected: PropTypes.func,
+
+    /**
+     * An array defining rejected URL-schemes.


This comment doesn't add any info to the name rejectedUrlSchemes below.

Can you please explain what "rejected" means?

mkonicek · 2016-11-23T17:00:17Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

+
+  private static String getUrlScheme(String url) {
+    if (url.contains("://"))
+      return url.split("://")[0];


I'm pretty sure there's a method for this in the SDK.

mkonicek · 2016-11-23T17:01:46Z

This seems similar to #10772.

jacobp100 · 2016-11-23T17:25:11Z

It is, the discussion has moved over to there to ensure that PR handles this use case.

lacker · 2017-02-08T22:43:08Z

OK, it seems like we should close this PR in favor of #10772 then.

bitfabrikken · 2018-12-12T10:18:00Z

@lacker

OK, it seems like we should close this PR in favor of #10772 then.

That PR was closed a while ago, appears it was abandoned. Any chance of reviving this one? Would be very nice to be able to handle mailto: links and other such things, such as tel:, etc.

Add prop + event for blacklisting URL-schemes in WebView

876024b

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 31, 2016

jacobp100 reviewed Oct 31, 2016

View reviewed changes

Rename props to rejected + refactor some native code

98ba789

jacobp100 approved these changes Oct 31, 2016

View reviewed changes

Merge branch 'master' into url-scheme-blacklist

c71a009

Refactor rejected logic and add to setSource on Android

0eeb21a

jacobp100 approved these changes Nov 1, 2016

View reviewed changes

Fix URL overridden on rejection

d1b87ad

jacobp100 approved these changes Nov 1, 2016

View reviewed changes

cbrevik mentioned this pull request Nov 5, 2016

[Android] WebView custom URL scheme, crashes if not supported #10055

Closed

jacobp100 mentioned this pull request Nov 6, 2016

Blockable WebView #10772

Closed

mkonicek reviewed Nov 23, 2016

View reviewed changes

hramos added the GH Review: needs-revision label Dec 22, 2016

hramos removed the GH Review: review-needed label Dec 22, 2016

lacker closed this Feb 8, 2017

Add prop + event for blacklisting URL-schemes in WebView #10654

Add prop + event for blacklisting URL-schemes in WebView #10654

Uh oh!

Conversation

cbrevik commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Oct 31, 2016

Uh oh!

jacobp100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbrevik commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrevik commented Nov 1, 2016

Uh oh!

jacobp100 commented Nov 1, 2016

Uh oh!

satya164 commented Nov 6, 2016

Uh oh!

cbrevik commented Nov 6, 2016

Uh oh!

jacobp100 commented Nov 6, 2016

Uh oh!

rseemann commented Nov 6, 2016

Uh oh!

cbrevik commented Nov 7, 2016

Uh oh!

hramos commented Nov 11, 2016

Uh oh!

mkonicek commented Nov 23, 2016

Uh oh!

mkonicek Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Nov 23, 2016

Uh oh!

jacobp100 commented Nov 23, 2016

Uh oh!

lacker commented Feb 8, 2017

Uh oh!

bitfabrikken commented Dec 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

cbrevik commented Oct 31, 2016 •

edited

Loading

cbrevik commented Oct 31, 2016 •

edited

Loading

mkonicek Nov 23, 2016 •

edited

Loading