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

Android WebView “tel:” links show web page not found - issue fixed #8526

Closed
wants to merge 9 commits into from

Conversation

sathyapriya31
Copy link

Submitting PR #6810 - Android WebView “tel:” links show web page not found - issue fixed.
'tel:' link web page not loading:
screenshot_2016-07-01-19-48-05

After Fixing the issue:
screenshot_2016-07-01-19-52-00

@ghost
Copy link

ghost commented Jul 1, 2016

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

@ghost
Copy link

ghost commented Jul 1, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@@ -110,7 +111,18 @@ public void onPageStarted(WebView webView, String url, Bitmap favicon) {
SystemClock.nanoTime(),
createWebViewEvent(webView, url)));
}

@Override
public boolean shouldOverrideUrlLoading(WebView view, String url){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space before {

@satya164
Copy link
Contributor

satya164 commented Jul 5, 2016

I don't think checking for tel: is the right way to do it. What about mailto:, or any custom schemes?

I think we should check if the URL is not http: or https:, then handle it. It'll probably suffice to just use a Intent.ACTION_VIEW for all URLs. In case of tel:, it won't dial the number, but will probably open the dialer with the number.

ReactContext reactContext = (ReactContext) view.getContext();
if(url.startsWith("tel:")) {
Intent intent = new Intent(Intent.ACTION_DIAL, Uri.parse(url));
reactContext.startActivity(intent);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work without the flag Intent.FLAG_ACTIVITY_NEW_TASK? We should use the current activity to start the intent (which doesn't need the flag) if we can. It provides better UX since most of the time the user can press back to come to the previous screen.

@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
ReactContext reactContext = (ReactContext) view.getContext();
Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if it is http or https before handling.

Also does startActivity work without FLAG_NEW_TASK

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
  if (url.startsWith("tel:") || url.startsWith("sms:") || url.startsWith("smsto:") || url.startsWith("mms:") || url.startsWith("mmsto:")) {
    ReactContext reactContext = (ReactContext) view.getContext();
    Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url));
    reactContext.startActivity(intent);
    return true;
  }
  return false;
}

@sathyapriya31
Copy link
Author

@satya164

public boolean shouldOverrideUrlLoading(WebView view, String url) {
        ReactContext reactContext = (ReactContext) view.getContext();
        if(url.startsWith("http")|| url.startsWith("http")) {
          return false;
        } else {
          Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url)); 
          intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
          reactContext.startActivity(intent);   
          return true;   
        }   
    }

Is this Correct? shall i proceed?

@satya164
Copy link
Contributor

It should be url.startsWith("https") || url.startsWith("http"), otherwise looks correct. Please also test that the code works.

@satya164
Copy link
Contributor

Also, do you need to cast this to ReactContext - (ReactContext) view.getContext();? It should work without casting.

@@ -113,13 +113,10 @@ public void onPageStarted(WebView webView, String url, Bitmap favicon) {
}
@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
ReactContext reactContext = (ReactContext) view.getContext();
if(url.startsWith("tel:")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
  if (url.startsWith("tel:") || url.startsWith("sms:") || url.startsWith("smsto:") || url.startsWith("mms:") || url.startsWith("mmsto:")) {
    ReactContext reactContext = (ReactContext) view.getContext();
    Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url));
    reactContext.startActivity(intent);
    return true;
  }
  return false;
}


@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
if(url.startsWith("http")|| url.startsWith("https")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space after if and before ||

@@ -112,6 +114,18 @@ public void onPageStarted(WebView webView, String url, Bitmap favicon) {
}

@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
if (url.startsWith("http") || url.startsWith("https")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to check for http:// and https://

@satya164
Copy link
Contributor

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 13, 2016
@ghost
Copy link

ghost commented Jul 13, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 33a1f28 Jul 13, 2016
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
Submitting PR facebook#6810 -  Android WebView “tel:” links show web page not found - issue fixed.
'tel:' link web page not loading:
![screenshot_2016-07-01-19-48-05](https://cloud.githubusercontent.com/assets/11989113/16525364/b3e9f10c-3fc9-11e6-8119-93cdf24d54df.png)

After Fixing the issue:
![screenshot_2016-07-01-19-52-00](https://cloud.githubusercontent.com/assets/11989113/16525371/c0d74d92-3fc9-11e6-899b-570a940692f6.png)
Closes facebook#8526

Differential Revision: D3554500

fbshipit-source-id: e8cc1ac4c36ddf0c6b261a29b2e038caddc03e75
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
Submitting PR facebook#6810 -  Android WebView “tel:” links show web page not found - issue fixed.
'tel:' link web page not loading:
![screenshot_2016-07-01-19-48-05](https://cloud.githubusercontent.com/assets/11989113/16525364/b3e9f10c-3fc9-11e6-8119-93cdf24d54df.png)

After Fixing the issue:
![screenshot_2016-07-01-19-52-00](https://cloud.githubusercontent.com/assets/11989113/16525371/c0d74d92-3fc9-11e6-899b-570a940692f6.png)
Closes facebook#8526

Differential Revision: D3554500

fbshipit-source-id: e8cc1ac4c36ddf0c6b261a29b2e038caddc03e75
tungdo194 pushed a commit to tungdo194/rn-test that referenced this pull request Apr 28, 2024
Summary:
Submitting PR #6810 -  Android WebView “tel:” links show web page not found - issue fixed.
'tel:' link web page not loading:
![screenshot_2016-07-01-19-48-05](https://cloud.githubusercontent.com/assets/11989113/16525364/b3e9f10c-3fc9-11e6-8119-93cdf24d54df.png)

After Fixing the issue:
![screenshot_2016-07-01-19-52-00](https://cloud.githubusercontent.com/assets/11989113/16525371/c0d74d92-3fc9-11e6-899b-570a940692f6.png)
Closes facebook/react-native#8526

Differential Revision: D3554500

fbshipit-source-id: e8cc1ac4c36ddf0c6b261a29b2e038caddc03e75
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants