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

Pressing Esc should dismiss drop down without changing selected item #42487

Closed
stevencl opened this issue Jan 30, 2018 · 8 comments · Fixed by #43152
Closed

Pressing Esc should dismiss drop down without changing selected item #42487

stevencl opened this issue Jan 30, 2018 · 8 comments · Fixed by #43152
Assignees
@cleidigh
Labels
dropdown DropDown (SelectBox widget) native and custom issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone
February 2018

Comments

@stevencl
Copy link
Member

Issue Type

Bug

Description

Open the output window
Click on the drop down
Press the down and up arrows to change the currently highlighted item.
Press Esc - the drop down closes and the currently highlighted item becomes the selected item
Expected - the drop down should close and the original selected item shoudl remain selected

VS Code Info

VS Code version: Code - Insiders 1.20.0-insider (eed7e19, 2018-01-30T09:51:29.494Z)
OS version: Windows_NT x64 10.0.16299

System Info
Item Value
CPUs Intel(R) Core(TM) i7-3687U CPU @ 2.10GHz (4 x 2594)
Memory (System) 7.89GB (0.83GB free)
Process Argv C:\Program Files\Microsoft VS Code Insiders\Code - Insiders.exe
Screen Reader yes
VM 0%
Extensions (12)
Extension Author (truncated) Version
html-snippets abu 0.2.1
vscode-eslint dba 1.4.4
tslint eg2 1.0.24
vscode-great-icons emm 2.1.22
vscode-gutter-preview kis 0.12.2
csharp ms- 1.13.1
vs-keybindings ms- 0.1.7
vsliveshare ms- 0.1.0-preview.171
team ms- 1.122.0
debugger-for-chrome msj 4.1.0
addDocComments ste 0.0.8
cordova-tools vsm 1.3.3

Reproduces only with extensions

Found when testing: #40589

See how this dropdown works to contrast (I press Esc at the end):
dropdown
@vscodebot vscodebot bot added editor editor-contrib Editor collection of extras labels Jan 30, 2018
@alexdima alexdima removed editor editor-contrib Editor collection of extras labels Jan 30, 2018
@bpasero bpasero assigned cleidigh and unassigned bpasero Jan 30, 2018
@bpasero
Copy link
Member

bpasero commented Jan 31, 2018

@cleidigh I guess somehwere in showSelectDropDown you should remember the currently selected index and in onEscape you should restore that item.

@cleidigh
Copy link
Contributor

cleidigh commented Feb 2, 2018

@stevencl
@bpasero
The custom select box behavior for Escape matches the original and current implementation of the native selectBox. The active selection remains with Escape.

Do we want to change that? Both custom and native?

@bpasero
Copy link
Member

bpasero commented Feb 2, 2018

@cleidigh looks like you are right, the previous behaviour is the same as the custom behaviour. Though it seems to me that the true native behaviour is what @stevencl describes. If it is not a lot of work and a small PR, let's do it. Not high prio though.

@bpasero bpasero added feature-request Request for new features or functionality layout General VS Code workbench layout issues dropdown DropDown (SelectBox widget) native and custom issues and removed layout General VS Code workbench layout issues labels Feb 2, 2018
@raygervais
Copy link
Contributor

I'd like to take a crack at adding this feature if it's within the scope of what is wanted for the custom select box component.

Again, just need some direction towards where I need to look in the code base and I'll get straight to it 💻 👍

@cleidigh
Copy link
Contributor

cleidigh commented Feb 4, 2018

@raygervais
Let's tagteam this as I had already started.

Start by reviewing the select box files:
\src\vs\base\browser\ui\selectBox\selectBoxCustom.ts etc

We have to change the keyboard controllers and used a cached selected option if Escape.
Why don't you start your focus on selectBoxCustom .
I am going to look at the native solution as that has several issues I have already been looking into.

@raygervais
Copy link
Contributor

@cleidigh,
Sounds good!

Should I create a WIP PR so that you and I can discuss from there (after I review and attempt changes)?
Or post here code changes / suggestions?

@cleidigh
Copy link
Contributor

cleidigh commented Feb 5, 2018

@raygervais
A WIP PR sounds great. We can work off that together.

@raygervais
Copy link
Contributor

Noted, will do tonight if I have a chance 👍

@raygervais raygervais mentioned this issue Feb 8, 2018
Merged
cleidigh pushed a commit that referenced this issue Feb 23, 2018
@raygervais @cleidigh
Dropdown: Restore original selection on escape exit Fixes: #42487 (#4…
5c9761a 
…3152)

* Initial WIP

* Added state persistence of original selected option

* Renamed selected option variable, added similar logic to onListBlur

* Removed onListBlur selection

* Added Dropdown Selection handler for onBlur Logic
@bpasero bpasero added this to the February 2018 milestone Feb 23, 2018
@bpasero bpasero added verification-needed Verification of issue is requested verified Verification succeeded labels Feb 23, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@cleidigh cleidigh

Labels
dropdown DropDown (SelectBox widget) native and custom issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Milestone
February 2018
Development

Successfully merging a pull request may close this issue.

Dropdown: Restore original selection on escape exit Fixes: #42487 raygervais/vscode
5 participants
@bpasero @stevencl @alexdima @raygervais @cleidigh