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.

Sign up for GitHub

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

Jump to bottom

Update ProgressRing to IsTabStop=True #6060

Closed
wants to merge 1 commit into from
Closed

Update ProgressRing to IsTabStop=True #6060

wants to merge 1 commit into from

Conversation

karkarl
Copy link
Contributor

@karkarl karkarl commented Oct 7, 2021

Description

Update ProgressRing to IsTabStop=True so that users can focus on it using Narrator

Motivation and Context

Fixes #3615

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Oct 7, 2021
marcelwgn
marcelwgn reviewed Oct 7, 2021
View reviewed changes
Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

I think this is based on a different branch, the changes to TreeView seem a bit unrelated to me.

@karkarl karkarl force-pushed the user/karenlai/ProgressRingIsTabStop branch from a86595d to e039e2f Compare October 7, 2021 21:18
@karkarl
Copy link
Contributor Author

karkarl commented Oct 7, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karkarl
Copy link
Contributor Author

karkarl commented Oct 7, 2021

I think this is based on a different branch, the changes to TreeView seem a bit unrelated to me.

already fixed :)

marcelwgn
marcelwgn reviewed Oct 7, 2021
View reviewed changes
Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

I just noticed something, how would this play together with the changes made in #5930? Do we need to set IsTabStop to false too then when the ProgressRing is not active?

@karkarl
Copy link
Contributor Author

karkarl commented Oct 7, 2021

I just noticed something, how would this play together with the changes made in #5930? Do we need to set IsTabStop to false too then when the ProgressRing is not active?

I tested it manually. Since the visibility collapses when not Active, it does not need IsTabStop to be set to false.

@marcelwgn
Copy link
Contributor

I just noticed something, how would this play together with the changes made in #5930? Do we need to set IsTabStop to false too then when the ProgressRing is not active?

I tested it manually. Since the visibility collapses when not Active, it does not need IsTabStop to be set to false.

Is this actually the case? In #5930 we only removed the control from the UIA tree, we did not set the visibility to collapsed and I think we also did not do that before the PR (only set the opacity to 0 looking at the source).

@ranjeshj ranjeshj added area-Progress ProgressBar, ProgressRing team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Oct 7, 2021
@karkarl
Copy link
Contributor Author

karkarl commented Oct 7, 2021

I just noticed something, how would this play together with the changes made in #5930? Do we need to set IsTabStop to false too then when the ProgressRing is not active?

I tested it manually. Since the visibility collapses when not Active, it does not need IsTabStop to be set to false.

Is this actually the case? In #5930 we only removed the control from the UIA tree, we did not set the visibility to collapsed and I think we also did not do that before the PR (only set the opacity to 0 looking at the source).

Oops you're right. ProgressRing does not toggle Visibility but Opacity. However, tested manually, your change in updating AccessibilityView=Raw works well even with this change.

@marcelwgn
Copy link
Contributor

I just noticed something, how would this play together with the changes made in #5930? Do we need to set IsTabStop to false too then when the ProgressRing is not active?

I tested it manually. Since the visibility collapses when not Active, it does not need IsTabStop to be set to false.

Is this actually the case? In #5930 we only removed the control from the UIA tree, we did not set the visibility to collapsed and I think we also did not do that before the PR (only set the opacity to 0 looking at the source).

Oops you're right. ProgressRing does not toggle Visibility but Opacity. However, tested manually, your change in updating AccessibilityView=Raw works well even with this change.

Oh missed that you already tested this😅 in that case, awesome!

StephenLPeters
StephenLPeters reviewed Oct 25, 2021
View reviewed changes
Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

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

I don't think this is the issue that the bug is talking about, and I don't think we want progress rings to be focusable.

@karkarl
Copy link
Contributor Author

karkarl commented Oct 25, 2021

I don't think this is the issue that the bug is talking about, and I don't think we want progress rings to be focusable.

Oops, yup. Closing.

@karkarl karkarl closed this Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephenLPeters StephenLPeters StephenLPeters left review comments

@marcelwgn marcelwgn marcelwgn approved these changes

Assignees
No one assigned
Labels
area-Progress ProgressBar, ProgressRing team-Controls Issue for the Controls team
Projects
None yet
Milestone
No milestone
4 participants
@karkarl @marcelwgn @StephenLPeters @ranjeshj