Skip to content

[Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator#2074

Merged
RussKie merged 1 commit intorelease/3.1from
dev/v-vlkres/NVDAToolTipError
Oct 30, 2019
Merged

[Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator#2074
RussKie merged 1 commit intorelease/3.1from
dev/v-vlkres/NVDAToolTipError

Conversation

@vladimir-krestov
Copy link
Contributor

@vladimir-krestov vladimir-krestov commented Oct 11, 2019 •
edited
Loading

Fixes #2073
Original bug: 987952

Proposed changes

  • Add AnnounceText method to raise UIA notification (ScreenReader says notification text) when a tooltip is shown

- Add IToolTip interface and implement it to unite elements (Control, ToolStripItem, DataGridViewElement) which can have a tooltip (this is necessary to the correct, simple and short implementation of raising UIA notification.)

ScreenReader reads the tooltip text of elements of in turn.

Customer Impact

  • A user can hear a text of a shown tooltip

Regression?

  • No

Risk

  • No

Screenshots

Before

image

After

  • A screen reader announces a text with a title of all tooltips (mouse and keyboard). 📗

image

2

Test methodology

  • Manual UI testing

Accessibility testing

  • Using NVDA and Narrator

Test environment(s)

  • .NET Core Version: 3.1.0-preview1.19458.7
  • Microsoft Windows [Version 10.0.18362.356]
Microsoft Reviewers: Open in CodeFlow

@vladimir-krestov vladimir-krestov requested a review from a team as a code owner October 11, 2019 14:24
@vladimir-krestov vladimir-krestov self-assigned this Oct 11, 2019
@codecov
Copy link

codecov bot commented Oct 11, 2019 •
edited
Loading

Codecov Report

Merging #2074 into release/3.1 will increase coverage by 0.04674%.
The diff coverage is 0%.

@@                  Coverage Diff                  @@
##           release/3.1       #2074         +/-   ##
=====================================================
+ Coverage      26.4715%   26.51825%   +0.04675%     
=====================================================
  Files              806         806                 
  Lines           268111      268121         +10     
  Branches         38071       38073          +2     
=====================================================
+ Hits             70973       71101        +128     
+ Misses          192056      191941        -115     
+ Partials          5082        5079          -3
Flag Coverage Δ
#Debug 26.51825% <0%> (+0.04674%) ⬆️
#production 26.51825% <0%> (+0.04674%) ⬆️
#test 100% <ø> (?)

@merriemcgaw
Copy link
Member

merriemcgaw commented Oct 11, 2019

Business case approved for 3.1 from the feature team side.

@merriemcgaw
Copy link
Member

merriemcgaw commented Oct 11, 2019

@vladimir-krestov has the test team verified this fix already? Are you ready for review now?

@RussKie RussKie added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 13, 2019
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 14, 2019

Not tested yet.

RussKie
RussKie reviewed Oct 14, 2019
View reviewed changes
@vladimir-krestov vladimir-krestov changed the title [Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator WIP: [Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator Oct 14, 2019
RussKie
RussKie previously approved these changes Oct 14, 2019
View reviewed changes
@vladimir-krestov vladimir-krestov force-pushed the dev/v-vlkres/NVDAToolTipError branch from ffb67ef to 2dd7fe1 Compare October 15, 2019 15:32
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 15, 2019

Changed the implementation.
Fixed test issues.
Waiting for retesting.

@vladimir-krestov vladimir-krestov mentioned this pull request Oct 16, 2019
Merged
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 16, 2019

The fix has some test issues.
Fixing of test issues is blocked until PR #2096 is resolved.

@vladimir-krestov vladimir-krestov added 🚧 work in progress Work that is current in progress and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Oct 16, 2019
@vladimir-krestov vladimir-krestov changed the title WIP: [Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator WIP: [Blocked][Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator Oct 17, 2019
zsd4yr
zsd4yr reviewed Oct 18, 2019
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/ToolTip.cs Outdated
/// </summary>
private void AnnounceText(Control tool, string text)
{
tool?.AccessibilityObject.RaiseAutomationNotification(
Copy link
Contributor

@zsd4yr zsd4yr Oct 18, 2019

Choose a reason for hiding this comment

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

Should you use the elvis operator ? after AccessibilityObject as well?

Copy link
Contributor

@zsd4yr zsd4yr Oct 18, 2019

Choose a reason for hiding this comment

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

Any need to validate Automation is not null?

Copy link
Contributor Author

@vladimir-krestov vladimir-krestov Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. Sometimes AccessibleObject property returns null. I need to handle it.

Copy link
Contributor Author

@vladimir-krestov vladimir-krestov Oct 24, 2019

Choose a reason for hiding this comment

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

Thanks, fixed.

@vladimir-krestov vladimir-krestov changed the title WIP: [Blocked][Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator WIP: [Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator Oct 23, 2019
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 23, 2019

#2096 is merged, so I can to continue work on this PR.

@vladimir-krestov vladimir-krestov added waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) and removed 🚧 work in progress Work that is current in progress labels Oct 23, 2019
@vladimir-krestov vladimir-krestov force-pushed the dev/v-vlkres/NVDAToolTipError branch from 2dd7fe1 to 3914bb7 Compare October 24, 2019 09:20
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 24, 2019

Testers approved this fix. ✔️

@vladimir-krestov vladimir-krestov added 📫 waiting-approval and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Oct 24, 2019
@vladimir-krestov vladimir-krestov changed the title WIP: [Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator [Accessibility][ScreenReader]: Fixing tooltip text announcing by NVDA/Narrator Oct 24, 2019
@leecow leecow added servicing-approved .NET Shiproom approved the PR for merge and removed Ask-Mode labels Oct 29, 2019
@RussKie RussKie merged commit ce9851d into release/3.1 Oct 30, 2019
@RussKie RussKie deleted the dev/v-vlkres/NVDAToolTipError branch October 30, 2019 00:00
@RussKie RussKie added this to the 3.1 milestone Oct 30, 2019
@vladimir-krestov vladimir-krestov mentioned this pull request Dec 20, 2019
Closed
@WPMGPRoSToTeMa WPMGPRoSToTeMa mentioned this pull request Jan 11, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@zsd4yr zsd4yr zsd4yr left review comments

@RussKie RussKie RussKie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vladimir-krestov vladimir-krestov

Labels

servicing-approved .NET Shiproom approved the PR for merge

Projects

None yet

Milestone

3.1

Development

Successfully merging this pull request may close these issues.

5 participants

@vladimir-krestov @merriemcgaw @RussKie @zsd4yr @leecow