From 00b8713881c2987e34043dd8725903e6648313a5 Mon Sep 17 00:00:00 2001 From: Dmitriy Komin <45051803+DmitriyKomin@users.noreply.github.com> Date: Fri, 20 Oct 2023 14:40:59 -0700 Subject: [PATCH] Make CoreWebView2 handlers robust around WebView2 destruction (#8969) * Harden all CoreWebView2 handlers with weak refs to WV2 object * fix strongThis comparison * Fix compariosn of strongThis with nextElement (strongThis->try_as always gives null) * Disable consistently failing RadioButtons.AccessKeys test * Update contribution_workflow.md - we no longer use FabricBot --------- Co-authored-by: Dmitriy Komin Co-authored-by: Keith Mahoney --- .../InteractionTests/RadioButtonsTests.cs | 1 + dev/WebView2/WebView2.cpp | 192 ++++++++++-------- docs/contribution_workflow.md | 5 - 3 files changed, 107 insertions(+), 91 deletions(-) diff --git a/dev/RadioButtons/InteractionTests/RadioButtonsTests.cs b/dev/RadioButtons/InteractionTests/RadioButtonsTests.cs index b38827f049..094bf9eec9 100644 --- a/dev/RadioButtons/InteractionTests/RadioButtonsTests.cs +++ b/dev/RadioButtons/InteractionTests/RadioButtonsTests.cs @@ -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)) diff --git a/dev/WebView2/WebView2.cpp b/dev/WebView2/WebView2.cpp index 3f422d3b35..aa3fdf2142 100644 --- a/dev/WebView2/WebView2.cpp +++ b/dev/WebView2/WebView2.cpp @@ -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::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()) { - 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() == *(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() == *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(); + } } }); } @@ -1958,4 +1978,4 @@ winrt::UISettings WebView2::GetUISettings() m_uiSettings = winrt::UISettings(); } return m_uiSettings; -} \ No newline at end of file +} diff --git a/docs/contribution_workflow.md b/docs/contribution_workflow.md index 7cd035e817..9e509d6e30 100644 --- a/docs/contribution_workflow.md +++ b/docs/contribution_workflow.md @@ -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