From 0b6e07db1eb13c43b2bd453018a48410bae576c4 Mon Sep 17 00:00:00 2001 From: Kristen Schau <47155823+krschau@users.noreply.github.com> Date: Thu, 26 May 2022 08:11:32 -0700 Subject: [PATCH] Revoke HandleAcceleratorKeyActivated when a WebView2 is closed (#7117) * Move code around to match up registering and revoking * Revoke m_acceleratorKeyActivatedRevoker * let xaml handle tab if corewebview2 is closed * Add test * update Microsoft.DotNet.Helix.Sdk Co-authored-by: Kristen Schau --- build/Helix/global.json | 2 +- .../InteractionTests/WebView2Tests.cs | 55 ++++++++++ dev/WebView2/TestUI/WebView2BasicPage.xaml | 1 + dev/WebView2/TestUI/WebView2BasicPage.xaml.cs | 9 +- dev/WebView2/WebView2.cpp | 100 ++++++++++-------- 5 files changed, 119 insertions(+), 48 deletions(-) diff --git a/build/Helix/global.json b/build/Helix/global.json index afa5e25ad1..a31d6c281f 100644 --- a/build/Helix/global.json +++ b/build/Helix/global.json @@ -3,6 +3,6 @@ "version": "3.1" }, "msbuild-sdks": { - "Microsoft.DotNet.Helix.Sdk": "6.0.0-beta.20529.1" + "Microsoft.DotNet.Helix.Sdk": "7.0.0-beta.22275.2" } } \ No newline at end of file diff --git a/dev/WebView2/InteractionTests/WebView2Tests.cs b/dev/WebView2/InteractionTests/WebView2Tests.cs index e4944ade48..0fe9f5ee54 100644 --- a/dev/WebView2/InteractionTests/WebView2Tests.cs +++ b/dev/WebView2/InteractionTests/WebView2Tests.cs @@ -402,6 +402,12 @@ private static void WaitForLoadCompleted() WaitForTextBoxValue(status2, string.Empty, false /* match = false for not match */); } + private static void WaitForEnsureCompleted() + { + var status1 = new Edit(FindElement.ById("Status1")); + WaitForTextBoxValue(status1, "EnsureCoreWebView2Async() completed", true); + } + private static void ChooseTest(string testName, bool waitForLoadCompleted = true) { WaitForCopyCompleted(); @@ -2202,6 +2208,55 @@ public void ParentHiddenThenVisibleTest() } } + [TestMethod] + [TestProperty("TestSuite", "A")] + public void LifetimeTabTest() + { + using (var setup = new WebView2TestSetupHelper(new[] { "WebView2 Tests", "navigateToBasicWebView2" })) + { + // Part 1: Tab with no core webview + + Log.Comment("Part 1: Tab with no core webview"); + TabTwicePastWebView2(); + + // Part 2: Tab with core webview, ensured but not navigated + + Log.Comment("Part 2: Tab with core webview, not navigated"); + Button ensureButton = new Button(FindElement.ById("EnsureCWV2Button")); + ensureButton.Invoke(); + WaitForEnsureCompleted(); + TabTwicePastWebView2(); + + // Part 3: Tab with closed core webview + + Log.Comment("Part 3: Tab with closed core webview"); + ChooseTest("LifetimeTabTest" /* waitForLoadCompleted */); + CompleteTestAndWaitForResult("LifetimeTabTest"); // Closes the CoreWebView2 + TabTwicePastWebView2(); + } + } + + private static void TabTwicePastWebView2() + { + Button x1 = new Button(FindElement.ById("TabStopButton1")); // Xaml TabStop 1 + Button x2 = new Button(FindElement.ById("TabStopButton2")); // Xaml TabStop 2 + + Log.Comment("Focus on x1"); + x1.SetFocus(); + Wait.ForIdle(); + Verify.IsTrue(x1.HasKeyboardFocus, "TabStopButton1 has keyboard focus"); + + Log.Comment("Tab x1 -> webview -> x2"); + using (var xamlFocusWaiter = new FocusAcquiredWaiter("TabStopButton2")) + { + KeyboardHelper.PressKey(Key.Tab); + KeyboardHelper.PressKey(Key.Tab); + xamlFocusWaiter.Wait(); + Log.Comment("Focus is on " + UIObject.Focused); + Verify.IsTrue(x2.HasKeyboardFocus, "TabStopButton2 has keyboard focus"); + } + } + private static void BeginSubTest(string testName, string testDescription) { Log.Comment(Environment.NewLine + testName + ": " + testDescription); diff --git a/dev/WebView2/TestUI/WebView2BasicPage.xaml b/dev/WebView2/TestUI/WebView2BasicPage.xaml index b2e7fc2cd7..e299507a5c 100644 --- a/dev/WebView2/TestUI/WebView2BasicPage.xaml +++ b/dev/WebView2/TestUI/WebView2BasicPage.xaml @@ -45,6 +45,7 @@ HiddenThenVisibleTest HostNameToFolderMappingTest HtmlDropdownTest + LifetimeTabTest MouseCaptureTest MouseLeftClickTest MouseMiddleClickTest diff --git a/dev/WebView2/TestUI/WebView2BasicPage.xaml.cs b/dev/WebView2/TestUI/WebView2BasicPage.xaml.cs index c84cd9f8d1..4c22563980 100644 --- a/dev/WebView2/TestUI/WebView2BasicPage.xaml.cs +++ b/dev/WebView2/TestUI/WebView2BasicPage.xaml.cs @@ -198,7 +198,8 @@ enum TestList OffTreeWebViewInputTest, HtmlDropdownTest, HiddenThenVisibleTest, - ParentHiddenThenVisibleTest + ParentHiddenThenVisibleTest, + LifetimeTabTest, }; // Map of TestList entry to its webpage (index in TestPageNames[]) @@ -264,6 +265,7 @@ enum TestList { TestList.HtmlDropdownTest, 5 }, { TestList.HiddenThenVisibleTest, 1 }, { TestList.ParentHiddenThenVisibleTest, 1 }, + { TestList.LifetimeTabTest, 0 }, }; readonly string[] TestPageNames = @@ -1905,6 +1907,11 @@ async public void CompleteCurrentTest(object sender, RoutedEventArgs args) selectedTest, MyWebView2.IsHitTestVisible)); } break; + case TestList.LifetimeTabTest: + { + MyWebView2.Close(); + } + break; default: break; diff --git a/dev/WebView2/WebView2.cpp b/dev/WebView2/WebView2.cpp index ab65c1ac59..ce9b9b0656 100644 --- a/dev/WebView2/WebView2.cpp +++ b/dev/WebView2/WebView2.cpp @@ -74,29 +74,6 @@ void WebView2::OnVisibilityPropertyChanged(const winrt::DependencyObject& /*send UpdateRenderedSubscriptionAndVisibility(); } -void WebView2::UnregisterCoreEventHandlers() -{ - if (m_coreWebView) - { - m_coreNavigationStartingRevoker.revoke(); - m_coreSourceChangedRevoker.revoke(); - m_coreNavigationCompletedRevoker.revoke(); - m_coreWebMessageReceivedRevoker.revoke(); - m_coreProcessFailedRevoker.revoke(); - } - - if (m_coreWebViewController) - { - m_coreMoveFocusRequestedRevoker.revoke(); - m_coreLostFocusRevoker.revoke(); - } - - if (m_coreWebViewCompositionController) - { - m_cursorChangedRevoker.revoke(); - } -} - // Public Close() API void WebView2::Close() { @@ -124,6 +101,13 @@ void WebView2::CloseInternal(bool inShutdownPath) m_visibilityChangedToken.value = 0; } + // TODO: We do not have direct analogue for AcceleratorKeyActivated with DispatcherQueue in Islands/ win32. Please refer Task# 30013704 for more details. + if (auto coreWindow = winrt::CoreWindow::GetForCurrentThread()) + { + m_inputWindowHwnd = nullptr; + m_acceleratorKeyActivatedRevoker.revoke(); + } + if (m_coreWebView) { m_coreWebView = nullptr; @@ -557,6 +541,28 @@ void WebView2::RegisterCoreEventHandlers() FireWebMessageReceived(args); }}); + m_coreProcessFailedRevoker = m_coreWebView.ProcessFailed(winrt::auto_revoke, { + [this](auto const&, winrt::CoreWebView2ProcessFailedEventArgs const& args) + { + winrt::CoreWebView2ProcessFailedKind coreProcessFailedKind{ args.ProcessFailedKind() }; + if (coreProcessFailedKind == winrt::CoreWebView2ProcessFailedKind::BrowserProcessExited) + { + 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(); + + // Null these out so we can't try to use them anymore + m_coreWebViewCompositionController = nullptr; + m_coreWebViewController = nullptr; + m_coreWebView = nullptr; + ResetProperties(); + } + + FireCoreProcessFailedEvent(args); + } }); + m_coreMoveFocusRequestedRevoker = m_coreWebViewController.MoveFocusRequested(winrt::auto_revoke, { [this](auto const&, const winrt::CoreWebView2MoveFocusRequestedEventArgs& args) { @@ -635,28 +641,6 @@ void WebView2::RegisterCoreEventHandlers() m_webHasFocus = false; }}); - m_coreProcessFailedRevoker = m_coreWebView.ProcessFailed(winrt::auto_revoke, { - [this](auto const&, winrt::CoreWebView2ProcessFailedEventArgs const& args) - { - winrt::CoreWebView2ProcessFailedKind coreProcessFailedKind{ args.ProcessFailedKind() }; - if (coreProcessFailedKind == winrt::CoreWebView2ProcessFailedKind::BrowserProcessExited) - { - 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(); - - // Null these out so we can't try to use them anymore - m_coreWebViewCompositionController = nullptr; - m_coreWebViewController = nullptr; - m_coreWebView = nullptr; - ResetProperties(); - } - - FireCoreProcessFailedEvent(args); - }}); - m_cursorChangedRevoker = m_coreWebViewCompositionController.CursorChanged(winrt::auto_revoke, { [this](auto const& controller, auto const& obj) { @@ -666,6 +650,29 @@ void WebView2::RegisterCoreEventHandlers() }}); } +void WebView2::UnregisterCoreEventHandlers() +{ + if (m_coreWebView) + { + m_coreNavigationStartingRevoker.revoke(); + m_coreSourceChangedRevoker.revoke(); + m_coreNavigationCompletedRevoker.revoke(); + m_coreWebMessageReceivedRevoker.revoke(); + m_coreProcessFailedRevoker.revoke(); + } + + if (m_coreWebViewController) + { + m_coreMoveFocusRequestedRevoker.revoke(); + m_coreLostFocusRevoker.revoke(); + } + + if (m_coreWebViewCompositionController) + { + m_cursorChangedRevoker.revoke(); + } +} + winrt::IAsyncAction WebView2::CreateCoreObjects() { MUX_ASSERT((m_isImplicitCreationInProgress && !m_isExplicitCreationInProgress) || @@ -1327,9 +1334,10 @@ void WebView2::MoveFocusIntoCoreWebView(winrt::CoreWebView2MoveFocusReason reaso // Xaml control and force HWND focus back to itself, popping Xaml focus out of the // WebView2 control. We mark TAB handled in our KeyDown handler so that it is ignored // by XamlRoot's tab processing. +// If the WebView2 has been closed, then we should let Xaml's tab processing handle it. void WebView2::HandleKeyDown(const winrt::Windows::Foundation::IInspectable&, const winrt::KeyRoutedEventArgs& e) { - if (e.Key() == winrt::VirtualKey::Tab) + if (e.Key() == winrt::VirtualKey::Tab && !m_isClosed) { e.Handled(true); }