mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-04-08 20:27:36 +02:00
CmdpPal: SearchBox visibility and async loading race (#42783)
## Summary of the Pull Request This PR introduces two related fixes to improve the stability and reliability of navigation and search UI behavior in the shell: - **Ensure search box visibility is correctly updated** - `ShellViewModel` previously set `IsSearchBoxVisible` after navigation to the page, but didn’t update it when the value changed. While the value isn’t expected to change dynamically, the property initialization is asynchronous, which could cause a race condition. - As a defensive measure, this also changes the default value of uninitialized property to make it visible by default. - **Cancel asynchronous focus placement if navigation changes** - Ensures that any pending asynchronous focus operation is cancelled when another navigation occurs before it completes. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] Closes: #42782 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed
This commit is contained in:
@@ -68,7 +68,7 @@ public partial class PageViewModel : ExtensionObjectViewModel, IPageContext
|
|||||||
// `IsLoading` property as a combo of this value and `IsInitialized`
|
// `IsLoading` property as a combo of this value and `IsInitialized`
|
||||||
public bool ModelIsLoading { get; protected set; } = true;
|
public bool ModelIsLoading { get; protected set; } = true;
|
||||||
|
|
||||||
public bool HasSearchBox { get; protected set; }
|
public bool HasSearchBox { get; protected set; } = true;
|
||||||
|
|
||||||
public IconInfoViewModel Icon { get; protected set; }
|
public IconInfoViewModel Icon { get; protected set; }
|
||||||
|
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
// The Microsoft Corporation licenses this file to you under the MIT license.
|
// The Microsoft Corporation licenses this file to you under the MIT license.
|
||||||
// See the LICENSE file in the project root for more information.
|
// See the LICENSE file in the project root for more information.
|
||||||
|
|
||||||
|
using System.ComponentModel;
|
||||||
using CommunityToolkit.Mvvm.ComponentModel;
|
using CommunityToolkit.Mvvm.ComponentModel;
|
||||||
using CommunityToolkit.Mvvm.Input;
|
using CommunityToolkit.Mvvm.Input;
|
||||||
using CommunityToolkit.Mvvm.Messaging;
|
using CommunityToolkit.Mvvm.Messaging;
|
||||||
@@ -48,6 +49,9 @@ public partial class ShellViewModel : ObservableObject,
|
|||||||
var oldValue = _currentPage;
|
var oldValue = _currentPage;
|
||||||
if (SetProperty(ref _currentPage, value))
|
if (SetProperty(ref _currentPage, value))
|
||||||
{
|
{
|
||||||
|
oldValue.PropertyChanged -= CurrentPage_PropertyChanged;
|
||||||
|
value.PropertyChanged += CurrentPage_PropertyChanged;
|
||||||
|
|
||||||
if (oldValue is IDisposable disposable)
|
if (oldValue is IDisposable disposable)
|
||||||
{
|
{
|
||||||
try
|
try
|
||||||
@@ -63,6 +67,14 @@ public partial class ShellViewModel : ObservableObject,
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private void CurrentPage_PropertyChanged(object? sender, PropertyChangedEventArgs e)
|
||||||
|
{
|
||||||
|
if (e.PropertyName == nameof(PageViewModel.HasSearchBox))
|
||||||
|
{
|
||||||
|
IsSearchBoxVisible = CurrentPage.HasSearchBox;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private IPage? _rootPage;
|
private IPage? _rootPage;
|
||||||
|
|
||||||
private bool _isNested;
|
private bool _isNested;
|
||||||
|
|||||||
@@ -48,7 +48,8 @@ public sealed partial class ShellPage : Microsoft.UI.Xaml.Controls.Page,
|
|||||||
IRecipient<ShowConfirmationMessage>,
|
IRecipient<ShowConfirmationMessage>,
|
||||||
IRecipient<ShowToastMessage>,
|
IRecipient<ShowToastMessage>,
|
||||||
IRecipient<NavigateToPageMessage>,
|
IRecipient<NavigateToPageMessage>,
|
||||||
INotifyPropertyChanged
|
INotifyPropertyChanged,
|
||||||
|
IDisposable
|
||||||
{
|
{
|
||||||
private readonly DispatcherQueue _queue = DispatcherQueue.GetForCurrentThread();
|
private readonly DispatcherQueue _queue = DispatcherQueue.GetForCurrentThread();
|
||||||
|
|
||||||
@@ -65,6 +66,9 @@ public sealed partial class ShellPage : Microsoft.UI.Xaml.Controls.Page,
|
|||||||
|
|
||||||
private SettingsWindow? _settingsWindow;
|
private SettingsWindow? _settingsWindow;
|
||||||
|
|
||||||
|
private CancellationTokenSource? _focusAfterLoadedCts;
|
||||||
|
private WeakReference<Page>? _lastNavigatedPageRef;
|
||||||
|
|
||||||
public ShellViewModel ViewModel { get; private set; } = App.Current.Services.GetService<ShellViewModel>()!;
|
public ShellViewModel ViewModel { get; private set; } = App.Current.Services.GetService<ShellViewModel>()!;
|
||||||
|
|
||||||
public event PropertyChangedEventHandler? PropertyChanged;
|
public event PropertyChangedEventHandler? PropertyChanged;
|
||||||
@@ -488,6 +492,7 @@ public sealed partial class ShellPage : Microsoft.UI.Xaml.Controls.Page,
|
|||||||
|
|
||||||
if (e.Content is Page element)
|
if (e.Content is Page element)
|
||||||
{
|
{
|
||||||
|
_lastNavigatedPageRef = new WeakReference<Page>(element);
|
||||||
element.Loaded += FocusAfterLoaded;
|
element.Loaded += FocusAfterLoaded;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -497,6 +502,18 @@ public sealed partial class ShellPage : Microsoft.UI.Xaml.Controls.Page,
|
|||||||
var page = (Page)sender;
|
var page = (Page)sender;
|
||||||
page.Loaded -= FocusAfterLoaded;
|
page.Loaded -= FocusAfterLoaded;
|
||||||
|
|
||||||
|
// Only handle focus for the latest navigated page
|
||||||
|
if (_lastNavigatedPageRef is null || !_lastNavigatedPageRef.TryGetTarget(out var last) || !ReferenceEquals(page, last))
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Cancel any previous pending focus work
|
||||||
|
_focusAfterLoadedCts?.Cancel();
|
||||||
|
_focusAfterLoadedCts?.Dispose();
|
||||||
|
_focusAfterLoadedCts = new CancellationTokenSource();
|
||||||
|
var token = _focusAfterLoadedCts.Token;
|
||||||
|
|
||||||
AnnounceNavigationToPage(page);
|
AnnounceNavigationToPage(page);
|
||||||
|
|
||||||
var shouldSearchBoxBeVisible = ViewModel.CurrentPage?.HasSearchBox ?? false;
|
var shouldSearchBoxBeVisible = ViewModel.CurrentPage?.HasSearchBox ?? false;
|
||||||
@@ -509,34 +526,57 @@ public sealed partial class ShellPage : Microsoft.UI.Xaml.Controls.Page,
|
|||||||
}
|
}
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
_ = Task.Run(async () =>
|
_ = Task.Run(
|
||||||
{
|
async () =>
|
||||||
await page.DispatcherQueue.EnqueueAsync(async () =>
|
|
||||||
{
|
{
|
||||||
// I hate this so much, but it can take a while for the page to be ready to accept focus;
|
if (token.IsCancellationRequested)
|
||||||
// focusing page with MarkdownTextBlock takes up to 5 attempts (* 100ms delay between attempts)
|
|
||||||
for (var i = 0; i < 10; i++)
|
|
||||||
{
|
{
|
||||||
if (FocusManager.FindFirstFocusableElement(page) is FrameworkElement frameworkElement)
|
return;
|
||||||
{
|
|
||||||
var set = frameworkElement.Focus(FocusState.Programmatic);
|
|
||||||
if (set)
|
|
||||||
{
|
|
||||||
break;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
await Task.Delay(100);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Update the search box visibility based on the current page:
|
try
|
||||||
// - We do this here after navigation so the focus is not jumping around too much,
|
{
|
||||||
// it messes with screen readers if we do it too early
|
await page.DispatcherQueue.EnqueueAsync(
|
||||||
// - Since this should hide the search box on content pages, it's not a problem if we
|
async () =>
|
||||||
// wait for the code above to finish trying to focus the content
|
{
|
||||||
ViewModel.IsSearchBoxVisible = ViewModel.CurrentPage?.HasSearchBox ?? false;
|
// I hate this so much, but it can take a while for the page to be ready to accept focus;
|
||||||
});
|
// focusing page with MarkdownTextBlock takes up to 5 attempts (* 100ms delay between attempts)
|
||||||
});
|
for (var i = 0; i < 10; i++)
|
||||||
|
{
|
||||||
|
token.ThrowIfCancellationRequested();
|
||||||
|
|
||||||
|
if (FocusManager.FindFirstFocusableElement(page) is FrameworkElement frameworkElement)
|
||||||
|
{
|
||||||
|
var set = frameworkElement.Focus(FocusState.Programmatic);
|
||||||
|
if (set)
|
||||||
|
{
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
await Task.Delay(100, token);
|
||||||
|
}
|
||||||
|
|
||||||
|
token.ThrowIfCancellationRequested();
|
||||||
|
|
||||||
|
// Update the search box visibility based on the current page:
|
||||||
|
// - We do this here after navigation so the focus is not jumping around too much,
|
||||||
|
// it messes with screen readers if we do it too early
|
||||||
|
// - Since this should hide the search box on content pages, it's not a problem if we
|
||||||
|
// wait for the code above to finish trying to focus the content
|
||||||
|
ViewModel.IsSearchBoxVisible = ViewModel.CurrentPage?.HasSearchBox ?? false;
|
||||||
|
});
|
||||||
|
}
|
||||||
|
catch (OperationCanceledException)
|
||||||
|
{
|
||||||
|
// Swallow cancellation - another FocusAfterLoaded invocation superseded this one
|
||||||
|
}
|
||||||
|
catch (Exception ex)
|
||||||
|
{
|
||||||
|
Logger.LogError("Error during FocusAfterLoaded async focus work", ex);
|
||||||
|
}
|
||||||
|
},
|
||||||
|
token);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -665,4 +705,11 @@ public sealed partial class ShellPage : Microsoft.UI.Xaml.Controls.Page,
|
|||||||
Logger.LogError("Error handling mouse button press event", ex);
|
Logger.LogError("Error handling mouse button press event", ex);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public void Dispose()
|
||||||
|
{
|
||||||
|
_focusAfterLoadedCts?.Cancel();
|
||||||
|
_focusAfterLoadedCts?.Dispose();
|
||||||
|
_focusAfterLoadedCts = null;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user