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

When the tab receives focus, immediately move focus to the active control #6290

Closed
wants to merge 2 commits into from
Closed

When the tab receives focus, immediately move focus to the active control #6290

wants to merge 2 commits into from

Conversation

zadjii-msft
Copy link
Member

I'm mostly just putting the PR out there for comments. I'm worried that this might have horrifying unintentional side-effects for accessibility, which I'm not loving.

This PR makes the Tab immediately toss focus to the active control, whenever the Tab receives focus, for any reason.

@carlos-zamora will this horribly break accessibility?

My other theory here is to pre-emptively prevent the tab from receiving focus. That would probably involve

  • Focusing the control when a tab is left clicked
  • Manually passing focus to the control when any flyout is closed
  • Manually passing focus to the control when the color picker is closed
  • Manually passing focus to the control when the tab renamer is dismissed
  • Manually passing focus to the control when the command palette is closed
  • Manually passing focus to the control when <any future UI> is closed

So that might not scale as well, but that might be the more correct solution.

@zadjii-msft
Try moving focus to the control when the tab itself gains focus. This…
aa2fe26 
… doesn't work when the unfocused tab is renamed, and might do weird things with accessibility
@zadjii-msft
This is better
548dd0e
@zadjii-msft zadjii-msft requested a review from carlos-zamora June 1, 2020 12:56
@ghost 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-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jun 1, 2020
@miniksa
Copy link
Member

miniksa commented Jun 1, 2020

I don't know about this. I recall there being some UI paradigm where you can use the tab key to move around between UI elements. And if that element is a tab row, you can then use the arrow keys to move a highlight between the tabs and press spacebar to then switch to the highlighted one. And if you're shirking the focus off the tab row anytime it hits... that's probably broken. I don't know if that's a part of the TabView control that we get from MUX, but I'm doing that right now in Microsoft Edge and it works that way...

@zadjii-msft
Copy link
Member Author

Yea, that was basically exactly my feeling with this PR. It seemed like "it worked", but not the right way.

Unless anyone else has any enlightened ideas about how to do this in a more clever way, I think I'm going to change this to the more manual "this control knows it's been closed, so it'll ask for focus to go back to the Terminal". It'll be more manual work to do for each and every UI element, but seems like the only good solution.

@carlos-zamora
Copy link
Member

Downloaded and did some testing:

  • Narrator:
    • user can use the Narrator+Left and Narrator+Right commands to navigate to other UI elements. So the user is still able to access the tabs (aside from using our custom keybindings to invoke them). (reference)
  • NVDA:
    • The good news is that NVDA already separates UI navigation from where the focus actually is (there's an NVDA keybinding to specifically navigate to the focused object). So, this basically doesn't affect it.
    • (unrelated to this PR) The bad news is, NVDA's UI navigation model exposes so many ugly details in our UI (i.e.: the cutout is something you can navigate to as an "unknown" object). This makes finding the tabs a pain (Tab Row Control --> list --> ), and I wasn't able to find the dropdown. The workaround here though is to use the dropdown keybinding.
    • All of these problems already existed before though. So I'll file an issue to track that specifically since it's unrelated to this PR.

TL;DR: this did not horribly break accessibility

@zadjii-msft zadjii-msft self-assigned this Jul 14, 2020
@mle-ii
Copy link

mle-ii commented Aug 14, 2020

Is this just waiting on the reviewer to approve? Besides the merge conflicts, or is there some other blocker here?

@zadjii-msft
Copy link
Member Author

I've kinda just left this stagnate while we were building some other UI elements, because this probably needs to be solved in a way that's more cohesive across various different UI elements. Most of that work is being tracked in #6680. I want to make sure that however we actually do end up fixing this is the right solution, not just a hack, and this fix seems like a hack ATM.

@DHowett DHowett mentioned this pull request Aug 14, 2020
Closed
@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27
@zadjii-msft zadjii-msft mentioned this pull request Feb 16, 2021
Closed
6 tasks
@zadjii-msft zadjii-msft mentioned this pull request May 6, 2021
Merged
14 tasks
@zadjii-msft
Copy link
Member Author

Closing in favor of the much more targeted strike in #10048

@zadjii-msft zadjii-msft closed this May 6, 2021
ghost pushed a commit that referenced this pull request May 11, 2021
@zadjii-msft
In specific scenarios, focus the active control (#10048)
8564b26 
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 
* [x] I work here
* [ ] This is UI so it doesn't have tests
* [x] Closes #3609
* [x] Closes #5750
* [x] Closes #6680

### scenarios:

* [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
DHowett pushed a commit that referenced this pull request May 14, 2021
@zadjii-msft @DHowett
In specific scenarios, focus the active control (#10048)
b345d39 
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
@zadjii-msft @DHowett
In specific scenarios, focus the active control (#10048)
042f9ef 
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
@zadjii-msft @DHowett
In specific scenarios, focus the active control (#10048)
aead759 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

Assignees

@zadjii-msft zadjii-msft

Labels
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-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
4 participants
@zadjii-msft @miniksa @carlos-zamora @mle-ii