Skip to content

Fix UIA navigation for TabControl#5380

Merged
dreddy-work merged 3 commits intodotnet:mainfrom
SergeySmirnov-Akvelon:Issue-3058_Add_UIA_support_TabControl
Aug 23, 2021
Merged

Fix UIA navigation for TabControl#5380
dreddy-work merged 3 commits intodotnet:mainfrom
SergeySmirnov-Akvelon:Issue-3058_Add_UIA_support_TabControl

Conversation

@SergeySmirnov-Akvelon
Copy link
Contributor

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon commented Aug 2, 2021 •
edited
Loading

Fixes #3058

Related issue: #3421

Proposed changes

  • The issue is reproduced because the old logic sometimes returned incorrect results for "NextSibling" and "PreviousSibling", which led to incorrect shortcuts behavior. To fix the issue, UIA support was added in which we return the correct results
  • Updated "SupportsUiaProviders" flag.
  • Added and implemented accessible objects for the TabControl and TabPage
  • Added unit tests.
  • Fixed issue with Narrator

Customer Impact

Before fix:
Shortcuts in some cases did not work for TabControl

After fix:
3058-expected

Regression?

  • No

Risk

  • Minimal

Test methodology

  • CTI team
  • Unit tests

Accessibility testing

  • Narrator
  • Accessibility Insights
  • Inspect

Test environment(s)

  • Microsoft Windows [Version 10.0.19041.388]
  • .NET Core SDK: 6.0.100-preview.3.21202.5
Microsoft Reviewers: Open in CodeFlow

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon requested a review from a team as a code owner August 2, 2021 10:21
dreddy-work
dreddy-work reviewed Aug 2, 2021
View reviewed changes
@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Aug 2, 2021
@vladimir-krestov vladimir-krestov self-requested a review August 3, 2021 05:59
@vladimir-krestov vladimir-krestov mentioned this pull request Aug 3, 2021
Closed
70 tasks
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 3, 2021
vladimir-krestov
vladimir-krestov reviewed Aug 3, 2021
View reviewed changes
vladimir-krestov
vladimir-krestov reviewed Aug 3, 2021
View reviewed changes
@vladimir-krestov
Copy link
Contributor

vladimir-krestov commented Aug 3, 2021

Fix that:
image

@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Aug 3, 2021
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 4, 2021
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3058_Add_UIA_support_TabControl branch from 148cd65 to 37c9842 Compare August 5, 2021 14:05
@dreddy-work
Copy link
Member

dreddy-work commented Aug 10, 2021

@Tanya-Solyanik , can you take a final look at this?

@RussKie RussKie changed the title Fixing UIA navigation through Accessibility Insights shortcuts Fix UIA navigation for TabControl Aug 16, 2021
RussKie
RussKie previously approved these changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@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.

👍 with few comments

src/System.Windows.Forms/src/System/Windows/Forms/TabPage.TabAccessibleObject.cs Outdated
Copy link
Contributor

@RussKie RussKie Aug 16, 2021

Choose a reason for hiding this comment

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

redundant

@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3058_Add_UIA_support_TabControl branch from 37c9842 to 57e4152 Compare August 16, 2021 09:48
RussKie
RussKie previously approved these changes Aug 16, 2021
View reviewed changes
@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 Aug 16, 2021
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3058_Add_UIA_support_TabControl branch from 57e4152 to e4960e3 Compare August 16, 2021 10:03
Tanya-Solyanik
Tanya-Solyanik reviewed Aug 18, 2021
View reviewed changes
Copy link
Contributor

@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.

@dreddy-work - what is the appropriate label for new public surface.

Looks good, please see my comments though.

@RussKie
Copy link
Contributor

RussKie commented Aug 18, 2021

what is the appropriate label for new public surface.

We're adding overrides here, which is acceptable; and thus we don't treat these as "new API", which require an API Review Board approval, documentations, etc.

@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Aug 18, 2021

@RussKie - Overrides require new documentation topics, are they generated automatically?

Tanya-Solyanik
Tanya-Solyanik reviewed Aug 18, 2021
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/TabControl.cs Outdated
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 18, 2021 •
edited by dreddy-work
Loading

Choose a reason for hiding this comment

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

Please add a test to see if clients are listening.

@RussKie
Copy link
Contributor

RussKie commented Aug 18, 2021

Overrides require new documentation topics, are they generated automatically?

I believe the override docs are generated automatically. @gewarren @carlossanlop is this correct?

@gewarren
Copy link
Contributor

gewarren commented Aug 18, 2021

Overrides require new documentation topics, are they generated automatically?

I believe the override docs are generated automatically. @gewarren @carlossanlop is this correct?

Yes the override methods will appear on the main page for the type, like this: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.metrics.observablecounter-1?view=net-6.0#methods

dreddy-work
dreddy-work reviewed Aug 19, 2021
View reviewed changes
Copy link
Member

@dreddy-work dreddy-work left a comment

Choose a reason for hiding this comment

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

LGTM.

Tanya-Solyanik
Tanya-Solyanik reviewed Aug 20, 2021
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/TabControl.TabControlAccessibleObject.cs Outdated
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 20, 2021

Choose a reason for hiding this comment

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

What happens when we hit the spinner control? Suppose we have more tab headers than fits into the control width, then we show a left/right arrows to navigate between the tab headers. Do we hit-test those arrows correctly?

Copy link
Contributor Author

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon Aug 23, 2021

Choose a reason for hiding this comment

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

I have investigated this question and did not find direct access to the Spinner from the TabControl. We are now using the native Spinner implementation. I have not seen any difference in behavior since .NET Framework 4.7.2, except that now the Spinner is displayed first in the list of control elements, and not second as in .NET Framework 4.7.2. How critical do you think this is?

image

Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 23, 2021

Choose a reason for hiding this comment

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

Let's keep the hierarchy as is and follow up with Ryan. We can address this separately.

src/System.Windows.Forms/src/System/Windows/Forms/TabControl.cs Outdated
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 20, 2021

Choose a reason for hiding this comment

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

The difference between UIA clients listening and AO create APIs is that the former will force creation of AO if AT (or touch driver) is listening but the object had not been created yet. This might be desired if tab is the first object accessed by narrator. Note that touch driver needs AO created to work as well as AT does.

@SergeySmirnov-Akvelon
Fix UIA navigation to move backwards/forwards through AIWin(Accessibi…
499d5bc 
…lity Insights For Windows) keyboard combination Shift+Ctrl+F5/F6/F7/F8/F9 doesn't work with the TabControl dotnet#3058

Updated "SupportsUiaProviders" flag.

Added and implemented accessible objects for the TabControl and TabPage

Added unit tests.

Fixed issue with Narrator
@SergeySmirnov-Akvelon
Fixed code review notes. Refactored unit tests. Added missing unit-te…
19627a0 
…sts.
@SergeySmirnov-Akvelon
Fixed code review notes.
52288b6
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3058_Add_UIA_support_TabControl branch from 8288bbd to 52288b6 Compare August 23, 2021 13:12
Tanya-Solyanik
Tanya-Solyanik approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, please get a review from Vladimir and follow up with Ryan if we have to create a custom provider for the spinner control.

@dreddy-work dreddy-work merged commit 962e1d3 into dotnet:main Aug 23, 2021
@ghost ghost added this to the 7.0 alpha1 milestone Aug 23, 2021
@dreddy-work
Copy link
Member

dreddy-work commented Aug 23, 2021

@vladimir-krestov , I merged this. If you have further feedback, please feel free to update this and @SergeySmirnov-Akvelon will update them in the next PR along with the pending investigation.

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon mentioned this pull request Sep 3, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@dreddy-work dreddy-work dreddy-work left review comments

@vladimir-krestov vladimir-krestov vladimir-krestov left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik approved these changes

@RussKie RussKie RussKie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon

Labels

waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s)

Projects

None yet

Milestone

7.0 Preview1

Development

Successfully merging this pull request may close these issues.

UIA navigation doesn't work with the TabControl

6 participants

@SergeySmirnov-Akvelon @vladimir-krestov @dreddy-work @RussKie @Tanya-Solyanik @gewarren