Dropdown: Restore original selection on escape exit Fixes: #42487

[raygervais]

Fixes #42487

Change new custom select (dropdown) to revert to current selection if user presses Escape or
blurs focus by pressing the mouse outside the drop-down.

The native select behavior remains unchanged by design.

[cleidigh]

@raygervais
Thanks for the PR 👍
I've cloned your repo and in building now. I'll post some review comments after further inspection.

[cleidigh]

on ... prefix within object variable names typically is used to indicate event methods or event action variables.

Probably should you something like:

_currentSelection

[cleidigh]

Within onEscape we should do a couple different things:

this.selectElement.selected = this.select(this._onOriginalSelect);

Not completely sure about the above. this.select()
does not return any value. Also this.selectElement.selected
does not exist.

We can just use:

this.select(this._onOriginalSelect);

Second:

I think we should actually not fire the onSelected event. Since we have not changed from the current selection I don't think it makes sense.

[cleidigh]

@raygervais
For consistency I think we should also restore the current selection when onBlur occurs in the same fashion
as Escape.

Lastly I am still working on what to do with the native select. Keyboard events are handled very differently when a native HTML select is expanded. We may not be able to make that clean.

FYI - I have to figure out how to quote code for reviews better , sorry 👎

[raygervais]

@cleidigh, quoting code reviews this way deff keeps me on my toes, so I don't mind. It's all a learning process in the end for us all 👍.

I've made the requested updates:

• Changing _onOriginalSelection to _currentSelection
• Updated selection logic to the much better idea of this.select(this._currentSelection);
• Added said logic to onListBlur function

I'm curious how you are thinking of handling the native HTML select list, but again happy to learn and help. Sorry if this PR seems rather messy or a shot in the dark, I haven't worked in the side of Code before, nor have I interacted with custom select components. It's been a great eye-opener so far of how Code and UI Components work 😄

Question, when you said we should remove the select event, were you referring to lines such as https://github.com/Microsoft/vscode/blob/master/src/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L532 ?

[raygervais]

@cleidigh, when you have a chance to review / critique, I'd love to understand this further and fix any issues that exist with this PR 😄 Then I can do some final testing / logic changes where appropriate.

👍

PS. Renamed based on your comment from last PR, glad to follow a semantic which is much cleaner. I did leave WIP tag in just incase there is unstable functionally that my part added, will remove once you give go ahead.

[raygervais]

Update
While testing, I found on Linux (Fedora 27 in this case) that unless you hit enter and move via the up / down arrows, it reverts to original selection. Likewise clicking does not alter selection. Looking into this now.

The issue appears to be the consequence of onListBlur(), which was suggested to mimic the same as onEscape. Removing this line (530) from my PR fixes the issue mentioned above.

this.select(this._currentSelection);

[cleidigh]

@raygervais
on your linux Comment
is this after your changes?
this should work the same way as Windows assuming the custom list

[raygervais]

@cleidigh, that comment was before the final commit with the single line change since I tested on Windows as well last night and found the fix / expected behavior to be relational to what I had noticed.

This is resolved now in latest commit.

[cleidigh]

@raygervais
so
Windows behavior === Linox and Escape/MouseBlur restores initial selection
yes?

[raygervais]

Yes,

Sorry for the confusion, I really need to think of what to say before attempting voice dictation. The expected update / accessibility fix is here and working on both Windows and Linux.

[cleidigh]

@raygervais
do you use voice based programming or just dictation sometimes?

I ask because I can only do voice programming based upon my disability.

[cleidigh]

@bpasero
@raygervais

@raygervais Has the tweaks for restoring the initial selection upon Escape/passive blur
with the custom select box. This behavior change makes sense to me.

I do not think we should to change the native select mode based on the following
after research and experiments:

• a native HTML select does not emmit keyboard events when the select is expanded

• there is a quirky behavior where a keyup event can be received for an Escape
  AFTER the select closes.

• we cannot detect a selection based upon Enter

• getting the event after the selection changes does not provide decent Control
  also for the debug configurations, the action to add a new configuration
  would already have fired

  Thoughts?

[raygervais]

@cleidigh, honestly I use dictation while I'm on the move such as walking home from work in the cold. Essentially, I dictate only when out and about. Curious what do you use for dictation?

Furthermore, I do agree with the idea to not change native HTML select behaviour. 👍

[bpasero]

Yeah I would not touch the native select box.

[cleidigh]

@raygervais
I use Dragon fourteen for programming dictation and everything.
FYI will try to close this up tomorrow, thanks for being patient I've been running slow 👎

[cleidigh]

@raygervais
Not sure if I have missed something, but the current PR does not handle the onBlur case.
This is actually a bit trickier then the Escape handling. The reason is that the blur event fires
on every dropDown close so we cannot reset the selection on all close paths.

One approach is to set this._currentSelection = -1 if:

• we do in active exit with Enter or a mouse selection.
• check if we have made a change to the selection ( _currentSelection === -1 ) within onBlur reset if not

Check out the following and try little more testing on Windows and Linux to see if we have everything
in good order.

private onListBlur(): void {
if (this._currentOption > 0) {
this.select(this._currentOption);
}

	this._onDidSelect.fire({
		index: this.selectElement.selectedIndex,
		selected: this.selectElement.title
	});

	this.hideSelectDropDown(false);
}

private onEnter(e: StandardKeyboardEvent): void {
dom.EventHelper.stop(e);

	// Track that we made an active change
	this._currentOption = -1;
	
	this.hideSelectDropDown(true);
	this._onDidSelect.fire({
		index: this.selectElement.selectedIndex,
		selected: this.selectElement.title
	});
}


private onMouseUp(e: MouseEvent): void {

	// Check our mouse event is on an option (not scrollbar)
	if (!e.toElement.classList.contains('option-text')) {
		return;
	}

	const listRowElement = e.toElement.parentElement;
	const index = Number(listRowElement.getAttribute('data-index'));
	const disabled = listRowElement.classList.contains('option-disabled');

	// Ignore mouse selection of disabled options
	if (index >= 0 && index < this.options.length && !disabled) {
		this.selected = index;
		this.select(this.selected);

		this.selectList.setFocus([this.selected]);
		this.selectList.reveal(this.selectList.getFocus()[0]);
		this._onDidSelect.fire({
			index: this.selectElement.selectedIndex,
			selected: this.selectElement.title
		});

		// Track that we made an active change
		this._currentOption = -1;
		
		this.hideSelectDropDown(true);
	}
	dom.EventHelper.stop(e);
}

[raygervais]

Hi @cleidigh,
You are correct regarding the onBlur case. This is what I had mentioned previously, but perhaps the issue was lost in translation. I had hadn't thought through at the time how else the onBlur case was used, and thought that it was not required to do the select statement in said function since it would never resolve to what the user had selected prior to closing the dropdown.

I will correct and test tonight if time permitting. Thanks for the pointer! This will be me for the next few hours :

[cleidigh]

@raygervais
Sounds good. I guess this is an example of something seemingly simple with more tentacles than expected.

As a note, these types of issues and the ability to control them were one of the main impetus for replacing the limited native select.

[raygervais]

Noted @cleidigh, was curious why the need for a non-native drop down had developed.
I've fixed based on your example, and tested on Linux / Windows. Seems to cover all edge cases I can think of:

• Key Up + ESC (expected behaviour)
• Key Down + ESC (expected behaviour)
• Key Up + ENTER (normal behaviour)
• Key Down + ENTER (normal behaviour)
• Mouse Click on Valid Item (normal behaviour)
• Mouse Click on anywhere but drop down (expected behaviour)

I'm glad that I'm learning from this how very simple logic can influence the entire operation of a UI component. This level of scope (low level, singular component scope) is very different from my experience of high level full UI layout scope (interaction between components, much less data interaction and manipulation in a single component's scope). 👍

[cleidigh]

@raygervais
I hope you saw I merged this "finally" 💯

I've been working with code for a year and still only know a fraction of the CodeBase.
every PR you do you learn something new.
what's really nice just when you get to the point where you can make contributions for new items.

Cheers - don't hesitate on any questions in the future...

[raygervais]

@cleidigh, thanks for the awesome mentoring and tips! It really did help a lot with my navigational skills around code and the custom dropdown logic. I really hope to contribute more, and perhaps even play a part if you'd be so keen in helping port the custom dropdown component for MacOS instances!

[cleidigh]

@raygervais
no problem at all

BTW the custom drop-down was purposely only provided for windows and Linux. it was actually a design decision because the team thought the Mac version was good.

if you inspect the code in particular selectbox.ts you will see the conditional for the platform:
if(isMacintosh)

if you drop this condition the code will actually use the custom select box on all platforms , no porting necessary.

You could propose a change such as a configurable parameter to use the custom select box on all platforms. this might be possible to add after some more feedback from users but if you want to you could go and create an issue now.