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

Removing separators from SizeOfSet and PositionInSet counts #2090

Merged

Conversation

arpitmathur
Copy link
Contributor

@arpitmathur arpitmathur commented Oct 22, 2019

Fixes #1467

5.0 PR: #1977

Description

Screen readers were reading incorrect values of the number of menu items and their position since we were counting separators as MenuItems in the automation tree.

I filtered for separators in the current MenuItems to fix the SizeOfSet count.

I filtered for separators again for PositionInSet but added a short-circuit to break when you're at the element whose position you're trying to find. This short-circuit was added to not remove separators below the relevant MenuItem from the PositionInSet count.

Customer Impact

Narrator and other screen readers read the wrong values of SizeOfSet and PositioninSet for each individual menu item. This is confusing for users with visual disabilities as they can not accurately tell what is being visually represented on the screen.

Regression

No. This was discovered during accessibility testing on .Net Core 3.0

Risk - Low

The fix is well-contained and has been tested well internally.

We're iterating over the menu items another time in a place where we're already iterating. Running another iteration through an enumerable does invalidate some caching but there doesn't seem to be an obviously better way to make this change. We rejected an approach storing the number of separators in a variable as the memory cost would always be paid and be higher than the time cost of another iteration.

@arpitmathur arpitmathur added Bug Product bug (most likely) * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) accessibility mas An issue which is a MAS (Microsoft Accessibility Standards) certification requirement ask-mode labels Oct 22, 2019
@arpitmathur arpitmathur added this to the 3.1 milestone Oct 22, 2019
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Oct 22, 2019
{
if (item == owner)
{
break;
Copy link
Member

Choose a reason for hiding this comment

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

Is it continue ?

Copy link

@weltkante weltkante Oct 24, 2019

Choose a reason for hiding this comment

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

no, you don't want to skip the owner, you want to stop at the owner (what you actually want to do is visit all items before the owner, so seeing the owner is your signal to end the loop). break is correct.

@arpitmathur arpitmathur removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 29, 2019
@arpitmathur arpitmathur merged commit be31d18 into dotnet:release/3.1 Oct 29, 2019
@arpitmathur arpitmathur deleted the dev/arpit/menuautomationpeer branch October 29, 2019 18:24
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility mas An issue which is a MAS (Microsoft Accessibility Standards) certification requirement Bug Product bug (most likely) PR metadata: Label to tag PRs, to facilitate with triage Servicing-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants