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

Screen reader does not read the heading of RadioButtons control #3183

Closed
alekhyareddy28 opened this issue Aug 21, 2020 · 23 comments · Fixed by #5094
Closed

Screen reader does not read the heading of RadioButtons control #3183

alekhyareddy28 opened this issue Aug 21, 2020 · 23 comments · Fixed by #5094
Labels
A11yMAS A11yMediumImpact A11ySev2 Non-blocking for core user tasks or blocking for non-core user tasks. accessibility Narrator, keyboarding, UIA, etc area-RadioButtons team-Controls Issue for the Controls team
Milestone

Comments

@alekhyareddy28
Copy link

Describe the bug

Steps to reproduce the bug

Steps to reproduce the behavior:

  1. Create a radiobuttons control as follows
<UserControl
    x:Class="Microsoft.PowerToys.Settings.UI.Views.ShellPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    xmlns:behaviors="using:Microsoft.PowerToys.Settings.UI.Behaviors"
    xmlns:winui="using:Microsoft.UI.Xaml.Controls"
    xmlns:helpers="using:Microsoft.PowerToys.Settings.UI.Helpers"
    xmlns:views="using:Microsoft.PowerToys.Settings.UI.Views"
    xmlns:ic="using:Microsoft.Xaml.Interactions.Core"
    xmlns:i="using:Microsoft.Xaml.Interactivity"
    xmlns:muxc="using:Microsoft.UI.Xaml.Controls"
    mc:Ignorable="d">


    <Grid>
        <muxc:RadioButtons Header="abcd">
            <RadioButton>Item 1</RadioButton>
            <RadioButton>Item 2</RadioButton>
            <RadioButton>Item 3</RadioButton>
        </muxc:RadioButtons>
    </Grid>
</UserControl>

Expected behavior

When we tab into the radio button the narrator must read the heading as well as the radio button that is in focus.
Eg: something along the lines of "abcd grouping Item 1 button 1 of 3".

Actual Behavior

It just says "Item 1, 1 of 3". It does not read out the heading set by the radiobuttons header. Setting an AutomationProperty does not fix the issue either.

Screenshots

Version Info

NuGet package version:

Microsoft.UI.Xaml.2.5.0-prerelease.200708003

Windows app type:

UWP Win32
Yes
Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041)
November 2019 Update (18363)
May 2019 Update (18362) Yes
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT

Additional context

Link to issue in the PowerToys repository - microsoft/PowerToys#6032

@ranjeshj
Copy link
Contributor

@StephenLPeters @YuliKl as FYI.

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Aug 31, 2020

I'm no expert with the implementation of accessibility info so I don't assume I have the complete picture here. That said, I took a closer look at this. According to the documentation, headered controls typically would use the AutomationProperties.LabeledBy API to read out the (text-based) header when a control gets focus. The issue here seems to be that LabeledBy is supposed to work with text elements (and as such controls which expose text, like TextBox or TextBlock.) The header control of the RadioButtons control, however, is a ContentPresenter with a Content API of type object. Passing the ContentPresenter to LabeledBy does not cause the header to be read, even if the actual ContentPresenter content is a string.

I did try to look into providing an own GetLabeledByCore implementation for the RadioButtons control here in the hope of working around this but I was not successful.

An attempt which did prove to be successful here is to create a TextBlock in "code behind", establish the LabeledBy relation with it and set its Text property to the header content - if it's a string:

// In RadioButtons::OnApplyTemplate()
if (const auto stackPanel = GetTemplateChildT<winrt::StackPanel>(L"RootPanel", controlProtected))
{
    m_headerUIATextBlock = winrt::TextBlock();
    winrt::AutomationProperties::SetLabeledBy(stackPanel, m_headerUIATextBlock);

    UpdateHeader();
}

void RadioButtons::UpdateHeader()
{
    if (m_headerUIATextBlock != nullptr)
    {
        m_headerUIATextBlock.Text(SharedHelpers::TryGetStringRepresentationFromObject(Header()));
    } 
}

void RadioButtons::OnPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args)
{
    winrt::IDependencyProperty property = args.Property();
   
    if (property == s_HeaderProperty)
    {
        UpdateHeader();
    }

    // More properties left out here....
}

While this works, I am not sure if that is a reasonable solution here. Controls like TextBox and ComboBox also have a Header property of type object and Narrator reads out their header value correctly (if it's a string). So it might be reasonable to just re-use the existing code used for the ComboBox,... in the RadioButtons context as well.

@ranjeshj @StephenLPeters FYI.

(P.S.: Again, without me knowing the full picture, could AutomationProperties.LabeledBy perhaps be updated to also work with UIElements which don't have a Text property but a more generic Content property? The Content property could then be read out by Narrator if its value happens to be a textual value. That way, LabeledBy would "just work" even if the bound UIElement is an element like a ContentPreseter with content like "I'm a header".)

@StephenLPeters
Copy link
Contributor

The TextBox hack is pretty funny but seems unreasonable. @YuliKl do you know the proper way to set up this relationship? @MikeHillberg we've been doing a lot of changing properties which would naively be strings to objects instead to allow for more customization. This could be a potential issue with that strategy?

@YuliKl YuliKl added the accessibility Narrator, keyboarding, UIA, etc label Sep 1, 2020
@YuliKl
Copy link

YuliKl commented Sep 1, 2020

I don't know the right way to implement this. The issue definitely feels like a bug we should try to fix. @kmahone any ideas?

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Sep 1, 2020

The header is read out fine by Narrator for a ComboBox, for example, which seems to have more or less the same setup as our RadioButtons control here (it also uses a ContentPresenter for its header). So my first thought here would be to just take a look at how the ComboBox implements this Narrator behavior and then just shamelessly copy/paste that logic into our RadioButtons control.

@ranjeshj
Copy link
Contributor

ranjeshj commented Sep 1, 2020

Looking at ComboBoxAutomationPeer, we override GetNameCore, call the base to see if there is already a name set (through automation name property). If an automation name is not set, then we provide the header as the name.

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Sep 1, 2020

@ranjeshj I will take a look if that will work here as well (I faintly recall I tried that a few days ago to no avail but perhaps I missed something).

@Felix-Dev
Copy link
Contributor

@ranjeshj As I thought to recall, overriding GetNameCore() alone doesn't appear to work. Here is what I did:

I created a new RadioButtonsAutomationPeer class and overrode GetNameCore() like this:

hstring RadioButtonsAutomationPeer::GetNameCore()
{
    winrt::hstring name = __super::GetNameCore();

    if (name.empty())
    {
        if (const auto RadioButtons = Owner().try_as<winrt::RadioButtons>())
        {
            name = SharedHelpers::TryGetStringRepresentationFromObject(RadioButtons.Header());
        }
    }

    return name;
}

When focus enters a RadioButtons control with a header "TestRadioButtons", the header isn't read out even though GetNameCore() returns the correct value:

image

image

Is the ComboBoxAutomationPeer doing anything else here?

@ranjeshj
Copy link
Contributor

ranjeshj commented Sep 4, 2020

I don't see anything else that might affect it in ComboBox. I do notice that in the case of ComboBox, ComboBox can take focus, so that might be when Narrator is reading it out. Following up with accessibility folks.

@ranjeshj
Copy link
Contributor

ranjeshj commented Sep 8, 2020

These are the 3 requirements for the screen reader to read it out.

  1. The element that you want to be read as a context of other controls has to be an ancestor of those other controls. (in the UIA tree).
  2. The element has to have an accessible name (in your case, AutomationProperties.Name).
  3. The element has to be one of control types that Narrator perceives as a context control (for instance, Groups and Panes are controls like that).

Can you inspect the tree via Accessibility Insights and see we are meeting these. I suspect #3 is what might be missing.

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Sep 29, 2020

@ranjeshj Sorry for the late reply.

I created the following test page:
image

As you can see, I have a ComboBox with the header "ComboBoxHeader" and a RadioButtons instance with the header "RadioButtonsHeader". Narrator reads out the ComboBox header correctly, but fails to mention the RadioButtons header when stepping into the RadioButtons control with focus,

Accessibility insights gives me the following for these two controls:

ComboBox

Control MUX UI AI Windows
ComboBox image image
RadioButtons image image

What I can notice is that for the ComboBox, the header is actually part of the control. For the RadioButtons control, however, it looks like the header is just acting as a stand-alone text component.

@ranjeshj Do these findings help you?

@crutkas
Copy link
Member

crutkas commented Oct 16, 2020

Note in PowerToys, we got flagged for accessibility on this

@YuliKl YuliKl added the A11yMAS label Oct 30, 2020
@Felix-Dev
Copy link
Contributor

@ranjeshj Are the images above helpful and if yes, what would be the next steps here? (If not, please let me know what additional images/gifs I should upload.)

@ranjeshj
Copy link
Contributor

ranjeshj commented Dec 1, 2020

@StephenLPeters @MikeHillberg thoughts ?

@Felix-Dev
Copy link
Contributor

@ranjeshj @StephenLPeters @MikeHillberg I want to point out that we still have the workaround mentioned above available to use if this should be fixed soon and providing proper support (this might be an ItemsRepeater issue where it might lack some support for a control header?) would take longer.

@crutkas
Copy link
Member

crutkas commented Dec 1, 2020

@Felix-Dev while yes but when @StephenLPeters responds with the item below, it doesn't exactly inspire confidence to adopt

The TextBox hack is pretty funny but seems unreasonable

also if we adopt the hack, how would the hack work with the fix as if adopted, we could take the fix without realizing it.

@marcelwgn
Copy link
Contributor

Maybe, to fix this "properly", from a UIA perspective, RadioButtons should be a panel or something similar so that Narrator will perceive it as a control with children. That way, it would announce the header and then the selected item while also allowing users to understand the control easier conceptually.

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Dec 1, 2020

@crutkas I am not entirely sure I understand your comment. The workaround using the TextBlock would be WinUI internal so customers such as PowerToys would just see that the header is pronounced as expected when focusing the RadioButtons control. Once WinUI has a proper fix ready, the workaround would be removed in a new WinUI version and customers won't notice any difference.

I agree with the sentiment expressed here though that we should strive to get this fixed without using such a workaround as other headered controls like the ComboBox also get the job done without it.

@chingucoding Sounds good to me though as I confessed some time ago, I am a novice to all things UIA considered so I will likely take some time tomorrow to read up more on this.

@crutkas
Copy link
Member

crutkas commented Dec 2, 2020

Ah, i thought you wanted the PT code base to have the workaround

@carlos-zamora
Copy link
Member

Windows Terminal will probably get flagged for accessibility on this too.
CC @DHowett and @cinnamon-msft for visibility

@ranjeshj ranjeshj added this to the WinUI 2.6 milestone Jan 26, 2021
@MarcoZehe
Copy link

FWIW: In good old WinForms, a group of radio buttons was a true groupbox container which contained the radio buttons. It had its own name (the label), role (group), and would be picked up by the ancestry heuristics of screen readers when focus shifted into the group. The radio buttons were the logical children of the group in the accessibility tree.

MFC C++ used a different model, there, the label for a group was a button that was a prior sibling of the first of radio buttons, and probably had some certain MSAA flags. Screen readers such as NVDA or JAWS have heuristics to automatically speak that, too.

As for comboboxes, these are different beasts: Their automation name is the label, and the currently selected item is the value. But since each radio button has its own name, the grouping logic seems to be the most sensible approach here.

@chigy chigy added the A11ySev2 Non-blocking for core user tasks or blocking for non-core user tasks. label Apr 6, 2021
@teaP
Copy link
Contributor

teaP commented May 26, 2021

What I found is that a ListView shows up in accessibility like this:

list "HeaderText"
    text "Header Text"
    list item "item 1"
    list item "item 2"
    etc.

Much like the ComboBox situation. So my plan to fix this is the same for RadioButtons, so that the items will be grouped like this:

group "HeaderText"
    text "Header Text"
    radio button "item 1"
    radio button "item 2"
    etc.

Everyone good with that?

@DHowett
Copy link
Member

DHowett commented May 26, 2021

Sounds good to me!

@ghost ghost added the working on it label May 26, 2021
@teaP teaP closed this as completed in #5094 Jun 8, 2021
@ghost ghost removed the working on it label Jun 8, 2021
ghost pushed a commit to microsoft/terminal that referenced this issue Oct 7, 2021
I thought that microsoft/microsoft-ui-xaml#3183 might just fix this for us, but it didn't. We've got our RadioButton's all up in SettingsContainers, so they all think they're `AutomationProperties.AccessibilityView="Raw"` for some reason. If you simply add the `Content` to these, then they all end up correct in Accessibility Insights

## PR Checklist
* [x] Will take care of #11248 but I can't be the one to close it.
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
PankajBhojwani pushed a commit to microsoft/terminal that referenced this issue Oct 15, 2021
I thought that microsoft/microsoft-ui-xaml#3183 might just fix this for us, but it didn't. We've got our RadioButton's all up in SettingsContainers, so they all think they're `AutomationProperties.AccessibilityView="Raw"` for some reason. If you simply add the `Content` to these, then they all end up correct in Accessibility Insights

* [x] Will take care of #11248 but I can't be the one to close it.
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

(cherry picked from commit bc4f410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11yMAS A11yMediumImpact A11ySev2 Non-blocking for core user tasks or blocking for non-core user tasks. accessibility Narrator, keyboarding, UIA, etc area-RadioButtons team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging a pull request may close this issue.