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.

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

In specific scenarios, focus the active control #10048

Merged
5 commits merged into from
May 11, 2021

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented May 6, 2021

A redo of #6290. That PR was overkill. In that one, we'd toss focus back to the active control any time that the tab view item got focus. That's maybe not the best solution.

Instead, this PR is precision strikes. We're re-using a lot of what we already have from #9260.

  • When the context menu is closed, yeet focus to the control.
  • When the renamer is dismissed, yeet focus to the control.
  • When the TabViewItem is tapped (meaning no one else handled it), yeet focus to the control.

checklist

scenarios:

  • focus the window by clicking on the tab -> Control is focused.
  • Open the color picker with the context menu, can move the focus inside the picker with the arrow keys.
  • Dismiss the picker with esc -> Control is focused.
  • Dismiss the picker with enter -> Control is focused.
  • Dismiss the renamer with esc -> Control is focused.
  • Dismiss the renamer with enter -> Control is focused.
  • Dismiss the context menu with esc -> Control is focused.
  • Start renaming, then click on the tab -> Rename is committed, Control is focused.
  • Start renaming, then click on the text box -> focus is still in the text box

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Scenario Priority-1 A description (P1) Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels May 6, 2021
@github-actions
Copy link

github-actions bot commented May 6, 2021

Misspellings found, please review:

  • yeet
To accept these changes, run the following commands from this repository on this branch
pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"actionmap reserialize Unregistering "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/f5182355993005639a79331f2c3673485e68a90a.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"unregistering yeet "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@zadjii-msft zadjii-msft changed the title In specific scenarios, focus to the active control In specific scenarios, focus the active control May 7, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you!

Tested out the branch myself and played around with it. Also tested it with NVDA and it looks good to me.

@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label May 11, 2021
@carlos-zamora carlos-zamora added the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label May 11, 2021
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

I can only approve this begrudgingly, after you removed "yeet".
While it's technically still correct unfortunately, it has lost its special place in my heart. 💔

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Love it.

@DHowett
Copy link
Member

DHowett commented May 11, 2021

@msftbot merge this in 1 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 11, 2021
@ghost
Copy link

ghost commented May 11, 2021

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 11 May 2021 23:54:53 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 8564b26 into main May 11, 2021
@ghost ghost deleted the dev/migrie/b/5750-sometimes-focus-moves-weird-again branch May 11, 2021 23:55
@DHowett DHowett removed the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label May 14, 2021
@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label May 14, 2021
DHowett pushed a commit that referenced this pull request May 14, 2021
A redo of #6290. That PR was overkill. In that one, we'd toss focus back to the active control any time that the tab view item got focus. That's maybe not the _best_ solution.

Instead, this PR is precision strikes. We're re-using a lot of what we already have from #9260.
* When the context menu is closed, yeet focus to the control.
* When the renamer is dismissed, yeet focus to the control.
* When the TabViewItem is tapped (meaning no one else handled it), yeet focus to the control.

* [x] I work here
* [ ] This is UI so it doesn't have tests
* [x] Closes #3609
* [x] Closes #5750
* [x] Closes #6680

* [x] focus the window by clicking on the tab -> Control is focused.
* [x] Open the color picker with the context menu, can move the focus inside the picker with the arrow keys.
* [x] Dismiss the picker with esc -> Control is focused.
* [x] Dismiss the picker with enter -> Control is focused.
* [x] Dismiss the renamer with esc -> Control is focused.
* [x] Dismiss the renamer with enter -> Control is focused.
* [x] Dismiss the context menu with esc -> Control is focused.
* [x] Start renaming, then click on the tab -> Rename is committed, Control is focused.
* [x] Start renaming, then click on the text box -> focus is still in the text box

(cherry picked from commit 8564b26)
DHowett pushed a commit that referenced this pull request May 14, 2021
A redo of #6290. That PR was overkill. In that one, we'd toss focus back to the active control any time that the tab view item got focus. That's maybe not the _best_ solution.

Instead, this PR is precision strikes. We're re-using a lot of what we already have from #9260.
* When the context menu is closed, yeet focus to the control.
* When the renamer is dismissed, yeet focus to the control.
* When the TabViewItem is tapped (meaning no one else handled it), yeet focus to the control.

* [x] I work here
* [ ] This is UI so it doesn't have tests
* [x] Closes #3609
* [x] Closes #5750
* [x] Closes #6680

* [x] focus the window by clicking on the tab -> Control is focused.
* [x] Open the color picker with the context menu, can move the focus inside the picker with the arrow keys.
* [x] Dismiss the picker with esc -> Control is focused.
* [x] Dismiss the picker with enter -> Control is focused.
* [x] Dismiss the renamer with esc -> Control is focused.
* [x] Dismiss the renamer with enter -> Control is focused.
* [x] Dismiss the context menu with esc -> Control is focused.
* [x] Start renaming, then click on the tab -> Rename is committed, Control is focused.
* [x] Start renaming, then click on the text box -> focus is still in the text box

(cherry picked from commit 8564b26)
DHowett added a commit that referenced this pull request May 14, 2021
DHowett pushed a commit that referenced this pull request May 14, 2021
A redo of #6290. That PR was overkill. In that one, we'd toss focus back to the active control any time that the tab view item got focus. That's maybe not the _best_ solution.

Instead, this PR is precision strikes. We're re-using a lot of what we already have from #9260.
* When the context menu is closed, yeet focus to the control.
* When the renamer is dismissed, yeet focus to the control.
* When the TabViewItem is tapped (meaning no one else handled it), yeet focus to the control.

* [x] I work here
* [ ] This is UI so it doesn't have tests
* [x] Closes #3609
* [x] Closes #5750
* [x] Closes #6680

* [x] focus the window by clicking on the tab -> Control is focused.
* [x] Open the color picker with the context menu, can move the focus inside the picker with the arrow keys.
* [x] Dismiss the picker with esc -> Control is focused.
* [x] Dismiss the picker with enter -> Control is focused.
* [x] Dismiss the renamer with esc -> Control is focused.
* [x] Dismiss the renamer with enter -> Control is focused.
* [x] Dismiss the context menu with esc -> Control is focused.
* [x] Start renaming, then click on the tab -> Rename is committed, Control is focused.
* [x] Start renaming, then click on the text box -> focus is still in the text box

(cherry picked from commit 8564b26)
DHowett added a commit that referenced this pull request May 14, 2021
ghost pushed a commit that referenced this pull request May 18, 2021
This is a hotfix to #10048. When the tab renamer is opened, we need to make sure to not immediately steal focus from it.

* [x] closes #10112
* [x] I work here
* [x] tested manually
DHowett added a commit that referenced this pull request May 24, 2021
DHowett pushed a commit that referenced this pull request May 24, 2021
This is a hotfix to #10048. When the tab renamer is opened, we need to make sure to not immediately steal focus from it.

* [x] closes #10112
* [x] I work here
* [x] tested manually

(cherry picked from commit 24f80bd)
@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label May 28, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Scenario Needs-Second It's a PR that needs another sign-off Priority-1 A description (P1) Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
4 participants