Auto-focus window renamer textbox on open #12798

zadjii-msft · 2022-03-31T13:57:58Z

Does what it says on the tin. This is maximal BODGE.

TeachingTip doesn't provide an Opened event.
(microsoft/microsoft-ui-xaml#1607). But we
want to focus the renamer text box when it's opened. We can't do that
immediately, the TextBox technically isn't in the visual tree yet. We
have to wait for it to get added some time after we call IsOpen. How do
we do that reliably? Usually, for this kind of thing, we'd just use a
one-off LayoutUpdated event, as a notification that the TextBox was
added to the tree. HOWEVER:

The first time this is fired, when the box is first opened,
yeeting focus doesn't work on the first LayoutUpdated. It does work on
the second LayoutUpdated. Okay, so we'll wait for two LayoutUpdated
events, and focus on the second.
On subsequent opens: We only ever get a single LayoutUpdated. Period.
But, you can successfully focus it on that LayoutUpdated.

So, we'll keep track of how many LayoutUpdated's we've ever gotten. If
we've had at least 2, then we can focus the text box.

We're also not using a ContentDialog for this, because in Xaml Islands a
text box in a ContentDialog won't receive any keypresses. Fun!

References

PR Checklist

Will close out [Terminal> Rename Window]: Keyboard users are not able to access 'Window 1' pop-up when they open it using defined shortcut key. #12021, but that's an a11y bug that needs secondary
validation
Closes Rename window command should automatically focus the new name textbox #11322
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Validation Steps Performed

Tested manually

…ain once the window already has a name

We do get layoutupdated's from the TextBox, but there's no ancestor Popup anywhere. So that's useless. Maybe we can jsut hack and _know_ that the second one always works. That's BODGY AF man.

github-actions · 2022-03-31T14:00:41Z

@check-spelling-bot Report

Unrecognized words, please review:

yeeting

Previously acknowledged words that are now absent

azurewebsites cxcy DCompile debolden deconstructed devicefamily GETKEYSTATE guardxfg LLVM LPCHARSETINFO MAPVIRTUALKEY MSDL ned NOWAIT pgorepro pgort PGU redistributable Timeline timelines unintense UWA UWAs VKKEYSCAN WResult xfg

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/migrie/b/window-renamer-prototype-2 branch:

update_files() {
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('"$patch_remove"');
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/2c2f4f9be230e0be015f9bf799f58ef4bea807eb.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/1084628595" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ 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/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... 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

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the 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).

DHowett · 2022-03-31T16:40:43Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

        //
        // We're also not using a ContentDialog for this, because in Xaml
        // Islands a text box in a ContentDialog won't receive _any_ keypresses.
        // Fun!
        // WindowRenamerTextBox().Focus(FocusState::Programmatic);
+        _renamerLayoutUpdatedRevoker.revoke();
+        _renamerLayoutUpdatedRevoker = WindowRenamerTextBox().LayoutUpdated(winrt::auto_revoke, [weakThis = get_weak()](auto&&, auto&&) {


i believe assigning over this thing does call revoke

I figured, but who knows how often we're burned by that

carlos-zamora

It all looks good to me! Thanks for doing this. 🙂

So is the plan that once the Opened() event gets added, we'll replace the LayoutUpdated section in AppActionHandlers.cpp? Should we file a follow-up? Might be good to keep track of things we'd like to do after ingesting WinUI 2.8 or something like that.

ghost · 2022-03-31T18:15:06Z

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 3 hours 42 minutes. No worries though, I will be back when the time is right! 😉

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.

DHowett · 2022-03-31T18:15:12Z

@msftbot merge this in 1 minute

ghost · 2022-03-31T18:15:15Z

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 Thu, 31 Mar 2022 18:16:14 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".

Does what it says on the tin. This is maximal BODGE. `TeachingTip` doesn't provide an `Opened` event. (microsoft/microsoft-ui-xaml#1607). But we want to focus the renamer text box when it's opened. We can't do that immediately, the TextBox technically isn't in the visual tree yet. We have to wait for it to get added some time after we call IsOpen. How do we do that reliably? Usually, for this kind of thing, we'd just use a one-off LayoutUpdated event, as a notification that the TextBox was added to the tree. HOWEVER: * The _first_ time this is fired, when the box is _first_ opened, yeeting focus doesn't work on the first LayoutUpdated. It does work on the second LayoutUpdated. Okay, so we'll wait for two LayoutUpdated events, and focus on the second. * On subsequent opens: We only ever get a single LayoutUpdated. Period. But, you can successfully focus it on that LayoutUpdated. So, we'll keep track of how many LayoutUpdated's we've _ever_ gotten. If we've had at least 2, then we can focus the text box. We're also not using a ContentDialog for this, because in Xaml Islands a text box in a ContentDialog won't receive _any_ keypresses. Fun! ## References * microsoft/microsoft-ui-xaml#1607 * microsoft/microsoft-ui-xaml#6910 * microsoft/microsoft-ui-xaml#3257 * #9662 ## PR Checklist * [x] Will close out #12021, but that's an a11y bug that needs secondary validation * [x] Closes #11322 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Tested manually (cherry picked from commit b57fe85) Service-Card-Id: 79978834 Service-Version: 1.13

Does what it says on the tin. This is maximal BODGE. `TeachingTip` doesn't provide an `Opened` event. (microsoft/microsoft-ui-xaml#1607). But we want to focus the renamer text box when it's opened. We can't do that immediately, the TextBox technically isn't in the visual tree yet. We have to wait for it to get added some time after we call IsOpen. How do we do that reliably? Usually, for this kind of thing, we'd just use a one-off LayoutUpdated event, as a notification that the TextBox was added to the tree. HOWEVER: * The _first_ time this is fired, when the box is _first_ opened, yeeting focus doesn't work on the first LayoutUpdated. It does work on the second LayoutUpdated. Okay, so we'll wait for two LayoutUpdated events, and focus on the second. * On subsequent opens: We only ever get a single LayoutUpdated. Period. But, you can successfully focus it on that LayoutUpdated. So, we'll keep track of how many LayoutUpdated's we've _ever_ gotten. If we've had at least 2, then we can focus the text box. We're also not using a ContentDialog for this, because in Xaml Islands a text box in a ContentDialog won't receive _any_ keypresses. Fun! ## References * microsoft/microsoft-ui-xaml#1607 * microsoft/microsoft-ui-xaml#6910 * microsoft/microsoft-ui-xaml#3257 * #9662 ## PR Checklist * [x] Will close out #12021, but that's an a11y bug that needs secondary validation * [x] Closes #11322 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Tested manually (cherry picked from commit b57fe85) Service-Card-Id: 79978833 Service-Version: 1.12

zadjii-msft · 2022-04-01T10:44:45Z

Should we file a follow-up

Well, so, I filed the PR to add Opened to MUX, so I REALLY doubt I'll forget to include that one 😜

ghost · 2022-04-19T19:57:41Z

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

Handy links:

ghost · 2022-04-19T19:59:38Z

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

Handy links:

zadjii-msft added 5 commits March 31, 2022 06:02

this seemingly works, but then auto-closes when you try opening it ag…

90c7377

…ain once the window already has a name

This doesn't work at all

c260409

We do get layoutupdated's from the TextBox, but there's no ancestor Popup anywhere. So that's useless. Maybe we can jsut hack and _know_ that the second one always works. That's BODGY AF man.

This is unbelievably stupid and I'm not at all surprised that this works

d7356a5

comment wtf is going on

6a76ea6

more cleanup

9469b93

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Mar 31, 2022

more comments

292af33

DHowett approved these changes Mar 31, 2022

View reviewed changes

DHowett requested a review from carlos-zamora March 31, 2022 17:15

carlos-zamora approved these changes Mar 31, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 31, 2022

ghost merged commit b57fe85 into main Mar 31, 2022

ghost deleted the dev/migrie/b/window-renamer-prototype-2 branch March 31, 2022 18:17

zadjii-msft added Needs-Second It's a PR that needs another sign-off and removed Needs-Second It's a PR that needs another sign-off labels Apr 1, 2022

zadjii-msft mentioned this pull request Apr 8, 2022

Ability to save selected text as a sendInput action #12857

Closed

ghost mentioned this pull request Apr 19, 2022

Rename window command should automatically focus the new name textbox #11322

Closed

zadjii-msft mentioned this pull request May 2, 2022

[megathread] Accessibility links #9990

Open

12 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-focus window renamer textbox on open #12798

Auto-focus window renamer textbox on open #12798

zadjii-msft commented Mar 31, 2022 •

edited by DHowett

Loading

github-actions bot commented Mar 31, 2022 •

edited

Loading

DHowett Mar 31, 2022

zadjii-msft Mar 31, 2022

carlos-zamora left a comment

ghost commented Mar 31, 2022

DHowett commented Mar 31, 2022

ghost commented Mar 31, 2022

zadjii-msft commented Apr 1, 2022

ghost commented Apr 19, 2022

ghost commented Apr 19, 2022

Auto-focus window renamer textbox on open #12798

Auto-focus window renamer textbox on open #12798

Conversation

zadjii-msft commented Mar 31, 2022 • edited by DHowett Loading

References

PR Checklist

Validation Steps Performed

github-actions bot commented Mar 31, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

DHowett Mar 31, 2022

Choose a reason for hiding this comment

zadjii-msft Mar 31, 2022

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

ghost commented Mar 31, 2022

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.

DHowett commented Mar 31, 2022

ghost commented Mar 31, 2022

zadjii-msft commented Apr 1, 2022

ghost commented Apr 19, 2022

ghost commented Apr 19, 2022

zadjii-msft commented Mar 31, 2022 •

edited by DHowett

Loading

github-actions bot commented Mar 31, 2022 •

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.