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

Adding UIA support for WebBrowser #7314

Merged

Conversation

dkazennov
Copy link
Contributor

@dkazennov dkazennov commented Jun 16, 2022

Partially implements #3421
For .Net 7

Proposed changes

  • Updated "SupportsUiaProviders" flag to WebBrowser
  • Added and implemented WebBrowser.WebBrowserAccessibleObject
  • Added unit tests
  • Testing application
    WebBrowserWinFormsApp.zip

Customer Impact

Screenshots

Before

before

ProviderDescription: "[pid:22832,providerId:0x402AA Main:Nested [pid:27400,providerId:0x402AA Main(parent link):Microsoft: MSAA Proxy (unmanaged:UIAutomationCore.dll)]; Hwnd(parent link):Microsoft: HWND Proxy (unmanaged:uiautomationcore.dll)]"

After

after

ProviderDescription: "[pid:25792,providerId:0x70BA2 Main:Nested [pid:30696,providerId:0x70BA2 Main(parent link):Unidentified Provider (unmanaged:coreclr.dll)]; Hwnd(parent link):Microsoft: HWND Proxy (unmanaged:uiautomationcore.dll)]"

Regression?

  • No

Risk

  • Minimal

Test methodology

  • Unit tests
  • Manual with custom application

Accessibility testing

  • Inspect
  • Accessibility Insights

Test environment

dotnet : 7.0.0-preview.4.22229.4

Microsoft Reviewers: Open in CodeFlow

@dkazennov dkazennov requested a review from a team as a code owner June 16, 2022 14:06
@ghost ghost assigned dkazennov Jun 16, 2022
@dkazennov dkazennov added the tenet-accessibility MAS violation, UIA issue; problems with accessibility standards label Jun 16, 2022
@dkazennov dkazennov force-pushed the Issue_3421_WebBrowser_ShouldSupport_UIAProvider branch from 0f28ee9 to 15873a3 Compare June 16, 2022 14:46
@RussKie
Copy link
Member

RussKie commented Jun 17, 2022

To be honest I don't think we should be spending any time updating any of WebBrowser API.

  1. IE is officially retired and out of support as of June 15, 2022.
  2. We're planning to deprecated all related API (see Mark WebBrowser and all related (e.g., HTML* types) as obsolete #6964).

/cc: @merriemcgaw @Tanya-Solyanik

@dkazennov
Copy link
Contributor Author

dkazennov commented Jun 17, 2022

Fixed tests: ControlAccessibleObject_GetPropertyValue_AccessKey_ReturnExpected
added typeof(WebBrowser) to s_controlsIgnoringTextChangesForTests (since WebBrowserBase has public override string Text property which throws exception in setter).

@dkazennov dkazennov force-pushed the Issue_3421_WebBrowser_ShouldSupport_UIAProvider branch from 15873a3 to fc1ea12 Compare June 17, 2022 07:36
@dkazennov
Copy link
Contributor Author

  1. WebBrowser class implements UIA provider now. New class WebBrowserAccessibleObject passes unit tests.
  2. Custom application works with WebBrowser control (both original and changed) as intended.
  3. New unit tests work as intended.

I believe that everything works now. So can we keep this fix as it is already complete?

Copy link
Contributor

@vladimir-krestov vladimir-krestov 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 see risk of implementing this fast fix, even if IE is outdated.
The main reason - #3037. We need to provide a correct accessibility tree for implemented apps, that can use WebBrowser.
@merriemcgaw, @Tanya-Solyanik, your thoughts?

internal override object? GetPropertyValue(UiaCore.UIA propertyID)
=> propertyID switch
{
UiaCore.UIA.AutomationIdPropertyId
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove, it will be implemented in #7206

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please remove, it will be implemented in #7206

In #7206 it will be UiaCore.UIA.AutomationIdPropertyId => AutomationId instead. Returning Owner.Name is pre-7206.
Or should we change it pre-emptively?

Copy link
Member

Choose a reason for hiding this comment

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

If we're close I don't mind making sure apps with the WebBrowser control have the right UIA tree. But I wouldn't go spending time implementing too many patterns on this particular control.

Copy link
Contributor Author

@dkazennov dkazennov Jun 22, 2022

Choose a reason for hiding this comment

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

Please remove, it will be implemented in #7206

Since #7206 is not yet implemented, I propose we don't change this method until #7206 is implemented (or if this issue is implemented first we will change WebBrowserAccessibleObject in #7206).

Copy link
Member

Choose a reason for hiding this comment

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

Just a friendly reminder to not lose this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a friendly reminder to not lose this

Thank you! It will be done now )

@@ -1130,6 +1132,8 @@ protected override WebBrowserSiteBase CreateWebBrowserSiteBase()
return new WebBrowserSite(this);
}

protected override AccessibleObject CreateAccessibilityInstance() => new WebBrowserAccessibleObject(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

UIAProvider support for WebBrowser control and UnitTests
@dkazennov dkazennov force-pushed the Issue_3421_WebBrowser_ShouldSupport_UIAProvider branch from fc1ea12 to 686642b Compare June 20, 2022 08:53
@RussKie
Copy link
Member

RussKie commented Jun 22, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

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.

LGTM

@dkazennov
Copy link
Contributor Author

CTI requested

@dkazennov dkazennov 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 Jun 23, 2022
@Olina-Zhang
Copy link
Member

Tested this PR with no new issue found.

@Olina-Zhang Olina-Zhang 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 Jun 24, 2022
@RussKie RussKie merged commit 250c0e1 into dotnet:main Jun 28, 2022
@ghost ghost added this to the 7.0 Preview7 milestone Jun 28, 2022
@Philip-Wang01
Copy link
Contributor

Verified on latest .NET core 7.0 build: .NET 7.0.100-preview.7.22330.8, issue is fixed. Screenshot as below:
image

@Cassie-Li01
Copy link

Verified on .NET 7.0.100-preview.7.22370.3 TP build, issue is fixed, screenshot as below:
image

@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tenet-accessibility MAS violation, UIA issue; problems with accessibility standards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants