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

Jump to bottom

Close command palette on focus loss #8355

Closed
Don-Vito opened this issue Nov 21, 2020 · 7 comments · Fixed by #8377
Closed

Close command palette on focus loss #8355

Don-Vito opened this issue Nov 21, 2020 · 7 comments · Fixed by #8377
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.6

Comments

@Don-Vito
Copy link
Contributor

Don-Vito commented Nov 21, 2020 •
edited
Loading

Description of the new feature/enhancement

Currently we dismiss command palette programmatically if user clicks in the bounds of "Tab Content" (second grid row of the terminal page), however if the click is performed outside of these boundaries (e.g., outside of the terminal window or in the tab row) we do not dismiss the palette.

And this is problematic for many reasons.

Here is the simplest example - opening a tab while palette is open leaves it open an without a focus (meaning it can be closed only with mouse):

PaletteLostFocusOpenTab

Same can be achieved by clicking on menu, and then click outside of it (but now you cannot even type)

PaletteLostFocusOpenMenu

Same of course happens for the tab switcher
PaletteLostFocus

This might might be a root cause for #8319.

The idea here is to close the palette upon focus loss.
There are two approaches to it:

  1. Close the palette upon focus loss - this approach is taken in VS Code.
  • This approach is more hermetic
  • This approach introduces a more significant change in user experience, since now clicking another window closes the palette
  1. Close palette when clicking on the tab row (or more precisely on any row of the terminal page which is not the one that palette resides in)
  • This will have lesser impact on the experience
  • But it will not help us if the focus is somehow lost no by clicking
  1. Close palette when tabs items are modified
@Don-Vito Don-Vito added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 21, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 21, 2020
@DHowett
Copy link
Member

DHowett commented Nov 21, 2020

My opinion: we should definitely do (1). It’s an ephemeral UI, users should not expect that it will hang around forever.

@zadjii-msft what does Sublime do?

@DHowett DHowett added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Nov 21, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 21, 2020
@DHowett DHowett added this to the Terminal v1.6 milestone Nov 21, 2020
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Nov 21, 2020 •
edited
Loading

I am checking how to do it hypothetically (less trivial than I thought, as dismissing the palette auto focuses the tab content, eventually dismissing menus, etc.).. so if @zadjii-msft likes this approach I will probably have some draft..
I guess we will need to do both 1 and 3 (because closing tab doesn't modify the focus)..

@Don-Vito
Copy link
Contributor Author

@zadjii-msft - if you like the idea I have the code.. but it is probably the worst one I wrote since I was 10: there are several small nuances that I had to handle each one of them.. probably there is some better solution, but I am still not in enough to see it.

@Don-Vito Don-Vito mentioned this issue Nov 23, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Nov 23, 2020
@Don-Vito
Copy link
Contributor Author

Added the PR Draft so it won't get lost.

@zadjii-msft
Copy link
Member

(I want to get back to this before the EOD but I'm also worried I might not, and I'm about to start a 6 day vacation. Just wanted to give a heads up)

@Don-Vito
Copy link
Contributor Author

(I want to get back to this before the EOD but I'm also worried I might not, and I'm about to start a 6 day vacation. Just wanted to give a heads up)

Thanks for the heads up and enjoy your vacation!
Don't be worried - IMHO this quite standalone and not urgent (it can probably resolve #8319, but I don't even now what Priority-2 means in terms of SLAs 😄 )
Happy holidays!

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 24, 2020
@zadjii-msft zadjii-msft added the Area-CmdPal Command Palette issues and features label Dec 1, 2020
@ghost ghost closed this as completed in #8377 Dec 9, 2020
ghost pushed a commit that referenced this issue Dec 9, 2020
@Don-Vito
Make command palette ephemeral (#8377)
e1a8657 
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 #8355
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 9, 2020
@Don-Vito Don-Vito mentioned this issue Dec 9, 2020
Closed
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8377, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@Don-Vito @mpela81
Make command palette ephemeral (microsoft#8377)
818f03b 
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
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.6
Development

Successfully merging a pull request may close this issue.

Make command palette ephemeral Don-Vito/terminal
3 participants
@DHowett @Don-Vito @zadjii-msft