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

Port accessibility fix for the text view control in lightbulb preview #70112

Merged
merged 6 commits into from
Sep 27, 2023

Conversation

mavasani
Copy link
Contributor

Addresses AB#1832532

This PR ports the LSP editor accessibility fix to the Roslyn preview pane implementation. We now show an interactive preview, which has a left border showing + and - for the changes, and keyboard navigation and text selection are now enabled for the preview text view.

Before (non-interactive)

NonInteractivePreview

After (interactive)

InteractivePreview

Addresses [AB#1832532](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1832532)

This PR ports the LSP editor (accessibility fix)[https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/496210] to the original Roslyn preview pane implementation. We now show an interactive preview, which has a left border showin `+` ir `-` for changes, and keyboard navigation and text selection are now enabled for the preview text view
@mavasani mavasani requested a review from olegtk September 25, 2023 10:24
@mavasani mavasani requested a review from a team as a code owner September 25, 2023 10:24
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 25, 2023
{
internal sealed partial class DifferenceViewerPreview
{
private sealed class NavigationalCommandTarget(ITextView textView, IEditorOperations editorOperations) : IOleCommandTarget
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

i really hate this. is there any plan to make a reusable component here? this seems to be taking us backwards wrt to being able to use the editor cleanly.


namespace Microsoft.CodeAnalysis.Editor.Implementation.Preview
{
internal sealed partial class DifferenceViewerPreview : IDifferenceViewerPreview<IWpfDifferenceViewer>
Copy link
Contributor Author

@mavasani mavasani Sep 25, 2023

Choose a reason for hiding this comment

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

This is an existing type that is now moved to the Wpf layer.

Almost all the new code in this file is a port from https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/496210, while also retaining the prior code in https://github.com/dotnet/roslyn/blob/main/src/EditorFeatures/Core/Preview/DifferenceViewerPreview.cs.

@@ -140,7 +140,7 @@
<Separator Name="HeaderSeparator" Margin="0" Visibility="Collapsed"/>
</StackPanel>
<DockPanel Name="PreviewDockPanel" DockPanel.Dock="Top" Visibility="Collapsed">
<ScrollViewer Name="PreviewScrollViewer" IsTabStop="True"
<ScrollViewer Name="PreviewScrollViewer" IsTabStop="False"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tab stop is now moved to the text view control host

@mavasani
Copy link
Contributor Author

@dotnet/roslyn-ide can I please get another review?

@mavasani
Copy link
Contributor Author

Thanks!

@mavasani mavasani merged commit 1e874bd into dotnet:main Sep 27, 2023
@mavasani mavasani deleted the PreviewPaneAccessibilityBug branch September 27, 2023 03:55
@ghost ghost added this to the Next milestone Sep 27, 2023
@mavasani mavasani restored the PreviewPaneAccessibilityBug branch September 27, 2023 03:58
@mavasani mavasani deleted the PreviewPaneAccessibilityBug branch September 27, 2023 06:24
@CyrusNajmabadi
Copy link
Member

@olegtk is this just a point in time thing, until we have a reusable difference viewer with the behavior needed? or is the expectation that roslyn always needs to own and maintain all this code?

@olegtk
Copy link
Contributor

olegtk commented Sep 27, 2023

@CyrusNajmabadi yes, this is a tactical fix to satisfy October 6 SLA for this accessibility bug. Yes, we will provide an interactive diff viewer control in 17.9 to replace this.

@CyrusNajmabadi
Copy link
Member

ok. then i'm ok with this :) thanks @olegtk . @mavasani can you file issue for tracking this?

@mavasani
Copy link
Contributor Author

ok. then i'm ok with this :) thanks @olegtk . @mavasani can you file issue for tracking this?

#70165

@olegtk
Copy link
Contributor

olegtk commented Sep 28, 2023

on our side it's tracked by https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1893411, so far scheduled for 17.9 Preview1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants