Skip to content

Commit

Permalink
Make CoreWebView2 handlers robust around WebView2 destruction (#8969)
Browse files Browse the repository at this point in the history 
* Harden all CoreWebView2 handlers with weak refs to WV2 object

* fix strongThis comparison

* Fix compariosn of strongThis with nextElement (strongThis->try_as<winrt::WebView2> always gives null)

* Disable consistently failing RadioButtons.AccessKeys test

* Update contribution_workflow.md - we no longer use FabricBot

---------

Co-authored-by: Dmitriy Komin <[email protected]>
Co-authored-by: Keith Mahoney <[email protected]>
  • Loading branch information
@DmitriyKomin @kmahone
3 people authored Oct 20, 2023
1 parent 4873563 commit 00b8713
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 91 deletions.
1 change: 1 addition & 0 deletions dev/RadioButtons/InteractionTests/RadioButtonsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,7 @@ public void ScrollViewerSettingSelectionDoesNotMoveFocus()
}
[TestMethod]
[TestProperty("TestSuite", "B")]
[TestProperty("Ignore", "True")] // Bug 47130840: [WinUI2] RadioButtons.AccessKeys test is failing
public void AccessKeys()
{
if (!PlatformConfiguration.IsOsVersionGreaterThanOrEqual(OSVersion.Redstone3))
Expand Down
192 changes: 106 additions & 86 deletions dev/WebView2/WebView2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,137 +521,157 @@ HWND WebView2::GetActiveInputWindowHwnd() noexcept
void WebView2::RegisterCoreEventHandlers()
{
m_coreNavigationStartingRevoker = m_coreWebView.NavigationStarting(winrt::auto_revoke, {
[this](auto const&, winrt::CoreWebView2NavigationStartingEventArgs const& args)
[weakThis{ get_weak() }](auto const&, winrt::CoreWebView2NavigationStartingEventArgs const& args)
{
// Update Uri without navigation
UpdateSourceInternal();
FireNavigationStarting(args);
if (auto strongThis = weakThis.get())
{
// Update Uri without navigation
strongThis->UpdateSourceInternal();
strongThis->FireNavigationStarting(args);
}
} });

m_coreSourceChangedRevoker = m_coreWebView.SourceChanged(winrt::auto_revoke, {
[this](auto const&, winrt::CoreWebView2SourceChangedEventArgs const& args)
[weakThis{ get_weak() }](auto const&, winrt::CoreWebView2SourceChangedEventArgs const& args)
{
// Update Uri without navigation
UpdateSourceInternal();
if (auto strongThis = weakThis.get())
{
strongThis->UpdateSourceInternal();
}
} });

m_coreNavigationCompletedRevoker = m_coreWebView.NavigationCompleted(winrt::auto_revoke, {
[this](auto const&, winrt::CoreWebView2NavigationCompletedEventArgs const& args)
[weakThis{ get_weak() }](auto const&, winrt::CoreWebView2NavigationCompletedEventArgs const& args)
{
FireNavigationCompleted(args);
if (auto strongThis = weakThis.get())
{
strongThis->FireNavigationCompleted(args);
}
} });

m_coreWebMessageReceivedRevoker = m_coreWebView.WebMessageReceived(winrt::auto_revoke, {
[this](auto const&, winrt::CoreWebView2WebMessageReceivedEventArgs const& args)
[weakThis{ get_weak() }](auto const&, winrt::CoreWebView2WebMessageReceivedEventArgs const& args)
{
// Fire the MUXC side NavigationCompleted event when the CoreWebView2 event is received.
FireWebMessageReceived(args);
if (auto strongThis = weakThis.get())
{
strongThis->FireWebMessageReceived(args);
}
} });

m_coreMoveFocusRequestedRevoker = m_coreWebViewController.MoveFocusRequested(winrt::auto_revoke, {
[this](auto const&, const winrt::CoreWebView2MoveFocusRequestedEventArgs& args)
[weakThis{ get_weak() }](auto const&, const winrt::CoreWebView2MoveFocusRequestedEventArgs& args)
{
winrt::CoreWebView2MoveFocusReason moveFocusRequestedReason{ args.Reason() };

if (moveFocusRequestedReason == winrt::CoreWebView2MoveFocusReason::Next ||
moveFocusRequestedReason == winrt::CoreWebView2MoveFocusReason::Previous)
if (auto strongThis = weakThis.get())
{
winrt::FocusNavigationDirection xamlDirection{ moveFocusRequestedReason == winrt::CoreWebView2MoveFocusReason::Next ?
winrt::FocusNavigationDirection::Next : winrt::FocusNavigationDirection::Previous };
winrt::FindNextElementOptions findNextElementOptions;
winrt::CoreWebView2MoveFocusReason moveFocusRequestedReason{ args.Reason() };

auto contentRoot = [this]()
if (moveFocusRequestedReason == winrt::CoreWebView2MoveFocusReason::Next ||
moveFocusRequestedReason == winrt::CoreWebView2MoveFocusReason::Previous)
{
if (auto thisXamlRoot = try_as<winrt::IUIElement10>())
winrt::FocusNavigationDirection xamlDirection{ moveFocusRequestedReason == winrt::CoreWebView2MoveFocusReason::Next ?
winrt::FocusNavigationDirection::Next : winrt::FocusNavigationDirection::Previous };
winrt::FindNextElementOptions findNextElementOptions;

auto contentRoot = [strongThis]()
{
if (auto xamlRoot = thisXamlRoot.XamlRoot())
if (auto thisXamlRoot = strongThis->try_as<winrt::IUIElement10>())
{
return xamlRoot.Content();
if (auto xamlRoot = thisXamlRoot.XamlRoot())
{
return xamlRoot.Content();
}
}
}
return winrt::Window::Current().Content();
}();
return winrt::Window::Current().Content();
}();

if (contentRoot)
{
findNextElementOptions.SearchRoot(contentRoot);
winrt::DependencyObject nextElement = [=]() {
if (SharedHelpers::Is19H1OrHigher())
{
return winrt::FocusManager::FindNextElement(xamlDirection, findNextElementOptions);
}
else
if (contentRoot)
{
findNextElementOptions.SearchRoot(contentRoot);
winrt::DependencyObject nextElement = [=]() {
if (SharedHelpers::Is19H1OrHigher())
{
return winrt::FocusManager::FindNextElement(xamlDirection, findNextElementOptions);
}
else
{
return winrt::FocusManager::FindNextElement(xamlDirection);
}
}();
if (nextElement && nextElement.try_as<winrt::WebView2>() == *(strongThis.get()))
{
return winrt::FocusManager::FindNextElement(xamlDirection);
// If the next element is this webview, then we are the only focusable element. Move focus back into the webview,
// or else we'll get stuck trying to tab out of the top or bottom of the page instead of looping around.
strongThis->MoveFocusIntoCoreWebView(moveFocusRequestedReason);
args.Handled(TRUE);
}
}();
if (nextElement && nextElement.try_as<winrt::WebView2>() == *this)
{
// If the next element is this webview, then we are the only focusable element. Move focus back into the webview,
// or else we'll get stuck trying to tab out of the top or bottom of the page instead of looping around.
MoveFocusIntoCoreWebView(moveFocusRequestedReason);
args.Handled(TRUE);
}
else if (nextElement)
{
// If core webview is also losing focus via something other than TAB (web LostFocus event fired)
// and the TAB handling is arriving later (eg due to longer MOJO delay), skip manually moving Xaml Focus to next element.
if (m_webHasFocus)
else if (nextElement)
{
const auto _ = [=]() {
if (SharedHelpers::Is19H1OrHigher())
{
return winrt::FocusManager::TryMoveFocusAsync(xamlDirection, findNextElementOptions);
}
else
{
return winrt::FocusManager::TryMoveFocusAsync(xamlDirection);
}
}();
m_webHasFocus = false;
// If core webview is also losing focus via something other than TAB (web LostFocus event fired)
// and the TAB handling is arriving later (eg due to longer MOJO delay), skip manually moving Xaml Focus to next element.
if (strongThis->m_webHasFocus)
{
const auto _ = [=]() {
if (SharedHelpers::Is19H1OrHigher())
{
return winrt::FocusManager::TryMoveFocusAsync(xamlDirection, findNextElementOptions);
}
else
{
return winrt::FocusManager::TryMoveFocusAsync(xamlDirection);
}
}();
strongThis->m_webHasFocus = false;
}

args.Handled(TRUE);
}

args.Handled(TRUE);
}
}

// If nextElement is null, focus is maintained in Anaheim by not marking Handled.
}
} // If nextElement is null, focus is maintained in Anaheim by not marking Handled.
} });

m_coreProcessFailedRevoker = m_coreWebView.ProcessFailed(winrt::auto_revoke, {
[this](auto const&, winrt::CoreWebView2ProcessFailedEventArgs const& args)
[weakThis{ get_weak() }](auto const&, const winrt::CoreWebView2ProcessFailedEventArgs& args)
{
winrt::CoreWebView2ProcessFailedKind coreProcessFailedKind{ args.ProcessFailedKind() };
if (coreProcessFailedKind == winrt::CoreWebView2ProcessFailedKind::BrowserProcessExited)
if (auto strongThis = weakThis.get())
{
m_isCoreFailure_BrowserExited_State = true;
winrt::CoreWebView2ProcessFailedKind coreProcessFailedKind{ args.ProcessFailedKind() };
if (coreProcessFailedKind == winrt::CoreWebView2ProcessFailedKind::BrowserProcessExited)
{
strongThis->m_isCoreFailure_BrowserExited_State = true;

// CoreWebView2 takes care of clearing the event handlers when closing the host,
// but we still need to reset the event tokens
UnregisterCoreEventHandlers();
// CoreWebView2 takes care of clearing the event handlers when closing the host,
// but we still need to reset the event tokens
strongThis->UnregisterCoreEventHandlers();

// Null these out so we can't try to use them anymore
m_coreWebViewCompositionController = nullptr;
m_coreWebViewController = nullptr;
m_coreWebView = nullptr;
ResetProperties();
}
// Null these out so we can't try to use them anymore
strongThis->m_coreWebViewCompositionController = nullptr;
strongThis->m_coreWebViewController = nullptr;
strongThis->m_coreWebView = nullptr;
strongThis->ResetProperties();
}

FireCoreProcessFailedEvent(args);
strongThis->FireCoreProcessFailedEvent(args);
}
} });

m_coreLostFocusRevoker = m_coreWebViewController.LostFocus(winrt::auto_revoke, {
[this](auto const&, auto const&)
[weakThis{ get_weak() }](auto const& controller, auto const& obj)
{
// Unset our tracking of Edge focus when it is lost via something other than TAB navigation.
m_webHasFocus = false;
if (auto strongThis = weakThis.get())
{
// Unset our tracking of Edge focus when it is lost via something other than TAB navigation.
strongThis->m_webHasFocus = false;
}
} });

m_cursorChangedRevoker = m_coreWebViewCompositionController.CursorChanged(winrt::auto_revoke, {
[this](auto const& controller, auto const& obj)
[weakThis{ get_weak() }](auto const& controller, auto const& obj)
{
UpdateCoreWindowCursor();
if (auto strongThis = weakThis.get())
{
strongThis->UpdateCoreWindowCursor();
}
} });
}

Expand Down Expand Up @@ -1958,4 +1978,4 @@ winrt::UISettings WebView2::GetUISettings()
m_uiSettings = winrt::UISettings();
}
return m_uiSettings;
}
}
5 changes: 0 additions & 5 deletions docs/contribution_workflow.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,6 @@ Pull requests from a fork will not automatically trigger all of these checks. A
team can trigger the Azure Pipeline checks by commenting `/azp run` on the PR. The Azure Pipelines
bot will then trigger the build.

In order to have PRs automatically merge once all checks have passed (including optional
checks), maintainers can apply the [auto merge](https://github.com/Microsoft/microsoft-ui-xaml/labels/auto%20merge)
label. It will take effect after an 8 hour delay,
[more info here (internal link)](https://microsoft.sharepoint.com/teams/FabricBot/SitePages/AutoMerge,-Bot-Templates-and.aspx).

### Other Pipelines

Unlike the above checks these are not required for all PRs, but you may see them on some PRs so we
Expand Down

0 comments on commit 00b8713

Please sign in to comment.