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.

Sign up for GitHub

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

Make CoreWebView2 handlers robust around WebView2 destruction #8969

Merged
merged 6 commits into from
Oct 20, 2023
Merged

Make CoreWebView2 handlers robust around WebView2 destruction #8969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1595a2
Harden all CoreWebView2 handlers with weak refs to WV2 object
Oct 11, 2023
8c74e30
fix strongThis comparison
Oct 12, 2023
227885f
Fix compariosn of strongThis with nextElement (strongThis->try_as<win…
Oct 18, 2023
36e3001
Disable consistently failing RadioButtons.AccessKeys test
Oct 18, 2023
46b1b11
Merge branch 'user/dkomin/disable_radiobuttons_accesskeys_test' into …
kmahone Oct 20, 2023
3c163c0
Update contribution_workflow.md - we no longer use FabricBot
kmahone Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev/RadioButtons/InteractionTests/RadioButtonsTests.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading