Skip to content

[a11y] Announce new tab/pane upon creation - #13575

Closed
Carlos Zamora (carlos-zamora) wants to merge 5 commits into
mainfrom
dev/cazamor/a11y-sev3/new-profile-announcement
Closed

Carlos Zamora (carlos-zamora) wants to merge 5 commits into
mainfrom
dev/cazamor/a11y-sev3/new-profile-announcement

Conversation

@carlos-zamora

Copy link
Copy Markdown

Summary of the Pull Request

Makes the automation peer for the TerminalPage announce that a new tab/pane was created successfully. In order to do this, we need to store the automation peer for TerminalPage. Then we have easy access to dispatch notifications from it.

Closes #12038

Validation Steps Performed

Open a new tab/pane with Narrator active.

@ghost ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jul 22, 2022
@github-actions

This comment has been minimized.

@carlos-zamora

Copy link
Copy Markdown
Author
  • I could add a similar notification for when the settings tab is opened. I wasn't sure if that was desired though?
  • I originally tried having this be a part of TerminalTab and Pane, but for some reason the TabViewItem's automation peer would be ignored when it dispatched a notification event.

@carlos-zamora

Copy link
Copy Markdown
Author

oops, doesn't work for duplicate tab. Should be easy to fix. I should check duplicate pane too.

@github-actions

This comment has been minimized.

@carlos-zamora
Carlos Zamora (carlos-zamora) force-pushed the dev/cazamor/a11y-sev3/new-profile-announcement branch from 38ac9c5 to 0473430 Compare July 25, 2022 17:31
@carlos-zamora Carlos Zamora (carlos-zamora) added the Needs-Second It's a PR that needs another sign-off label Jul 25, 2022
@carlos-zamora

Copy link
Copy Markdown
Author

@msftbot merge this in 10 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 26, 2022
@ghost

This comment was marked as outdated.

Comment on lines +750 to +754
<value>A new tab for the "{}" profile was created successfully</value>
<comment>{} will be replaced with the name of the profile (i.e. PowerShell, Command Prompt, etc.). The name may span multiple words. This announcement is read by a screen reader.</comment>
</data>
<data name="SplitPaneAnnouncement" xml:space="preserve">
<value>A new pane for the "{}" profile was created successfully</value>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the wording here throws me a bit (feels clunky).. gut check how it sounds?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

A11y team requested: "profile name created successfully"

We've diverged a bit since then haha. I find value in explicitly saying "pane" or "tab". I also think saying "profile" is important because some profile names could be weird. That said, what if we change it to...
"{}" pane/tab created successfully

It's more succinct? But there's still some doubts when it comes to localization. Thoughts?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 26, 2022
@DHowett Dustin L. Howett (DHowett) removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 26, 2022
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 27, 2022
@carlos-zamora Carlos Zamora (carlos-zamora) removed the Needs-Second It's a PR that needs another sign-off label Jul 27, 2022
{
page->_processingCommandlineArgs = true;
auto finishProcessing = wil::scope_exit([&page]() {
page->_processingCommandlineArgs = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.... Can't you just use _startupState <= StartupState::Initialized instead of introducing a new member?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No because we're trying to suppress notifications when processing commandline args regardless of initialization step. So your proposal works for wt.exe args from Run, but not from commandline mode in the command palette.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, i am of two minds. I don't know if I would be surprised if they were suppressed if I just typed in the command that made it happen.

@codeofdusk

Copy link
Copy Markdown
Contributor

NVDA will always filter out notifications from terminal (see nvaccess/nvda#13261). Are these being sent by the TermCtrl provider?

@carlos-zamora

Copy link
Copy Markdown
Author

NVDA will always filter out notifications from terminal (see nvaccess/nvda#13261). Are these being sent by the TermCtrl provider?

These aren't sent by TermCtrl provider. They're sent by a provider for the whole window, which manages the tabs and UI outside/above the terminal area itself.

Tested this with NVDA and NVDA does read out these notifications, probably for the reason above.

{
page->_processingCommandlineArgs = true;
auto finishProcessing = wil::scope_exit([&page]() {
page->_processingCommandlineArgs = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, i am of two minds. I don't know if I would be surprised if they were suppressed if I just typed in the command that made it happen.

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 25, 2022
@zadjii-msft Mike Griese (zadjii-msft) removed this from the Terminal v1.17 milestone Jul 5, 2023
microsoft-github-policy-service Bot pushed a commit that referenced this pull request Aug 25, 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
Dustin L. Howett (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
@DHowett

Copy link
Copy Markdown
Member

Carlos Zamora (@carlos-zamora) Alright, what should I do about this, Carlos?

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 Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Settings> Add new profile]: Confirmation message is not announced by Screen Reader when user create a new profile and duplicate the profile.

5 participants