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

Make screen reader announce successful MovePane and MoveTab actions #15771

Merged
merged 5 commits into from
Aug 25, 2023

Conversation

carlos-zamora
Copy link
Member

Uses the RaiseNotificationEvent() API from UIA automation peers to announce successful MovePane and MoveTab actions. The announcements are localized in the resw file.

Closes #15159
Based on #13575

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. labels Jul 27, 2023
lhecker
lhecker previously approved these changes Jul 27, 2023
@DHowett
Copy link
Member

DHowett commented Jul 27, 2023

This complies with the letter of the requirement, but does it comply with the spirit of the requirement? What does it mean for it to have been "moved successfully"?

@carlos-zamora
Copy link
Member Author

This complies with the letter of the requirement, but does it comply with the spirit of the requirement? What does it mean for it to have been "moved successfully"?

Yeah, the associated bug isn't that descriptive. And I have trouble finding other apps that do this too (VS Code doesn't do it, and VS is notoriously noisy so I didn't bother to check), ugh.

All the associated bug says (even in the video) is that we need to say that we've moved successfully. So, it seems like the requirement is just to tell the user that the action was executed successfully.

@codeofdusk is there anything specific you'd expect the screen reader to say if you moved a tab or pane to another window?

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Aug 22, 2023

The announcements now include a bunch more info. I tried building/deploying this commit, but I got a weird error down in the AcvtivationFactory that didn't really explain anything at runtime. I'll try building/deploying on my machine at home and I'll look into this more tomorrow. EDIT: verified it builds. I wanna verify all of these work and feel good but I can do that on my own.

For now, this should be in a good reviewable state though :)

@carlos-zamora carlos-zamora dismissed lhecker’s stale review August 22, 2023 21:11

Significant changes

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I'm not allergic to the OnCreateAutomationPeer change, but it scares me a little bit!

src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 25, 2023
</data>
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow" xml:space="preserve">
<value>Active pane moved to "{0}" tab in "{1}" window</value>
<comment>{Locked="{0}"}{Locked="{1}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the tab the pane was moved to. {1} is the name of the window the pane was moved to.</comment>
Copy link
Member

Choose a reason for hiding this comment

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

It seems you can also write {Locked="{0}","{1}"}

Comment on lines +2097 to +2103
if (auto autoPeer = Automation::Peers::FrameworkElementAutomationPeer::FromElement(*this))
{
autoPeer.RaiseNotificationEvent(Automation::Peers::AutomationNotificationKind::ActionCompleted,
Automation::Peers::AutomationNotificationProcessing::ImportantMostRecent,
RS_(L"TerminalPage_PaneMovedAnnouncement_NewTab"),
L"TerminalPageMovePaneToNewTab" /* unique name for this notification category */);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to construct the peer every time? Is it just a getter and cheap to call? Should we cache it? (I think this could be figured out by checking if the pointer that's returned is always the same. In that case it's probably a cheap getter.)

You could consider abstracting the code away like so:

void TerminalPage::_raiseActionCompletedEventforUIA(const std::wstring_view& activityId, const std::wstring_view& displayString) {
    using namespace ::winrt::Windows::UI::Xaml::Automation::Peers;
    if (auto autoPeer = FrameworkElementAutomationPeer::FromElement(*this))
    {
        autoPeer.RaiseNotificationEvent(
            AutomationNotificationKind::ActionCompleted,
            AutomationNotificationProcessing::ImportantMostRecent,
            displayString,
            activityId);
    }
}

So that the remaining code becomes:

_raiseActionCompletedEventforUIA(L"TerminalPageMovePaneToNewTab", RS_(L"TerminalPage_PaneMovedAnnouncement_NewTab"));

(etc.)

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this shouldn't construct but get the one from XAML's internal cache

@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 2fb4a7f into main Aug 25, 2023
15 of 17 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the dev/cazamor/a11y/announce-move branch August 25, 2023 16:38
DHowett pushed a commit that referenced this pull request Sep 22, 2023
…15771)

Uses the `RaiseNotificationEvent()` API from UIA automation peers to
announce successful `MovePane` and `MoveTab` actions. The announcements
are localized in the resw file.

Closes #15159
Based on #13575

(cherry picked from commit 2fb4a7f)
Service-Card-Id: 90438494
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. zBugBash-Consider
Projects
4 participants