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

[Catalyst] Allow to use a custom WKUIDelegate on WebView #18483

Closed
wants to merge 10 commits into from

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Error with a simple fix that I have seen doing triage taking a look to new issues. Allow to use a custom WKUIDelegate on WebView

Issues Fixed

Fixes #18394

@jsuarezruiz jsuarezruiz added t/bug Something isn't working platform/macOS 🍏 macOS / Mac Catalyst area-controls-webview labels Nov 2, 2023
@jsuarezruiz jsuarezruiz requested a review from a team as a code owner November 2, 2023 13:01
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Nov 2, 2023
@PureWeen PureWeen added this to the Under Consideration milestone Nov 2, 2023
@mattleibow
Copy link
Member

Do we have any tests for this - even for iOS?

@tekmun
Copy link

tekmun commented Nov 3, 2023

Line 31 should be || MacCatalyst, not && MacCatalyst.
With AND operation, the conditional statement will not work on iOS and Mac. Please kindly change it! Thanks.

@tekmun
Copy link

tekmun commented Nov 5, 2023

Do we have any tests for this - even for iOS?

Yes. Use this project.
#17431

Discussion was done in https://stackoverflow.com/questions/77170252/requestcapturemediapermission-for-wkwebview-in-maui-ios/77200328

@mattleibow
Copy link
Member

I mean do we have any unit or device tests in Maui? The fact that this PR is green and the wrong condition means that we don't and should. All changes should have a test to ensure that it never breaks again.

@tekmun
Copy link

tekmun commented Nov 7, 2023

With current main repository, Line 15 on Platforms/iOS/WebViewUIDelegate.cs of this project https://github.com/tekmun/WebViewIssue.git will be called for iOS.

I have not tested it on Mac though. Can someone test whether with the suggested changes, Line 15 on Platforms/iOS/WebViewUIDelegate.cs will be called on Mac/MacCatalyst? In theory, it should.

If it works, then this project https://github.com/tekmun/WebViewIssue.git can be easily converted to become a unit test because it needs to ensure Line 15 on Platforms/iOS/WebViewUIDelegate.cs and Platforms/MacCatalyst/WebViewUIDelegate.cs are called.

@tekmun
Copy link

tekmun commented Nov 9, 2023

Can this change make it into GA next week?

@tekmun
Copy link

tekmun commented Nov 10, 2023

This change is not destructive, meaning that it will not affect systems that are working now. It just allows the MacCatalyst app to have the same functionality as the iOS app. Please kindly merge the changes before GA next week. Thanks.

@tekmun
Copy link

tekmun commented Nov 13, 2023

Any updates on when the merging of this code into the main repository?

@jsuarezruiz
Copy link
Contributor Author

I mean do we have any unit or device tests in Maui? The fact that this PR is green and the wrong condition means that we don't and should. All changes should have a test to ensure that it never breaks again.

Require a custom WKUIDelegate and handlers, already added a sample in the Gallery.

@tekmun
Copy link

tekmun commented Nov 20, 2023

Any updates on when the merging of this code into the main repository?
I understand maui is now a nuget package and we can use to download a nightly build. Can I test it out soon?

@tekmun
Copy link

tekmun commented Nov 23, 2023

It looks like the test case has been added and all checks have passed. Can the reviewers approve this request into the main branch?

@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Dec 7, 2023
@jsuarezruiz jsuarezruiz removed the stale Indicates a stale issue/pr and will be closed soon label Jan 8, 2024
@tekmun
Copy link

tekmun commented Jan 13, 2024

The requested change has not been reflected in the main branch. Why is request marked as DONE?

@jsuarezruiz
Copy link
Contributor Author

Yay, build is passing. Could we get another review here?

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

I don't think we need this

@@ -28,7 +28,7 @@ public partial class WebViewHandler : IWebViewHandler
[nameof(WebViewClient)] = MapWebViewClient,
[nameof(WebChromeClient)] = MapWebChromeClient,
[nameof(WebView.Settings)] = MapWebViewSettings
#elif __IOS__
#elif __IOS__ || MACCATALYST
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this does anything in our code

Semi-confusingly

We already build everything marked as IOS for MACCatalyst
https://github.com/dotnet/maui/blob/main/src/MultiTargeting.targets#L99-L102

I'm not really sure what the users problem is but I don't think this will fix anything for them.

I don't quite understand on the repro, why they are setting the delegate inside a "BeginInvoke" call.

Like, if I check the type of the UIDelegate it's definitely already being replaced

image

you can also see it here that it's already set
image

@PureWeen
Copy link
Member

#18483 (comment)

@PureWeen PureWeen closed this Apr 12, 2024
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-webview platform/macOS 🍏 macOS / Mac Catalyst t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MapWKUIDelegate conditional compilation for MacCatalyst
6 participants