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

Fix issue with announcing ExpandedCollapsedState for ComboBox in Property grid #1987 #4059

Closed
wants to merge 1 commit into from
Closed

Fix issue with announcing ExpandedCollapsedState for ComboBox in Property grid #1987 #4059

wants to merge 1 commit into from

Conversation

SergeySmirnov-Akvelon
Copy link
Contributor

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon commented Oct 6, 2020 •
edited
Loading

Fixes #1987

Proposed changes

  • removed RaiseAutomationEvent and RaiseAutomationPropertyChangedEvent and replaced them on a RaiseAutomationNotification

Customer Impact

Due to two competing requests to the Narrator in half of the cases the narrator announces the selected option, and in the other half the ExpandedCollapsedState of the control:
1987-issue
After fixing, the focus always remains on the selected item and Narrator announces selected option and ExpandedCollapsedState of control:
1987-fixed

Regression?

  • No

Risk

  • Minimal

Test methodology

  • Manually

Test environment(s)

  • Microsoft Windows [Version 10.0.19041.388]
  • .Net SDK 5.0.100-rc.1.20420.14
Microsoft Reviewers: Open in CodeFlow

@SergeySmirnov-Akvelon
Fix issue with announcing ExpandedCollapsedState for ComboBox in Prop…
0d20a62 
…erty grid dotnet#1987

Issue is reproduced due to two competing requests to the Narrator. First request moves focus on selected item in options list, second request moves focus on control itself. As a result, in half of the cases the narrator announces the selected option, and in the other half the ExpandedCollapsedState of the control.

As a fix, I removed RaiseAutomationEvent and RaiseAutomationPropertyChangedEvent and replaced them on a RaiseAutomationNotification. Now, the focus always remains on the selected item and Narrator announces selected option and ExpandedCollapsedState of control.
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon requested a review from a team as a code owner October 6, 2020 12:21
@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 6, 2020
RussKie
RussKie reviewed Oct 6, 2020
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍

Tanya-Solyanik
Tanya-Solyanik reviewed Oct 8, 2020
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/PropertyGridInternal/PropertyGridView.cs
@@ -1774,11 +1774,10 @@ public virtual int GetValueWidth()
var gridEntry = GetGridEntryFromRow(selectedRow);
if (gridEntry != null)
{
gridEntry.AccessibilityObject.RaiseAutomationEvent(UiaCore.UIA.AutomationFocusChangedEventId);
gridEntry.AccessibilityObject.RaiseAutomationPropertyChangedEvent(
Copy link
Member

Choose a reason for hiding this comment

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

Once RaiseAutomationPropertyChangedEvent is removed, does Inspect show the change of state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tanya-Solyanik , I checked that the presence or absence of a "RaiseAutomationPropertyChangedEvent" call does not affect the behavior of the Inspector

Tanya-Solyanik
Tanya-Solyanik previously approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

Issue is reproduced due to two competing requests to the Narrator.

If competing requests are send in response to focus change, who is changing focus? Isn't focus change the root cause?

@Tanya-Solyanik Tanya-Solyanik self-requested a review October 8, 2020 03:51
@Tanya-Solyanik Tanya-Solyanik dismissed their stale review October 8, 2020 03:53

Clicked a wrong button

@SergeySmirnov-Akvelon
Copy link
Contributor Author

@Tanya-Solyanik , we have two calls of RaiseAutomationEvent(UiaCore.UIA.AutomationFocusChangedEventId) method. First time this method is called inside ListBox when ListBox tries to set focus to selected option. Second time it is called in PropertyGridView when we move focus to control and call RaiseAutomationPropertyChangedEvent method. As result these RaiseAutomationEvent methods compete with each other, leading to unstable behavior of Narrator. In some cases the focus moves to the selected element in the ListBox and we announce it, not the ExpandedCollapsedState of the ListBox. In other cases, we move focus to ListBox and announce the ExpandedCollapsedState of it, but do not announce the selected option. The behavior is considered correct when we move focus to the selected option, announce this option and announce ExpandCollapsedState

@Tanya-Solyanik
Copy link
Member

Second time it is called in PropertyGridView when we move focus to control and call RaiseAutomationPropertyChangedEvent method.

Is this the one you removed? I don't see SetFocus calls around the changed code.

@SergeySmirnov-Akvelon
Copy link
Contributor Author

The RaiseAutomationEvent(UiaCore.UIA.AutomationFocusChangedEventId) method affects the Narrator's focus. For example on next gif last combobox has additional RaiseAutomationEvent on selected item. We see that narrator focuses on selected item in the this combobox, but in the penultimate Narrator focuses on editable part of combobox:
focusbyraiseautomationevent

Inspector is also affected by this method. For example on next gif we see that focus moves to first textbox, but focus of Inspector switches to second textbox, because we call the 'RaiseAutomationEvent' for it.
focusbyraiseautomationevent-textbox

@RussKie RussKie removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 11, 2020
@RussKie
Copy link
Member

RussKie commented Nov 11, 2020

It was decided not to take the fix.

@RussKie RussKie closed this Nov 11, 2020
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon deleted the Issue-1987_Fix_issue_with_announcing_ExpandedCollapsedState_by_Narrator_ branch January 18, 2021 06:29
@ghost ghost locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RussKie RussKie RussKie left review comments

@Tanya-Solyanik Tanya-Solyanik Awaiting requested review from Tanya-Solyanik

Assignees

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Screen reader does not announce expanding and collapsing action for Open button in PropertyGrid ComboBox
3 participants
@SergeySmirnov-Akvelon @Tanya-Solyanik @RussKie