Allow widgets to be rearranged by keyboard #3426
Conversation
krschau
requested review from
dhoehna,
guimafelipe,
vineeththomasalex,
dkbennett,
EricJohnson327 and
lauren-ciha
| var index = ViewModel.PinnedWidgets.IndexOf(widgetViewModel); | ||
| _log.Information($"Move widget {widgetViewModel.WidgetDisplayTitle} at index {index} {key}"); | ||
|
|
||
| if (key == VirtualKey.Left && index > 0) |
There was a problem hiding this comment.
I believe this can be reduced to figuring out the new index in the if/else block.
| else if (key == VirtualKey.Right && index < (ViewModel.PinnedWidgets.Count - 1)) | ||
| { | ||
| // Setting focus before and after the move looks more natural than letting the focus move to the wrong element. | ||
| await FocusManager.TryFocusAsync(WidgetGridView.ItemsPanelRoot.Children.ElementAt(index + 1), FocusState.Keyboard); |
There was a problem hiding this comment.
Why the extra focus here, but not in the if case?
There was a problem hiding this comment.
There was nothing I could set focus on in the if case that would make the animation look less awkward. Someday I'd like better animations for both keyboard and the drag and drop scenario, but I'd rather unblock accessibility asap than hold anything up for that.
| @@ -99,7 +101,8 @@ | |||
| <ItemsPanelTemplate> | |||
| <controls:WidgetBoard XYFocusKeyboardNavigation="Enabled" | |||
| XYFocusRightNavigationStrategy="RectilinearDistance" | |||
| XYFocusLeftNavigationStrategy="RectilinearDistance"/> | |||
| XYFocusLeftNavigationStrategy="RectilinearDistance" | |||
| KeyUp="HandleKeyUpAsync" /> | |||
There was a problem hiding this comment.
Would promoting this to a command prevent any unexpected concurrent asynchronous execution behavior?
There was a problem hiding this comment.
Because it's in a ItemsPanelTempate, I can't bind to the command 🙁
microsoft/microsoft-ui-xaml#2508
| { | ||
| _log.Debug($"Key up"); | ||
|
|
||
| await _moveWidgetsLock.WaitAsync(); |
There was a problem hiding this comment.
Oh I see, is this lock to prevent concurrent execution from manipulating shared resources? If yes, putting [RelayCommand] is sufficient as by default AllowConcurrentExecutions is set to false.
References: RelayCommand docs
AmelBawa-msft
added
Needs-Second
Summary of the pull request
Allows widgets to be rearranged via Alt+Shift+LeftArrow and Alt+Shift+RightArrow keyboard commands.
Separately, create widget's ActionParser and ElementParser only once and reuse them, since they never change.
References and relevant issues
https://task.ms/49658818
Detailed description of the pull request / Additional comments
Validation steps performed
Validated locally.
PR checklist