Skip to content

Code Quality: Fixed a number of focus related issues in Omnibar #17239

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.

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

Merged
merged 11 commits into from
Jul 3, 2025
Merged

Conversation

yaira2
Copy link
Member

@yaira2 yaira2 commented Jul 1, 2025

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

  • For Feature: Omnibar #16029
  • Fixed "Entering text in Command mode, switching to Edit path mode, and then back to Command mode doesn't clear the original text"
  • Fixed "Pressing tab in Command mode closes flyout but doesn't switch to the next control"
  • Fixed "Switching to Path mode from Command mode isn't selecting the text"
  • Fixed "When breadcrumb flyout is open, clicking on Command mode switches to path mode"
  • Fixed "Pressing enter key while focused on a mode button isn't switching modes"

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

Fixed Command Palette text when switching between modes

  1. Entered text in Command mode
  2. Switched to Edit path mode
  3. Switched back to Command mode
  4. Confirmed the text was correctly cleared

Moved the logic for resetting the selected mode on LostFocus from the app into the control

  1. Switched to Edit path mode
  2. Removed focus by clicking the file area
  3. Confirmed the Omnibar switched to Breadcrumb mode
  4. Switched to Command mode and repeated steps 2 and 3

Pressing tab in Command mode closes flyout but doesn't switch to the next control

  1. Focused on path box
  2. Pressed tab
  3. Confirmed the breadcrumb bar is selected
  4. Switched to Command mode
  5. Confirmed pressing tab selects the mode button

Switching to Path mode from Command mode isn't selecting the text

  1. Switched to Command mode
  2. Switched to Edit path mode
  3. Confirmed the text was selected

When breadcrumb flyout is open, clicking on Command mode switches to path mode

  1. Expanded breadcrumb flyout
  2. Switched to Command mode
  3. Confirmed the mode properly switches

@yaira2 yaira2 requested a review from Copilot July 2, 2025 23:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the Omnibar control’s focus and mode-switch behavior to address several focus-related bugs and ensure modes and text states reset correctly.

  • Introduces a ModeChanged event handler to clear command palette text when switching modes.
  • Moves lost-focus logic into the Omnibar control to reset to the default mode.
  • Adds Tab key handling in PreviewKeyDown to close flyouts and move focus to the next control.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/Files.App/UserControls/NavigationToolbar.xaml.cs Replaced LostFocus handler with ModeChanged, clear command text on mode change, and handle Tab key focus
src/Files.App/UserControls/NavigationToolbar.xaml Updated XAML to use ModeChanged instead of LostFocus
src/Files.App.Controls/Omnibar/Omnibar.Events.cs Moved lost-focus logic into the control, reset default mode on LostFocus, and auto-select text on changes

@yaira2 yaira2 merged commit 622b705 into main Jul 3, 2025
8 of 9 checks passed
@yaira2 yaira2 deleted the ya/Omnibar branch July 3, 2025 02:02
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant