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

Dyn 5106 webview2 documentation browser #13212

Merged
merged 8 commits into from
Aug 16, 2022
Merged

Dyn 5106 webview2 documentation browser #13212

merged 8 commits into from
Aug 16, 2022

Conversation

RobertGlobant20
Copy link
Contributor

@RobertGlobant20 RobertGlobant20 commented Aug 12, 2022

Purpose

I replaced the WebBrowser component by WebView2 in the file DocumentationBrowserView.xaml, also I added some functions for initializing WebView2,
The method ShouldAllowNavigation was modified due that WebView2 doesn't have the Navigating event and also the event handler receive different parameters then modified the function to implement the same behavior than the previous one.
Also I noticed that WebView2 CORS was blocking to load the jpg images that are shown for several nodes, then I had to create a mapped virtual directory in this way the html will be able to load the images and show them in the web page.
Finally I removed a tag in all the html that was making the web page compatible for IE based browsers (due that now we are using WebView2 a Chromium based browser).

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Replacing WebBrowser by WebView2 in DocumentationBrowser

Reviewers

@QilongTang

FYIs

I replaced the WebBrowser component by WebView2 in the file DocumentationBrowserView.xaml, also I added some functions for initializing WebView2,
The method ShouldAllowNavigation was modified due that WebView2 doesn't have the Navigating event and also the event handler receive different parameters then modified the function to implement the same behavior than the previous one.
Also I noticed that WebView2 CORS was blocking to load the jpg images that are shown for several nodes, then I had to create a mapped virtual directory in this way the html will be able to load the images and show them in the web page.
Finally I removed a tag in all the html that was making the web page compatible for IE based browsers (due that now we are using WebView2 a Chromium based browser).
@RobertGlobant20
Copy link
Contributor Author

@RobertGlobant20
Copy link
Contributor Author

GIF showing documentation browser with several nodes.
nodes_documentation

@RobertGlobant20
Copy link
Contributor Author

GIF showing the behavior when a link is opened
nodes_documentation_openlink
.

@QilongTang QilongTang added this to the 2.16.0 milestone Aug 12, 2022
await documentationBrowser.EnsureCoreWebView2Async();

//Due that the Web Browser(WebView2 - Chromium) security CORS is blocking the load of resources like images then we need to create a virtual folder in which the image are located.
this.documentationBrowser.CoreWebView2.SetVirtualHostNameToFolderMapping(VIRTUAL_FOLDER_MAPPING, FALLBACK_DOC_DIRECTORY_NAME, CoreWebView2HostResourceAccessKind.DenyCors);
Copy link
Contributor

Choose a reason for hiding this comment

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

From your testing, how costly is this operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang not sure exactly what you mean by costly, but if you refer to the time that takes WebView2 for loading the webpage and the image, at first glance I didn't notice any difference by using the full path "file:///C:/pathtoimage/image.png" against "http://virtualfolder/image.png", if you want I can check how we can measure the loading time and see the differences.

Copy link
Member

Choose a reason for hiding this comment

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

I think you will need to consider how to this works for the other fallback docs folders as well, like the Revit/host one.

internal DirectoryInfo hostDynamoFallbackDocPath;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjkkirschner do you know if when Dynamo is running over Revit both paths (DynamoCoreDirectory, HostApplicationDirectory) could have values (see image below)? or just one and the other will be empty?
Because if only one has value then we should use an if ........ else if ...... structure, right? Just to not follow the same pattern.
image

Copy link
Member

Choose a reason for hiding this comment

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

yes, @RobertGlobant20 I think they will both have values, one folder will contain docs for core nodes, the other docs for the host.

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM with some comments

@@ -14,6 +19,10 @@ public partial class DocumentationBrowserView : UserControl, IDisposable
{
private const string ABOUT_BLANK_URI = "about:blank";
private readonly DocumentationBrowserViewModel viewModel;
private const string FALLBACK_DOC_DIRECTORY_NAME = "fallback_docs";
Copy link
Member

Choose a reason for hiding this comment

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

isn't this string already defined somewhere in this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjkkirschner yes, is defined in PackageDocumentationManager.cs and DocumentationBrowserViewExtension.cs (I just move it here).
Do you have any idea in which place I can place it so we can use it in both places?

Due that the fallback_doc directory path can change depending if Dynamo is executed as Sandbox or if is over Revit then the path can be different so I added a code that will create the virtual directory depending of the correct fallback_doc directory path.
Also I modified the Md2Html class so now it will be inserting in the image source (<img src) the prefix "http://appassets" so it can be loaded from a virtual directory created by the DocumentationBrowser.
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

A few comments then LGTM

@QilongTang
Copy link
Contributor

Fixing CanCreateNodeDocumenationHtmlFromNodeAnnotationEventArgsWithPackageNodeWithAddtionalDocumentation test due that it was using and old format of the html <img> tab, then I updated the image to use the virtual folder and http.
@RobertGlobant20
Copy link
Contributor Author

@RobertGlobant20 Can you take a look at regression DynamoCoreWpfTests.DocumentationBrowserViewExtensionTests.CanCreateNodeDocumenationHtmlFromNodeAnnotationEventArgsWithPackageNodeWithAddtionalDocumentation

@QilongTang I notice that it was using the wrong URL for then I did a fix, let's see if it fixes the test failing.
979f0fc

@QilongTang
Copy link
Contributor

@QilongTang QilongTang merged commit 2686b5d into DynamoDS:master Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants