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 5107 webview2 update in guided tours #13142

Merged
merged 3 commits into from
Aug 3, 2022
Merged

Dyn 5107 webview2 update in guided tours #13142

merged 3 commits into from
Aug 3, 2022

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

I replaced the WebBrowser component used in Guided Tours by WebView2.
The WebBrowserWindow was removed due that the component was inserted directly in the PopupWindow.
Also I noticed that some code for hiding the Guided Tour Popups when Dynamo is in background was producing a crash in the WebView2 component, then I removed the code in GuidesManager.cs.
In PopupWindow.xaml.cs added the code needed for loading the html page in the WebView2 compoonent.

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

I replaced the WebBrowser component used in Guided Tours by WebView2.

Reviewers

@QilongTang

FYIs

I replaced the WebBrowser component used in Guided Tours by WebView2.
The WebBrowserWindow was removed due that the component was inserted directly in the PopupWindow.
Also I noticed that some code for hiding the Guided Tour Popups when Dynamo is in background was producing a crash in the WebView2 component, then I removed the code in GuidesManager.cs.
@RobertGlobant20
Copy link
Contributor Author

This is a GIF showing the Package guide using the WebView2 component
PackagesGuideWebView2
.

webBrowserWindow.IsOpen = false;
if (webBrowserComponent != null)
{
webBrowserComponent.Visibility = Visibility.Collapsed;
Copy link
Contributor

Choose a reason for hiding this comment

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

@RobertGlobant20 Can you explain when is the webBrowserComponent disposed?

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 the WebView2 control is created every time the Popup is opened and disposed when the Popup is Hidden (specifically in the Step.Hide() method that is executed when we click the next or prev Popup button), the problem with Popup.IsOpen = false is that it generates an async call to Popup.OnClose() so if we don't dispose the WebView2 instance before we close the PackageManagerSearch, the WebView2 crashes (in the image below you will see that we set IsOpen = false and then we execute the UIAutomation Steps - inside one of this we are closing PackageManagerSearch).
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@RobertGlobant20 Gotcha, thanks!

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

/// <param name="bodyHtmlPage">Html page string</param>
/// <param name="resources">Resources to be loaded</param>
/// <returns></returns>
private string LoadResouces(string bodyHtmlPage, Dictionary<string, string> resources)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a good thing is to add those methods in the Utils class, so they can be reused. (For example in our notifications extension method). Let me know your opinion about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The methods LoadWebBrowser, LoadResouces and LoadResourceAndReplaceByKey were moved to the ResourceUtilities class so they can be used in other places, please check the next commit:
f8c0b4b

Copy link
Contributor

@filipeotero filipeotero left a comment

Choose a reason for hiding this comment

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

LGTM

@QilongTang QilongTang added this to the 2.16.0 milestone Aug 2, 2022
The methods LoadWebBrowser, LoadResouces and LoadResourceAndReplaceByKey were moved to the ResourceUtilities class so they can be used in other places
@QilongTang
Copy link
Contributor

Thank you @RobertGlobant20 , I will need to build your fork to understand the performance impact. Although we talked about replacing the browser for Guided Tour being easiest :) My gut feeling is that, it actually impact performance most since other places, the WebView2 component will not be initialized and disposed with such frequency..

@RobertGlobant20
Copy link
Contributor Author

Thank you @RobertGlobant20 , I will need to build your fork to understand the performance impact. Although we talked about replacing the browser for Guided Tour being easiest :) My gut feeling is that, it actually impact performance most since other places, the WebView2 component will not be initialized and disposed with such frequency..

@QilongTang yes, as you said I was thinking that this should be the easiest one but there was an issue happening when passing from Step7 to Step8 in the Packages guide that took me more time.
As I mentioned before the WebView2 instance was crashing, the main problem was that the code that I added for hiding the Popups when Dynamo is in background didn't allow me to see the real bug then I was able to see that the real problem was not disposing the WebView2 component in a specific time. We need to dispose the WebView2 instance because when showing the Step7 the Popup.PlacementTarget = PackageManagerSearch but when passing to Step8 we close the Popup trying to open a new one, I guess .NET has a mechanism that tries to call the WebView2.IsVisible() method when the PackageManagerSearch is closed (due that is the Popup.PlacementTarget) but the WebView2 instance was already disposed when we closed the Popup setting the IsOpen = false (async call to the protected Popup.OnClose() method), then that's the reason why it was crashing, after disposing the WebView2 it worked correctly.

Something that we will need to research is what happens when the WebView2 is hidden or collapsed, since I noticed that the CoreWebView2 is disposed automatically (also applies if is inside a Popup and is hidden, the CoreWebView2 instance is disposed), so we need to re-initialized again calling EnsureCoreWebView2Async and reloading the page, so probably in flows in which we are hiding the LibraryView or the DocumentationBrowser we will need to do that.

@QilongTang QilongTang merged commit 9316af8 into DynamoDS:master Aug 3, 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