Make command palette ephemeral #8377

Don-Vito · 2020-11-23T18:46:46Z

So the implementation is somewhat dirty.
The ideas was nice - add lostFocusHandler

However it broke few things:

In the TabSwitcher the ListItem must be focusable since otherwise
the SingleSelectionMode behavior breaks.
To address this I had to put the lostFocusHandler on the items as well
When you click the flyout, the palette loses focus,
which makes the terminal page to set the focus on the tab, closing the flyout.
To address this I had to ensure the tab won't get focused once the flyout is open.
In addition, flyout should fix the focus before opening,
otherwise alt+tab will put a focus on a tab row rather than on tab
I also had to close the palette if the tab order changes.
To prevent inconsistencies.

zadjii-msft

I've got a couple questions and spelling nits. I've checked out the branch and played with it and it feels fine to me, so I'm okay with this. I just want to make sure the TODOs shouldn't be "TODONEs"

zadjii-msft · 2020-11-30T18:04:33Z

src/cascadia/TerminalApp/CommandPalette.cpp

+    }
+
+    // Method Description:
+    // - The purpose of this event handler is to hind the palette if it loses focus.


Suggested change

// - The purpose of this event handler is to hind the palette if it loses focus.

// - The purpose of this event handler is to hide the palette if it loses focus.

I guess I was drunk or something 😕

zadjii-msft · 2020-11-30T18:22:47Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        // We cannot do it on closing because if the window loses focus (alt+tab)
+        // the closing event is not fired.
+        // It is important to set the focus on the tab
+        // Sine the previous focus location might be discarded in the background,


Suggested change

// Sine the previous focus location might be discarded in the background,

// Since the previous focus location might be discarded in the background,

zadjii-msft · 2020-11-30T18:28:50Z

src/cascadia/TerminalApp/TerminalPage.cpp

-        // Return focus to the active control
-        if (auto index{ _GetFocusedTabIndex() })
+        // We don't want to set focus on the tab if fly-out is open as it will be closed
+        // TODO: consider checking we are not in the opening state, by hooking both Opening and Open events


Do we want to address this TODO during this review?

@zadjii-msft - don't think so - its highly theoretical.

zadjii-msft · 2020-11-30T18:29:25Z

src/cascadia/TerminalApp/CommandPalette.cpp

+    // Since the focus might move within the palette we need to register this handler for all focusable children
+    // Right now this is achieved by explicitly registering on the root on the highlighted text controls.
+    // Few notes:
+    // - found no way to entires disable focus on list-view elements in single-item-selection mode without breaking list behavior


Suggested change

// - found no way to entires disable focus on list-view elements in single-item-selection mode without breaking list behavior

// - found no way to entirely disable focus on list-view elements in single-item-selection mode without breaking list behavior

(I think?)

zadjii-msft · 2020-11-30T18:30:38Z

src/cascadia/TerminalApp/CommandPalette.cpp

+    // Right now this is achieved by explicitly registering on the root on the highlighted text controls.
+    // Few notes:
+    // - found no way to entires disable focus on list-view elements in single-item-selection mode without breaking list behavior
+    // - TODO: the explicit implementation can be generalized by checking if the focused element is a descendant of palette


Do we want to address this TODO? Are there any drawbacks from the current approach? Like, will he manually have to add this _lostFocusHandler to all the focusable UI elements in the cmdpal?

As you said, the only drawback of the current approach is that we add another field (that might get focus) we need to remember to add it here. On the other hand this solution is simple (funny stuff is that Xaml UI guys have implemented similar function but do not export it - seen it occasionally, trying to understand if we can bind the tabview in any case).

Okay. That's definitely annoying to remember, but hopefully we'll remember to do this.

Okay. That's definitely annoying to remember, but hopefully we'll remember to do this.

IT IS NOW FIXED HA HA HA

Don-Vito · 2020-11-30T20:13:51Z

I've got a couple questions and spelling nits. I've checked out the branch and played with it and it feels fine to me, so I'm okay with this. I just want to make sure the TODOs shouldn't be "TODONEs"

@zadjii-msft - thanks for the review! And sorry for all misspellings (I guess it is an hour of the day.. or of the night.. that I start to err the way that even spellchecker cannot stop me). I am not sure if we need to implement TODOs now.. as both are somewhat hypothetical.. I can absolutely work on them if you find it is a correct timing 😊

Don-Vito · 2020-12-08T13:19:30Z

@zadjii-msft - a week has passed, so bumping 😄

zadjii-msft

I'm okay with this, mostly on the premise that it works well when I played with it. I really don't love having to remember to add that event handler each time manually, but we can live with it. And If we can't live with ti, then we've at least got a plan.

My only real nit is that we don't usually allow bare TODOs in the code, so I'm suggesting that we just link these to the bullet points I added in #5400. They don't really deserve a whole issue, but best to have them enumerated somewhere.

zadjii-msft · 2020-12-08T14:01:57Z

src/cascadia/TerminalApp/CommandPalette.cpp

+    // Right now this is achieved by explicitly registering on the root on the highlighted text controls.
+    // Few notes:
+    // - found no way to entires disable focus on list-view elements in single-item-selection mode without breaking list behavior
+    // - TODO: the explicit implementation can be generalized by checking if the focused element is a descendant of palette


Okay. That's definitely annoying to remember, but hopefully we'll remember to do this.

src/cascadia/TerminalApp/CommandPalette.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft · 2020-12-08T14:07:08Z

(I'm throwing Needs-Second on this, but I'd bet that @DHowett would want to take a look)

DHowett · 2020-12-08T19:48:27Z

src/cascadia/TerminalApp/CommandPalette.cpp

+    void CommandPalette::_lostFocusHandler(Windows::Foundation::IInspectable const& /* sender*/,
+                                           Windows::UI::Xaml::RoutedEventArgs const& /*args*/)
+    {
+        auto focusedElement = Input::FocusManager::GetFocusedElement(this->XamlRoot()).try_as<FrameworkElement>();
+        if (focusedElement && focusedElement != *this && focusedElement.Name() != L"_highlightedTextControl" && Visibility() != Visibility::Collapsed)
+        {
+            _dismissPalette();
+        }


I've seen some strange things with the Focus event that make me feel like this should be easier.

If you add a LostFocus handler on the root control, and check the sender parameter instead of GetFocusedElement, you might find that you're getting LostFocus event fired for all contained controls. (I am not certain about this, but it is a routed event!) That would make this easier: use VisualTreeHelper to walk the control's parent to see if it is us or is within us.

@DHowett - Yeah.. I have a code with VisualTreeHelper iterating the parents. This is how I started, but putting the lost focus on the root of the palette was not sufficient. Are you suggesting to put in on TerminalPage?

Also from the documentation of Lost Focus Event:

Specific Windows Runtime controls may have class-based handling for the LostFocus event. If so, the control probably has an override for the method OnLostFocus. Typically the event is marked handled by the class handler, and the LostFocus event is not raised for handling by any user code handlers on that control. Controls might handle the event in order to unload a visual state that displayed a focus rectangle in response to OnGotFocus.

I can check if we can do something with LosingFocus event that won't probably get swallowed the way LostFocus does

DHowett

Blocking to have out the discussion about LostFocus -- not totally sure -- otherwise I like this! 😄

DHowett · 2020-12-08T21:01:30Z

Ah. Don't do any work to investigate that -- I don't want you to waste your effort. After we get through this spec review meeting I'll come back to tap this PR. 😄

Don-Vito · 2020-12-08T21:03:58Z

Ah. Don't do any work to investigate that -- I don't want you to waste your effort. After we get through this spec review meeting I'll come back to tap this PR. 😄

Too late - already playing with this 😄

Don-Vito · 2020-12-08T21:17:38Z

So.. LosingFocus seems to catch everything:

void CommandPalette::_losingFocusHandler(Windows::Foundation::IInspectable const& /*sender*/,
                                           Windows::UI::Xaml::Input::LosingFocusEventArgs const& args)
    {
        auto element = args.NewFocusedElement();
        while (element)
        {
            if (element == *this)
            {
                return;
            }

            element = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(element);
        }

        _dismissPalette();
    }

The problem I believe is that _dismissPalette, changes the visibility of the palette, that invokes a property changed handler in terminal page, that sets focus, that causes corruption 😄

Don-Vito · 2020-12-08T21:33:08Z

So it seems that lostFocusHandler does capture all events (I cannot reproduce why it didn't work for me earlier), but sender is not good enough, it is always ourselves. So i am submitting something in between:

void CommandPalette::_lostFocusHandler(Windows::Foundation::IInspectable const& /*sender*/,
                                           Windows::UI::Xaml::RoutedEventArgs const& /*args*/)
    {        
        auto focusedElement = Input::FocusManager::GetFocusedElement(this->XamlRoot()).try_as<DependencyObject>();        
        while (focusedElement)
        {
            if (focusedElement == *this)
            {
                return;
            }

            focusedElement = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(focusedElement);
        }

        _dismissPalette();
    }

…terminal into 8355-palette-ephemeral

…nymore

DHowett · 2020-12-08T22:40:09Z

src/cascadia/TerminalApp/CommandPalette.cpp

+        while (focusedElement)
+        {
+            if (focusedElement == *this)
+            {
+                return;
+            }
+
+            focusedElement = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(focusedElement);


k so just so I understand: If the thing losing focus is inside us or is us, we will return here and keep the palette open.

I don't totally understand why this works as described 😄 because if we aren't reacting to the command palette itself or any of its descendants losing focus and dismissing, when ARE we dismissing?

@DHowett - I mean you were right, the lost focus event is getting routed to us if we or one of our children loses focus. We only need to distinguish if it loses the focus in favor of some other our child or some external element.

We cannot achieve this with the sender, because it is always the element that loses focus, while we are interested in element that gets focus.

I guess I will issue another commit explaining this 😊

DHowett

Oh, of course, it makes sense. Not sender. "Is the thing that now has focus inside us".

Thanks!

ghost · 2020-12-09T22:00:37Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-01-28T18:03:40Z

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

So the implementation is somewhat dirty. The ideas was nice - add lostFocusHandler However it broke few things: * In the TabSwitcher the ListItem must be focusable since otherwise the SingleSelectionMode behavior breaks. To address this I had to put the lostFocusHandler on the items as well * When you click the flyout, the palette loses focus, which makes the terminal page to set the focus on the tab, closing the flyout. To address this I had to ensure the tab won't get focused once the flyout is open. In addition, flyout should fix the focus before opening, otherwise alt+tab will put a focus on a tab row rather than on tab * I also had to close the palette if the tab order changes. To prevent inconsistencies. Closes microsoft#8355

Fixes a regression of command palette accessibility. The regression was introduced in #8377 by setting `IsTabStop` to false. Though the commands would light up, the focus didn't technically get on the command, so the screen reader would just read the text box. ## Validation Steps Performed Opened the command palette while NVDA is active. It now reads the commands as focus moves on them.

Fixes a regression of command palette accessibility. The regression was introduced in #8377 by setting `IsTabStop` to false. Though the commands would light up, the focus didn't technically get on the command, so the screen reader would just read the text box. Opened the command palette while NVDA is active. It now reads the commands as focus moves on them. (cherry picked from commit ac3e4bf)

Don-Vito added 3 commits November 21, 2020 23:44

Close command palette if tab items are modified

1a8f7a6

Prevent command palette closing from affecting flyout correctness

e40457b

Dismiss palette upon focus loss

917110a

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Nov 23, 2020

Don-Vito marked this pull request as draft November 23, 2020 18:47

Don-Vito marked this pull request as ready for review November 29, 2020 13:36

zadjii-msft self-assigned this Nov 30, 2020

zadjii-msft reviewed Nov 30, 2020

View reviewed changes

zadjii-msft assigned Don-Vito and unassigned zadjii-msft Nov 30, 2020

zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 30, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 30, 2020

Fix spelling

f2d9b04

Don-Vito requested a review from zadjii-msft December 1, 2020 22:38

zadjii-msft self-assigned this Dec 1, 2020

zadjii-msft approved these changes Dec 8, 2020

View reviewed changes

Apply suggestions from code review

9eb2698

zadjii-msft unassigned Don-Vito and zadjii-msft Dec 8, 2020

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 8, 2020

ghost requested review from miniksa, carlos-zamora, DHowett, leonMSFT and PankajBhojwani December 8, 2020 14:06

ghost added the Area-CmdPal Command Palette issues and features label Dec 8, 2020

DHowett reviewed Dec 8, 2020

View reviewed changes

DHowett requested changes Dec 8, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 8, 2020

Don-Vito added 3 commits December 8, 2020 23:37

Modify LostFocusHandler of CmdPal to iterate via visual hieararchy

7fc4fa5

Merge branch '8355-palette-ephemeral' of https://github.com/Don-Vito/…

5259aed

…terminal into 8355-palette-ephemeral

Remove the name of the highlighted text control as we don't need it a…

bde5de1

…nymore

DHowett reviewed Dec 9, 2020

View reviewed changes

Document lost focus handler logic

7c82e1b

Don-Vito requested a review from DHowett December 9, 2020 08:50

Don-Vito added 2 commits December 9, 2020 10:53

Fix a comment typo

09f27a6

Fix another documentation typo

76dde26

DHowett approved these changes Dec 9, 2020

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 9, 2020

ghost merged commit e1a8657 into microsoft:main Dec 9, 2020

ghost mentioned this pull request Jan 28, 2021

Close command palette on focus loss #8355

Closed

carlos-zamora mentioned this pull request Feb 12, 2021

Fix command palette accessibility #9143

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make command palette ephemeral #8377

Make command palette ephemeral #8377

Don-Vito commented Nov 23, 2020 •

edited by DHowett

Loading

zadjii-msft left a comment

zadjii-msft Nov 30, 2020

Don-Vito Nov 30, 2020

zadjii-msft Nov 30, 2020

zadjii-msft Nov 30, 2020

Don-Vito Nov 30, 2020

zadjii-msft Nov 30, 2020

zadjii-msft Nov 30, 2020

Don-Vito Nov 30, 2020 •

edited

Loading

zadjii-msft Dec 8, 2020

DHowett Dec 8, 2020

Don-Vito commented Nov 30, 2020

Don-Vito commented Dec 8, 2020

zadjii-msft left a comment

zadjii-msft Dec 8, 2020

zadjii-msft commented Dec 8, 2020

DHowett Dec 8, 2020

Don-Vito Dec 8, 2020

Don-Vito Dec 8, 2020 •

edited

Loading

Don-Vito Dec 8, 2020

DHowett left a comment

DHowett commented Dec 8, 2020

Don-Vito commented Dec 8, 2020

Don-Vito commented Dec 8, 2020

Don-Vito commented Dec 8, 2020

DHowett Dec 8, 2020

Don-Vito Dec 9, 2020 •

edited

Loading

Don-Vito Dec 9, 2020

DHowett left a comment

ghost commented Dec 9, 2020

ghost commented Jan 28, 2021

	// - The purpose of this event handler is to hind the palette if it loses focus.
	// - The purpose of this event handler is to hide the palette if it loses focus.

	// Sine the previous focus location might be discarded in the background,
	// Since the previous focus location might be discarded in the background,

	// - found no way to entires disable focus on list-view elements in single-item-selection mode without breaking list behavior
	// - found no way to entirely disable focus on list-view elements in single-item-selection mode without breaking list behavior

Make command palette ephemeral #8377

Make command palette ephemeral #8377

Conversation

Don-Vito commented Nov 23, 2020 • edited by DHowett Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito commented Nov 30, 2020

Don-Vito commented Dec 8, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Dec 8, 2020

Don-Vito commented Dec 8, 2020

Don-Vito commented Dec 8, 2020

Don-Vito commented Dec 8, 2020

Choose a reason for hiding this comment

Don-Vito Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Dec 9, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jan 28, 2021

Don-Vito commented Nov 23, 2020 •

edited by DHowett

Loading

Don-Vito Nov 30, 2020 •

edited

Loading

Don-Vito Dec 8, 2020 •

edited

Loading

Don-Vito Dec 9, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.