microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.29k stars 675 forks source link

MapControl in release not working without a call to InitializeMap #9324

Closed nickrandolph closed 6 months ago

nickrandolph commented 7 months ago

Describe the bug

Is the MapControl in the experimental release supposed to be working? All I'm seeing is a blue screen

image

Steps to reproduce the bug

New WinUI application Update to experimental release Use following XAML in MainWindow

<Window
    x:Class="MapSample.MainWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:MapSample"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    mc:Ignorable="d">

    <Grid>
        <MapControl MapServiceToken="XXXXX" />
    </Grid>
</Window>

Expected behavior

Shows a map, or at least throws a meaningful error as why it's not showing anything

Screenshots

image

NuGet package version

1.5.240124002-experimental2

Packaging type

Tried both Packaged and Unpackaged

Windows version

Windows 11

IDE

VS 2022 (Version 17.9.0 Preview 4.0)

dotMorten commented 7 months ago

I hit the same problem. Tried both the token you'd use with UWP mapcontrol and the one you'd use with Azure maps (the latter which it looks like this control is just wrapping the Azure Maps Javascript control in a webview).

dotMorten commented 7 months ago

Looks like initializeMap isn't called. I got things working (thank you open source!!!) with this workaround:

 public MainWindow()
 {
     this.InitializeComponent();
     mapControl.MapServiceToken = mapServiceToken;
     mapControl.Loaded += MapControl_Loaded;
 }

 private void MapControl_Loaded(object sender, RoutedEventArgs e)
 {
     var webView = VisualTreeHelper.GetChild(mapControl, 0) as WebView2;
     webView.NavigationCompleted += WebView_NavigationCompleted;
 }

 private async void WebView_NavigationCompleted(WebView2 sender, Microsoft.Web.WebView2.Core.CoreWebView2NavigationCompletedEventArgs args)
 {
     sender.NavigationCompleted -= WebView_NavigationCompleted;
     var result = await sender.ExecuteScriptAsync($"initializeMap(0,0,\"{mapControl.MapServiceToken}\");");
 }

I'm honestly not quite sure why this fixes it - it seems like this is exactly what the code is already doing.

hez2010 commented 7 months ago

BTW I would like to expect we can have a native MapControl just like what the UWP MapControl was, instead of the current WebView based garbage.

devsko commented 7 months ago

It kind of works for me after upgrading to version 1.5.240205001-preview1. Unfortunately, this kind of map is far from being an adequate replacement for the UWP MapControl.

eduardobragaxz commented 7 months ago

BTW I would like to expect we can have a native MapControl just like what the UWP MapControl was, instead of the current WebView based garbage.

I was taken aback by that. It makes no sense. Oh boy can't wait to see how they bring back InkCanvas now...

dotMorten commented 7 months ago

@eduardobragaxz It's probably worth opening an issue specifically discussing this rather than bringing it up on this unrelated issue, since several are now mentioning it here

eduardobragaxz commented 7 months ago

@dotMorten you're right, my bad. I opened a discussion in the WinUI repo: https://github.com/microsoft/microsoft-ui-xaml/discussions/9323

bkudiess commented 7 months ago

I tested with preview 1 and it works fine, please reopen the issue with a repro app if you can still repro. Here is the code I used: <MapControl x:Name="myMap" Width="800" Height="600" InteractiveControlsVisible="True" MapServiceToken = "foobar" /> This is the link to get a MapServiceToken: https://learn.microsoft.com/en-us/azure/azure-maps/quick-demo-map-app#get-the-subscription-key-for-your-account

nickrandolph commented 7 months ago

Still able to reproduce this issue: image

Target framework: net8.0-windows10.0.19041.0

Package references

    <PackageReference Include="Microsoft.WindowsAppSDK" Version="1.5.240205001-preview1" />
    <PackageReference Include="Microsoft.Windows.SDK.BuildTools" Version="10.0.22621.2428" />

With the workaround @dotMorten suggested, the map shows. Otherwise, just a blue screen (worse if running unpackaged).

dotMorten commented 7 months ago

I'm not seeing Preview1 work any better either without my workaround. both x86 and x64 tested, with and without debugger attached, and both C# and C++. I think this issue should be reopened.

ranjeshj commented 7 months ago

@dotMorten can you share the repro project you have?

dotMorten commented 7 months ago

Here you go: MapLoadRepro.zip

bkudiess commented 7 months ago

@dotMorten can you send more information regarding hardware/os version you tried on? Just tested this repro app and it worked for me.

dotMorten commented 7 months ago

@bkudiess I've tried on 4 different systems, and they all exhibit the same behavior, including on a Surface Laptop Studio 2 and Surface Laptop 5, both with latest Windows 11, and two desktop PCs (both Win11). I don't think this is a hardware/OS specific. Also reported by someone else here

dotMorten commented 6 months ago

Still broken in the final v1.5 release. I've seen this issue referred to in other discussions as well. Surprised this made it to release. My workaround above does still seem to do the trick, but it's by no means ideal.

ranjeshj commented 6 months ago

Still broken in the final v1.5 release. I've seen this issue referred to in other discussions as well. Surprised this made it to release. My workaround above does still seem to do the trick, but it's by no means ideal.

@dotMorten we are still working on getting a repro. Will service a fix to 1.5 once the issue is understood and we have a fix.

dotMorten commented 6 months ago

Since my workaround essentially does what your code does but ensures it delays until webview is ready I’m guessing there’s race conditions at play. But then again I’ve never seen it work, and I don’t know of anyone outside Microsoft who has

ranjeshj commented 6 months ago

Agreed that this needs to be addressed. Does sound like a race condition. Just tried out a simple example

 <Grid>
     <MapControl MapServiceToken=..  />
 </Grid>

and that worked out of the box for me on 1.5 stable. Will ask a few more folks in the team to give it a try to see if someone can repro.

image

If there is some pattern where it works vs where it does not work, that would be helpful.

There are also a couple of issues here. If the key is not correct, then we also get the blue screen, but this seems to be different from that since it works with your workaround for you which means the key is fine.

dotMorten commented 6 months ago

Yes this isn't a keys issue. The blue background is caused by this: https://github.com/microsoft/microsoft-ui-xaml/issues/9377 I'm guessing someone forgot to remove some early debugging style? It would be nice if there was a better error if there was a keys issue, like a callback or message on the map area that the key is invalid or the very least a message in the output window.

riverar commented 6 months ago

I can reproduce this.

The root cause is this check for an alpha-numeric token. There should be no such check and instead rely on error propagation from the server side. (My token has a hyphen in it.)

https://github.com/microsoft/microsoft-ui-xaml/blob/c8bd154c015b914e171a326481fef6532bc943de/controls/dev/MapControl/MapControl.cpp#L131-L133

dotMorten commented 6 months ago

LOLOLOL. I looked at that but didn't think too much of it other than it was an odd check to see if it is valid. I have an underscore in mine. On the upside, this is a win for debuggable sourcecode. I think this and this gave me a bigger pause/understanding.

riverar commented 6 months ago

Workaround: Ask users to recycle their keys until an alpha-numeric-only variant appears. This is of course unacceptable from a security perspective, so hopefully the team is now paying attention to this issue and will service this ASAP.

gegao18 commented 6 months ago

Thank you for reporting this issue (and huge thanks to @riverar for debugging this). We've checked in a fix for our first 1.5 servicing release.

IsValidMapServiceToken is still needed since the token is passed into ExecuteScriptAsync in a string literal. We've updated it to just check for a double quote instead:

bool MapControl::IsValidMapServiceToken()
{
    // This token is passed as a JavaScript param in double quotes. Make sure the token itself doesn't contain any.
    return !MapServiceToken().empty()
        && std::all_of(MapServiceToken().begin(), MapServiceToken().end(), [](wchar_t i){return i != '"';});
}
riverar commented 6 months ago

@gegao18 This change will fail on ", which could be another valid token character (now or in the future). It also misses characters such as \ which will break ExecuteScriptAsync. I would instead consider encoding the input token and skip the testing altogether.

Something like...

- auto token = IsValidMapServiceToken() ? MapServiceToken() : L"";
- auto parameters =  std::format(L"{},\"{}\"", pos, token);
+ auto token = JsonValue::CreateStringValue(MapServiceToken());
+ auto script = std::format(L"initializeMap({},'{}');", pos, token.Stringify().c_str());

  if (auto webView = m_webView.get())
  {
      auto core = webView.CoreWebView2();
      // Initializing Azure Maps on WebView
      // params: longitude, latitude, mapServiceToken
-     returnedValue = co_await core.ExecuteScriptAsync(L"initializeMap(" + parameters + L");");
+     returnedValue = co_await core.ExecuteScriptAsync(script);

      for (uint32_t i = 0; i < Layers().Size(); i++)
      {
          winrt::MapElementsLayer layer = Layers().GetAt(i).as<winrt::MapElementsLayer>();
          OnLayerAdded(layer);
      }
  }

Thanks for the quick triage!

riverar commented 5 months ago

@gegao18 Ping

gegao18 commented 5 months ago

Thanks for pinging. For 1.5.1 we made the quick fix that accepts all but ". For 1.6 we have the full fix that passes an encoded token into the map.

riverar commented 5 months ago

@gegao18 Perfect, thanks again.