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

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

Closed
Rosefield opened this issue Sep 23, 2021 · 3 comments · Fixed by #12798
Closed

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

Rosefield opened this issue Sep 23, 2021 · 3 comments · Fixed by #12798
Labels
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. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Backlog

Comments

@Rosefield
Copy link
Contributor

Windows Terminal version (or Windows build number)

1.11.2421

Other Software

No response

Steps to reproduce

Run the Rename window... command from command palette, focus is lost from the window until something is clicked.

Expected Behavior

The new name textbox should be focused.

Actual Behavior

Nothing in the window is focused until I click into it with my mouse.
@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 Sep 23, 2021
@zadjii-msft zadjii-msft added the Product-Terminal The new Windows Terminal. label Sep 24, 2021
@zadjii-msft
Copy link
Member

So, yes I suppose we should track this on our side too. From the window renamer code:

terminal/src/cascadia/TerminalApp/AppActionHandlers.cpp

Lines 835 to 846 in 2be394f

// PAIN: We can't immediately focus the textbox in the TeachingTip. It's
// not technically focusable until it is opened. However, it doesn't
// provide an event to tell us when it is opened. That's tracked in
// microsoft/microsoft-ui-xaml#1607. So for now, the user _needs_ to
// click on the text box manually.
//
// 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);
args.Handled(true);

So, we'll also need microsoft/microsoft-ui-xaml#1607

@zadjii-msft zadjii-msft 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. Priority-3 A description (P3) labels Sep 27, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 27, 2021
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Sep 27, 2021
@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 Oct 15, 2021
@zadjii-msft zadjii-msft mentioned this issue Jan 3, 2022
Closed
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
@zadjii-msft zadjii-msft mentioned this issue Mar 31, 2022
Merged
4 tasks
@ghost ghost added the In-PR This issue has a related PR label Mar 31, 2022
@ghost ghost closed this as completed in #12798 Mar 31, 2022
ghost pushed a commit that referenced this issue Mar 31, 2022
@zadjii-msft
Auto-focus window renamer textbox on open (#12798)
b57fe85 
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
@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 Mar 31, 2022
DHowett pushed a commit that referenced this issue Mar 31, 2022
@zadjii-msft @DHowett
Auto-focus window renamer textbox on open (#12798)
7575f4d 
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
DHowett pushed a commit that referenced this issue Mar 31, 2022
@zadjii-msft @DHowett
Auto-focus window renamer textbox on open (#12798)
28443c2 
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
@ghost
Copy link

ghost commented Apr 19, 2022

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

Handy links:

@ghost
Copy link

ghost commented Apr 19, 2022

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

Handy links:

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-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. Priority-3 A description (P3) 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
Backlog
Development

Successfully merging a pull request may close this issue.

Auto-focus window renamer textbox on open microsoft/terminal
3 participants
@DHowett @Rosefield @zadjii-msft