Lightbulb header accessibility when selected/focused #24552

dpoeschl · 2018-01-31T06:36:36Z

Two major parts of this change

Porting some old C# to XAML to make theming of the Roslyn part of the lightbulb work. Previously, the top of the lightbulb would bet stuck in whatever theme was first used to show the lightbulb. This was not strictly necessary but very convenient to do, got rid of some bad code, fixed theming of the lightbulb, and helped keep our trigger definitions consistent, making things more maintainable going forward.
Defining triggers to set the Hyperlink & Run foregrounds to something readable when the parent of the PreviewPane is focused. The actual implementation is somewhat hacky, and would be better if the editor exposed a property that let us know the PreviewPanel has keyboard focus, but it's probably too late / onerous to try to coordinate a mutual fix there. This part fixes the actual bug.
- This is done in two parts. One is fixing the actual foreground color when focused. We set it to a definitely-contrasting color, which works exactly as desired in High Contrast. Unfortunately, it turns the link Black in the Blue Theme, for example, when we'd probably prefer it stay blue.
- To compensate for this and to make it more consistent with the rest of the lightbulb, we now underline the link when it is focused instead of showing the dotted border focus visual (which helps in the Blue Theme as mentioned above, but also helps in HC when you can't tell if you're on the right tab stop).

Customer scenario

A sight-impaired user may have a difficult time navigating to the lightbulb's "CS0248" (or similar) hyperlink in High Contrast themes.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/_workitems?id=488229&_a=edit&triage=true

Workarounds, if any

The link is visible before it is selected, so the user can see the link text, then repeatedly hit tab/enter until it navigates (because there are too many tab stops and no indication the link is actually selected).

Risk

The risk is very low. Nothing else builds on this, and it's a simple change. In the very unlikely event a new xaml binding introduced here fails, it won't crash VS or corrupt state (but a color could be wrong).

Performance impact

Basically none. This is a simple xaml change that is only relevant when the lightbulb is opened (on the user action) and when the user tabs through the control (per user action).

Is this a regression from a previous update?

No.

Root cause analysis

Accessibility was not taken into account when the Roslyn part of the lightbulb was built.

How was the bug found?

Work on a related bug uncovered this one. Even though it wasn't filed by accessibility testers or a customer, it is a violation that needs to be fixed.

dpoeschl · 2018-01-31T06:42:37Z

build/config/PublishData.json

@@ -23,7 +23,7 @@
        },
        "dev15.7.x-vs-deps": {
            "nugetKind": "PerBuildPreRelease",
-            "version": "2.7.*",
+            "version": "2.8.*",


Ignore this. I'll figure out how to get rid of it.

dpoeschl · 2018-01-31T06:43:41Z

src/VisualStudio/Core/Def/Implementation/PreviewPane/PreviewPane.xaml

+            <Setter Property="Foreground" Value="{DynamicResource {x:Static vsui:EnvironmentColors.ControlLinkTextBrushKey}}" />
+            <Setter Property="TextDecorations" Value="{x:Null}" />
+            <Setter Property="FocusVisualStyle" Value="{x:Null}" />
+            <Style.Triggers>


Some of these may be redundant or useless, but they're a direct port from C# and I didn't want to make even more changes here.

dpoeschl · 2018-01-31T06:47:54Z

src/VisualStudio/Core/Def/Implementation/Utilities/HyperlinkStyleHelper.cs

-        {
-            // this completely override existing hyperlink style.
-            // I couldn't find one that does this for me or change existing style little bit for my purpose.
-            var style = new Style(typeof(Hyperlink));


This class was only used for the PreviewPane, and it had two problems:

It cached the colors from the first run of the lightbulb, meaning the colors wouldn't update after a theme change.

Implementing the DataTriggers you see added in XAML above would have been way harder to write in C#.

For these reasons, they've been ported to XAML.

dpoeschl · 2018-01-31T06:51:32Z

src/VisualStudio/Core/Def/Implementation/PreviewPane/PreviewPane.xaml

+                    <Setter Property="TextDecorations" Value="Underline" />
+                </Trigger>
+                <DataTrigger Binding="{Binding Path=ParentElement.IsKeyboardFocusWithin, RelativeSource={RelativeSource Mode=FindAncestor, AncestorType={x:Type pp:PreviewPane}}}" Value="True">
+                    <Setter Property="Foreground" Value="{DynamicResource {x:Static vsui:EnvironmentColors.CommandBarTextSelectedBrushKey}}" />


Full disclaimer, in the light theme for example, this makes it so these hyperlinks turn black when focused (you have to tab past Preview Changes | Document | Project | Solution to get the focus back up to the top). This is exactly what we want for the High Contrast themes, but sub-optimal for the regular themes. Given the timeline (doing this the right way would require a new ShellColor in the VS repo to be coordinated with this change) and accessibility correctness of this approach, I vote we accept this very minor drawback.

Why does it need a new color, there are no suitable existing colors?

sounds minor indeed, can you please show a screenshot?

Looks ok to me, although I don't understand why you cannot use the same color as "Preview Changes", "Document" etc links in the bottom panel.

it's a little ugly :(

dpoeschl · 2018-01-31T06:52:46Z

Tagging for review: @dotnet/roslyn-ide @matthejo

333fred · 2018-02-01T20:23:18Z

src/VisualStudio/Core/Def/Implementation/PreviewPane/PreviewPane.xaml

@@ -77,9 +108,12 @@
                                          Checked="ExpanderToggleButton_CheckedChanged" Unchecked="ExpanderToggleButton_CheckedChanged"/>
                            <Border Name="SeverityIconBorder" Height="16" Width="16" Margin="-3,0,0,-2"/>
                            <TextBlock Name="IdTextBlock" Margin="2,0">
-                                <Hyperlink Name="IdHyperlink" IsEnabled="False" RequestNavigate="LearnMoreHyperlink_RequestNavigate"/>
+                                <Hyperlink Name="IdHyperlink" 


Trailing whitespace.

333fred

LGTM other than the trailing whitespace in the XAML file.

olegtk · 2018-02-01T20:45:29Z

src/VisualStudio/Core/Def/Implementation/PreviewPane/PreviewPane.xaml.cs

+        private void PreviewPane_Loaded(object sender, RoutedEventArgs e)
+        {
+            ParentElement = Parent as FrameworkElement;
+        }


nit: unsubscribe from Loaded?

dpoeschl · 2018-02-05T16:51:54Z

Feedback addressed and screenshot/gif attached. @dotnet/roslyn-ide

jinujoseph · 2018-02-05T19:55:22Z

Approved via Link

natidea · 2018-02-05T23:59:36Z

Please retarget this PR to dev15.6.x to get this in that release.

This also lets the colors change appropriately when the theme is changed.

jcouv · 2018-02-06T18:12:02Z

@dpoeschl It seems you pinged the compiler team for review, but I don't see any compiler changes in this PR. Did you mean some other alias?

dpoeschl · 2018-02-06T18:18:47Z

@jcouv -- Hahahaha. I changed the base branch to 15.6.x before I had the correct commits in place, so it pulled in ~120 commits that must have spanned the compiler & interactive parts of the code base. I've removed the requests. :)

dpoeschl · 2018-02-06T18:22:25Z

retest ubuntu_16_debug_prtest please
https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/ubuntu_16_debug_prtest/182/
Tracked by #24671

dpoeschl requested review from a team as code owners January 31, 2018 06:36

dpoeschl force-pushed the lightbulbSelectedAccessibility branch from 24165cf to 3355db7 Compare January 31, 2018 06:39

dpoeschl commented Jan 31, 2018

View reviewed changes

dpoeschl force-pushed the lightbulbSelectedAccessibility branch from 3355db7 to ca0a8fe Compare January 31, 2018 19:05

chborl approved these changes Feb 1, 2018

View reviewed changes

333fred reviewed Feb 1, 2018

View reviewed changes

333fred approved these changes Feb 1, 2018

View reviewed changes

olegtk reviewed Feb 1, 2018

View reviewed changes

olegtk approved these changes Feb 5, 2018

View reviewed changes

jinujoseph added Needs Shiproom Approval Approved to merge and removed Needs Shiproom Approval labels Feb 5, 2018

dpoeschl changed the base branch from master to dev15.6.x February 6, 2018 17:50

dpoeschl changed the base branch from dev15.6.x to master February 6, 2018 17:50

dpoeschl added 5 commits February 6, 2018 10:01

Theme selected Runs in the LightBulb

418b837

Convert C# to XAML

c011f0a

This also lets the colors change appropriately when the theme is changed.

Pick a HC color for the link when selected.

0f8ddb5

Underline the hyperlink when focused (instead of the focus visual)

d35d1fd

Small PR review fixes

cff110b

dpoeschl force-pushed the lightbulbSelectedAccessibility branch from f1a72f3 to cff110b Compare February 6, 2018 18:02

dpoeschl requested review from a team as code owners February 6, 2018 18:02

dpoeschl changed the base branch from master to dev15.6.x February 6, 2018 18:03

dpoeschl removed request for a team February 6, 2018 18:18

dpoeschl mentioned this pull request Feb 6, 2018

M.CA.CS.UT.DeclarationParsingTests.ProduceErrorsOnRef_Properties_Ref_Get failed on ubuntu #24671

Closed

dpoeschl merged commit 7aaa570 into dotnet:dev15.6.x Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightbulb header accessibility when selected/focused #24552

Lightbulb header accessibility when selected/focused #24552

dpoeschl commented Jan 31, 2018 •

edited

Loading

dpoeschl Jan 31, 2018

dpoeschl Jan 31, 2018

dpoeschl Jan 31, 2018 •

edited

Loading

dpoeschl Jan 31, 2018

olegtk Feb 1, 2018

olegtk Feb 1, 2018

dpoeschl Feb 5, 2018

olegtk Feb 5, 2018

Neme12 Feb 6, 2018

dpoeschl commented Jan 31, 2018

333fred Feb 1, 2018

333fred left a comment

olegtk Feb 1, 2018

dpoeschl commented Feb 5, 2018

jinujoseph commented Feb 5, 2018

natidea commented Feb 5, 2018

jcouv commented Feb 6, 2018

dpoeschl commented Feb 6, 2018

dpoeschl commented Feb 6, 2018 •

edited

Loading

Lightbulb header accessibility when selected/focused #24552

Lightbulb header accessibility when selected/focused #24552

Conversation

dpoeschl commented Jan 31, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl commented Jan 31, 2018

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl commented Feb 5, 2018

jinujoseph commented Feb 5, 2018

natidea commented Feb 5, 2018

jcouv commented Feb 6, 2018

dpoeschl commented Feb 6, 2018

dpoeschl commented Feb 6, 2018 • edited Loading

dpoeschl commented Jan 31, 2018 •

edited

Loading

dpoeschl Jan 31, 2018 •

edited

Loading

dpoeschl commented Feb 6, 2018 •

edited

Loading