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

[Inline Rename] Update the UI related to Copilot #71332

Merged
merged 14 commits into from
Dec 30, 2023
Merged

[Inline Rename] Update the UI related to Copilot #71332

merged 14 commits into from
Dec 30, 2023

Conversation

veler
Copy link
Contributor

@veler veler commented Dec 19, 2023 •
edited
Loading

Update the UI of Inline Rename in a manner that when Copilot is available, a click is necessary in order to start getting suggestions. Suggestions are then available in a drop down list.

smart-rename

image

image

image

image

With updated icon:

image

@veler veler requested review from a team as code owners December 19, 2023 00:02
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 19, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 19, 2023
@CyrusNajmabadi
Copy link
Member

What's the key binding for this? I don't think we can pass accessibility review if it always requires a mouse click.

@AmadeusW
Copy link
Contributor

What's the key binding for this? I don't think we can pass accessibility review if it always requires a mouse click.

Designers proposed Tab to set focus on the button, and Enter/Space to invoke it.
Tab doesn't work, and we propose Ctrl+Space as a common way to invoke suggestion lists.
Another suggestion is to use the Alt accelerator key, which is what powers the Include comments, Include strings checkboxes

@CyrusNajmabadi
Copy link
Member

@AmadeusW can you show that on the hover of that button? Thanks!

@vesarautiainen
Copy link

What's the key binding for this? I don't think we can pass accessibility review if it always requires a mouse click.

Designers proposed Tab to set focus on the button, and Enter/Space to invoke it. Tab doesn't work, and we propose Ctrl+Space as a common way to invoke suggestion lists. Another suggestion is to use the Alt accelerator key, which is what powers the Include comments, Include strings checkboxes

I don't mind adding Ctrl+Space to open the list. But I don't really understand why adding the generate button to the tab order is not doable. Isn't that just very standard way to navigate via keyboard (tab to focus to the next tab stop)? Just some examples to how I think it should work:

VS Code search box:
CleanShot 2023-12-19 at 11 41 45@2x

VS Code generate commit message:
CleanShot 2023-12-19 at 11 42 41@2x

VS Copilot chat:
CleanShot 2023-12-19 at 11 43 52@2x

So is the problem that the generate button is inside the text box? Since if it was outside the text box I would expect that tab would move the focus to it. Wouldn't it be quite strange if you can see the button in the UI but there is no way to move the focus to it? I'm just quite adamant with this as I feel that to provide the best keyboard experience that button needs to be focusable.

@veler
Copy link
Contributor Author

veler commented Dec 19, 2023 •
edited
Loading

What's the key binding for this? I don't think we can pass accessibility review if it always requires a mouse click.

@CyrusNajmabadi We can use TAB followed by SPACE key to use it. The focus visual isn't there but I'm working on fixing it.
@AmadeusW, @vesarautiainen should the button NOT be focusable with the keyboard? How will the customer discover Ctrl+Space? Doesn't seem very accessible either if the keyboard is not indicated anywhere.

@sharwell
Copy link
Member

Can you include screenshots of the changes?

@veler
Copy link
Contributor Author

veler commented Dec 19, 2023 •
edited
Loading

Can you include screenshots of the changes?

Here it is. I also included some screenshot in the PR description above.
smart-rename

@vesarautiainen
Copy link

@veler : We can use TAB followed by SPACE key to use it.

This is exactly what I had in mind. Not TAB + SPACE/ENTER at the same time. But just to tab to move the focus to the button and then SPACE/ENTER to invoke it.

@veler
Copy link
Contributor Author

veler commented Dec 20, 2023

@veler : We can use TAB followed by SPACE key to use it.

This is exactly what I had in mind. Not TAB + SPACE/ENTER at the same time. But just to tab to move the focus to the button and then SPACE/ENTER to invoke it.

Great! We can still do this. Do we want to keep Ctrl+Space at the same time too?

@AmadeusW
Copy link
Contributor

Great! We can still do this. Do we want to keep Ctrl+Space at the same time too?

In the discussion outside of this PR, we concluded to keep Tab navigation as well as Ctrl+Space

I verified that the button correctly interacts with the Copilot related API.
I also verified that rename seems to work fine when Copilot is not installed with only one exception:

Pressing TAB causes focus to leave the TextBox (typing works fine, it's just that in current state of the change, the TextBox is grayed out instead of blue)
image

@AmadeusW
Copy link
Contributor

Looks good from my point of view! Consider updating the main PR with up to date screenshots

@Cosifne
Copy link
Member

Cosifne commented Dec 21, 2023 •
edited
Loading

Is the star icon wider than a common button in VS?
image

@veler
Copy link
Contributor Author

veler commented Dec 21, 2023

Is the star icon wider than a common button in VS? image

It was, but the icon and padding got changed since then. Is this better?
image

This was referenced Dec 22, 2023
Closed
Open
@Cosifne
Copy link
Member

Cosifne commented Dec 22, 2023

Also tag @ryzngard here. Could you give this a peek?
This is a request from the editor team, which adds another UI component (copilot suggestion) on top of the inline rename UI.

ryzngard
ryzngard reviewed Dec 28, 2023
View reviewed changes
Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments. The conversation on the UI looks good to me. Make sure to do a pass with accessibility insights to catch what my human eyes can't, and request a manual run at some point to make sure everything is MAS compliant. I can't see any faults, but I don't know MAS by heart.

I give soft approval, since I don't want to unblock merging on behalf of the Roslyn team. Feel free to ping with any questions or follow up needed.

src/EditorFeatures/Core.Wpf/InlineRename/UI/Adornment/RenameFlyout.xaml.cs Show resolved Hide resolved
src/EditorFeatures/Core.Wpf/InlineRename/UI/Adornment/RenameFlyout.xaml.cs Show resolved Hide resolved
src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameUserInputComboBox.xaml.cs Outdated Show resolved Hide resolved
@veler
Copy link
Contributor Author

veler commented Dec 28, 2023

Small comments. The conversation on the UI looks good to me. Make sure to do a pass with accessibility insights to catch what my human eyes can't, and request a manual run at some point to make sure everything is MAS compliant. I can't see any faults, but I don't know MAS by heart.

I give soft approval, since I don't want to unblock merging on behalf of the Roslyn team. Feel free to ping with any questions or follow up needed.

Thanks! I just ran Accessibility Insight and it seems fine. Only thing not good is this button's icon in dark theme, which is already a known issue that existed before this PR:
image

Cosifne
Cosifne reviewed Dec 30, 2023
View reviewed changes
src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameViewModel.cs
var listenerToken = listener.BeginAsyncOperation(nameof(_smartRenameSession.GetSuggestionsAsync));
_smartRenameSession.GetSuggestionsAsync(_cancellationTokenSource.Token).CompletesAsyncOperation(listenerToken);
BaseViewModel = baseViewModel;
this.BaseViewModel.IdentifierText = baseViewModel.IdentifierText;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line not needed?

Cosifne
Cosifne approved these changes Dec 30, 2023
View reviewed changes
Copy link
Member

@Cosifne Cosifne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment.
🚀

@arkalyanms arkalyanms merged commit ebb5887 into dotnet:main Dec 30, 2023
28 checks passed
@ghost ghost added this to the Next milestone Dec 30, 2023
@allisonchou allisonchou mentioned this pull request Dec 31, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzngard ryzngard ryzngard left review comments

@AmadeusW AmadeusW AmadeusW left review comments

@Cosifne Cosifne Cosifne approved these changes

@arkalyanms arkalyanms arkalyanms approved these changes

Assignees
No one assigned
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.9 P3
Development

Successfully merging this pull request may close these issues.
8 participants
@veler @CyrusNajmabadi @AmadeusW @vesarautiainen @sharwell @Cosifne @ryzngard @arkalyanms