Properly close dialog with escape

[lewis-sanchez]

This PR fixes #23083

Removed the focus from the ok button in the filter dialog, so that the dialog closes the first time the escape key is pressed.

[Charles-Gagnon]

I didn't realize this was because the button was consuming the event - I don't think this is the correct fix then. This behavior applies to ALL buttons in the UI (try it with the buttons for connection dialog for example) and so changing it for this single dialog is going to be confusing for the user.

Instead I would suggest at looking at having the focus initially be in the grid instead of the OK button instead - for one that's just better behavior anyways IMO (you don't have to tab back up to make changes) and that should also solve the issue since it doesn't have the same escape-catching logic the buttons do.

[alanrenmsft]

I think I looked into this issue before, for some reason, vscode decides to handle the escape key on button, your current fix is just a workaround, if the focus has been moved to a button, the problem is still there, the proper fix would be change the button component so that it won't handle the escape key press event but before that we need to understand what made vscode designed it that way.

[lewis-sanchez]

@alanrenmsft, the first commit I made changed the vscode Button class by adding a flag that allowed the parent component to tell the button not to consume the escape key event. The default value for that flag was set to true, so the escape key would be consumed like it currently does now, but could be changed through a setter to stop the event from being consumed in some cases like the one here. Would that approach be more in line with what we want? Here's the commit I'm referring to: 9ea5ec4

[alanrenmsft]

Your change is only for the filter dialog, I think we should fix it for all the places in ADS, it would be nice if you can try to find out why VSCode did it that way, I had a feeling it is not needed, could be a bug of vscode.

[Charles-Gagnon]

@alanrenmsft I would definitely push back that unless we had a compelling reason (i.e. a11y) to change this that it's not worth the effort. It's been like this for basically as long as VS Code has existed and this is the first we've noticed it so I think it's fine to leave it for now.

[alanrenmsft]

@Charles-Gagnon just to clarify, you are saying the issue Lewis is trying to fix here should be closed as a not planned unless it is opened by the a11y test team, right? If so I agree, that is why I didn't investigate it when I first noticed the issue.

[Charles-Gagnon]

I'm fine with closing it - but I think the change should still happen since it's just a better experience to have the focus on the grid to start with anyways. We should assume most people want to actually use the dialog and so having it on the OK button to start with just makes that more annoying.

[lewis-sanchez]

To me it seems counterproductive to fix this for all areas in ADS when no one has reported this kind of issue in the other areas. I think it's better to merge this in to improve the experience with the filter dialog and we can look into improving other areas when the a11y test team or users start to report issues around buttons.

@kburtram, any thoughts on this?

[Charles-Gagnon]

@lewis-sanchez Is there a reason you don't want to make this change? It seems a lot better to me to have the focus start in the grid since the user is going to want to make changes there for anything to happen anyways...most of our dialogs start with the focus in the first element in the dialog so that makes sense.

[lewis-sanchez]

Yeah, I was looking at the behavior for other dialogs like "New Deployment" and "Linked Accounts" and found that those also require double clicks. If we're going to be consistent I was going to leave as is.

Your stance about changing the focus to the first element makes sense. So I will reopen and set the focus to the grid.