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] Add JavaScript dialog delegate to WkWebView #4254

Merged
merged 9 commits into from Nov 20, 2018

Conversation

@alanag13
Copy link
Contributor

commented Oct 30, 2018

Description of Change

The default renderer for WebView on iOS uses UIWebView, which handles Javascript-based user dialogs automatically. With WKWebView, these dialogs need to be set up manually with a delate.

This will allow users choosing to "upgrade" to WkWebView to retain the functionality of any javascript dialogs their web source may be using.

Issues Resolved

Platforms Affected

  • iOS

Behavioral/Visual Changes

Without this change, dialogs created via Javascript in the WKWebView would never appear, now they will appear at the same times as they do when using UIWebView.

Testing Procedure

Use the repro steps mentioned in #4253 and the updated UI test to ensure the dialogs appear and behave correctly (confirm logs a bool, prompt logs an input string, etc).

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
@@ -151,7 +153,7 @@ void OnEvalRequested(object sender, EvalRequested eventArg)
async Task<string> OnEvaluateJavaScriptRequested(string script)
{
var result = await EvaluateJavaScriptAsync(script);
return result.ToString();
return result?.ToString();

This comment has been minimized.

Copy link
@alanag13

alanag13 Oct 30, 2018

Author Contributor

Any js statement without a return value will throw NRE without this

@@ -187,12 +189,12 @@ void UpdateCanGoBackForward()
((IWebViewController)WebView).CanGoForward = CanGoForward;
}

class CustomWebViewDelegate : WKNavigationDelegate
class CustomWebViewNavigationDelegate : WKNavigationDelegate

This comment has been minimized.

Copy link
@alanag13

alanag13 Oct 30, 2018

Author Contributor

renamed this class since it was private and there are now multiple delegates

@alanag13

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

I'm not sure whether or not the native dialogs that show up are targetable via automation, but I tried to do what I could re: UI tests. Let me know if you have any questions.

@PureWeen PureWeen requested review from kingces95 and PureWeen Oct 30, 2018

@PureWeen PureWeen added this to In Review in v3.6.0 Oct 30, 2018

@kingces95
Copy link
Member

left a comment

Wonderful! Thanks for the PR. Only functional change was to have cancel return false. As part of that change I deduped some of the logic but that shouldn't have resulted in any functional change.

@alanag13

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

Thanks, I like the dedupe changes a lot better 😄 . Good catch on having cancel return false, that was 100% a result of copy-pasting code.

@PureWeen
Copy link
Contributor

left a comment

One small change to load the correct gallery.

The button on the Javascript Alert View is a lot smaller in the WkWebGallery version which probably doesn't apply to this PR :-)

But figured I'd mention just in case

Xamarin.Forms.Controls/CoreGallery.cs Outdated Show resolved Hide resolved
Load correct galley for WkWebView
Co-Authored-By: alanag13 <alan.grgic@gmail.com>
@alanag13

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@PureWeen the difference in button size is due to WkWebView not applying a default DOCTYPE, and possibly some other things (default zoom level, etc) that UIWebView does for you. In short, the difference in button size here is basically any implementation detail of the two WebViews. Most sites developed for the actual web should have these set in the html already in order to account for browser differences that happen without it. I don't think it's our place to be checking for/inserting missing html just to ensure the two views look the same, but what we could do is simply add those changes to the gallery code so that the galleries look the same. Let me know your thoughts.

@PureWeen

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

@alanag13 if it's easy enough to add to the gallery code to make them look the same I'd say do it. Having that in there I feel would be useful. Plus that button is almost hard to even see small :-)

@alanag13

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@PureWeen done. As an aside, I did notice that I can reproduce #2952 by simply flipping through the gallery views that use HtmlWebViewSource.

@PureWeen PureWeen merged commit 63b3714 into xamarin:master Nov 20, 2018

1 check passed

license/cla All CLA requirements met.
Details

v3.6.0 automation moved this from In Progress to Done Nov 20, 2018

@samhouts samhouts added this to the 4.0.0 milestone Dec 4, 2018

@samhouts samhouts removed this from Done in v3.6.0 Jan 3, 2019

@samhouts samhouts modified the milestones: 4.0.0, 3.5.0 Jan 10, 2019

@samhouts samhouts added this to Done in v3.5.0 Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.