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

Escaping from a custom menu loses focus #90729

Closed
chrisjj opened this issue Feb 14, 2020 · 15 comments · Fixed by #97965
Closed

Escaping from a custom menu loses focus #90729

chrisjj opened this issue Feb 14, 2020 · 15 comments · Fixed by #97965
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues menus Menu items and widget issues
Milestone

Comments

@chrisjj
Copy link

chrisjj commented Feb 14, 2020

1 Tab to this
image
2 Press Enter
image
3 Press Esc

Expected: focus returns to '...'
Observed: '...' button disappears and the focus moves to somewhere invisible.
image

V 1.42.1 on Windows 7.

@isidorn
Copy link
Contributor

isidorn commented Feb 17, 2020

This works fine on the mac with native context menus.
The problem is windows and custom menus. Thus renaming and forwarding to @sbatten

@isidorn isidorn assigned sbatten and unassigned isidorn Feb 17, 2020
@isidorn isidorn added the menus Menu items and widget issues label Feb 17, 2020
@isidorn isidorn changed the title Escaping from a drop-down can lose button and focus Escaping from a custom menu loses focus Feb 17, 2020
@chrisjj
Copy link
Author

chrisjj commented Feb 17, 2020

Thanks. Any idea why Windows native menus are not used?

@isidorn
Copy link
Contributor

isidorn commented Feb 17, 2020

They have a lot of issues, for example with accessiblity, thus we use custom menus on Windows.

@chrisjj
Copy link
Author

chrisjj commented Feb 17, 2020

Thanks. I'm surprised Mac has fewer accessibility issues.

@sbatten sbatten added bug Issue identified by VS Code Team member as probable bug accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues labels Feb 18, 2020
@sbatten sbatten modified the milestones: Backlog, On Deck Feb 18, 2020
@sbatten
Copy link
Member

sbatten commented Mar 3, 2020

same as #90730, cannot reproduce in master

@sbatten sbatten removed this from the On Deck milestone Mar 3, 2020
@sbatten sbatten added info-needed Issue requires more information from poster and removed bug Issue identified by VS Code Team member as probable bug labels Mar 3, 2020
@eamodio
Copy link
Contributor

eamodio commented Mar 4, 2020

I can reproduce this on master

@sbatten sbatten removed the info-needed Issue requires more information from poster label Mar 4, 2020
@sbatten
Copy link
Member

sbatten commented Mar 4, 2020

got it, always show actions setting breaks repro.

@sbatten sbatten added this to the March 2020 milestone Mar 4, 2020
@isidorn
Copy link
Contributor

isidorn commented Mar 5, 2020

Somewhat similar #91085
When you press esc the focus should ideally return to the outer continer (like the tree does). If that is not possible the focus should not move and stay put.

@isidorn
Copy link
Contributor

isidorn commented Mar 5, 2020

Actually that might be the same issue since the custom menu extends from action bar.
What I suggest is the following: in IActionBarOptions we introduce an optional attribute focusOnBlur: HTMLElement that the creators of the actionBar can pass in.
The ActionBar would then pass focus to that element when user presses esc.
Alternative is that all ActionBar users need to listen to onDidCancel event and when the event fires they would pass focus to the appropriate element - similar to what the menu already does.
There are a lot of ActionBar clients and I think all of them need this behavior fixed.

Alternative is just to make esc a no-op in non menu use cases. However I think passing focus to the outer element is actually what the users might expect as @jvesouza points out

Let me know what you think @sbatten @bpasero

@chrisjj
Copy link
Author

chrisjj commented Mar 6, 2020

we introduce an optional attribute focusOnBlur: HTMLElement that the creators of the actionBar can pass in.

Can't you stack the previous holder and then restore?

Alternative is just to make esc a no-op in non menu use cases.

I vote against.

@bpasero
Copy link
Member

bpasero commented Mar 6, 2020

Alternative is just to make esc a no-op in non menu use cases.

I vote against.

Also, -1 for that. It is relatively simple: check what the OS does when you open a menu and press ESC and mimic that. After all we try to make the menu as close to the native experience as possible.

@sbatten sbatten modified the milestones: March 2020, April 2020 Apr 3, 2020
@sbatten sbatten added this to the April 2020 milestone Apr 3, 2020
@sbatten
Copy link
Member

sbatten commented Apr 30, 2020

fix is likely going to have to involve this #93415 (comment)

@sbatten sbatten modified the milestones: April 2020, May 2020 Apr 30, 2020
sbatten added a commit to sbatten/vscode that referenced this issue May 16, 2020
sbatten added a commit that referenced this issue May 27, 2020
…97965)

highly scoped implementation for select boxes and dropdowns that need it
fixes #90729
fixes #93415
@chrisjj
Copy link
Author

chrisjj commented May 27, 2020 via email

@sbatten
Copy link
Member

sbatten commented May 27, 2020

@chrisjj The issue has been resolved with 01b37c6

@chrisjj
Copy link
Author

chrisjj commented May 27, 2020 via email

@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues menus Menu items and widget issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants