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

Introduce PseudoConsoleWindow a11y provider #14541

Merged

5 commits merged into main from dev/cazamor/a11y/pseudo-console-window

Jan 12, 2023

Member

carlos-zamora commented Dec 13, 2022 •

edited by DHowett

Loading

In order to modify the accessibility information for the PseudoConsoleWindow, it needs to have a UIA provider registered. This PR introduces PseudoConsoleWindowAccessibilityProvider and registers it as a UIA provider appropriately. The registration process is based on that of the WindowUiaProvider for ConHost.

Closes #14385

Validation Steps Performed

Run Accessibility Insights FastPass on the window. The PseudoConsoleWindow no longer is tagged as missing a name.


          Introduce PseudoConsoleWindow a11y provider

25bc6fa

ghost added Area-Accessibility Issue-Bug Product-Terminal labels

DHowett requested changes

View reviewed changes

Member

DHowett left a comment

It would be very much better if this window could be yeeted out of the a11y tree, because it contains no user-useful information and should not under any circumstances be user-accessible.

This PR fixes the symptom, "the window that nobody should access has no name," rather than the cause -- is it possible to fix the cause?

ghost added the Needs-Author-Feedback label

DHowett reviewed

View reviewed changes

src/interactivity/base/InteractivityFactory.cpp Outdated Show resolved Hide resolved

zadjii-msft previously requested changes

View reviewed changes

Member

zadjii-msft left a comment

Yea hard block. The conpty window, by all accounts, should not be something the user EVER interacts with.

carlos-zamora commented

View reviewed changes

src/interactivity/base/InteractivityFactory.cpp Outdated

Comment on lines 76 to 85

+                  else if (propertyId == UIA_IsControlElementPropertyId)
+                  {
+                      pVariant->vt = VT_BOOL;
+                      pVariant->boolVal = VARIANT_FALSE;
+                  }
+                  else if (propertyId == UIA_IsContentElementPropertyId)
+                  {
+                      pVariant->vt = VT_BOOL;
+                      pVariant->boolVal = VARIANT_FALSE;
+                  }

Member Author

carlos-zamora Dec 13, 2022

@DHowett @zadjii-msft
So, in XAML, you can set the AccessibilityView to Raw to remove it from the UIA tree. This is actually an abstraction:

AccessibilityView	IsControlElement	IsContentElement
Raw	false	false
Control	true	false
Content	true	true

Source: https://learn.microsoft.com/en-us/windows/win32/winauto/uiauto-treeoverview

By setting both of these to false, we're essentially "removing" this from the UIA tree. However, you can't really remove it. It's just accessible if you want the raw version of the tree.

This distinction is particularly important for Accessibility Insights. Unfortunately, the FastPass tool navigates through the UIA tree and looks for common errors. So, it still finds this PseudoConsoleWindow and complains that it needs a name (unfortunately).

To make matters worse, even with this fix where we change it to Raw and give it a name, we're still getting a bug from FastPass saying that we should set IsControlElement to true. But for the reasons you've stated, we obviously don't want that.

Idk if there's a way to exclude this window from FastPass. Like we said, it's not focusable. We basically want to completely omit this from the UIA tree, but this is as close as we can get, it seems.

Member

DHowett Dec 13, 2022

Got it! Thank you!

ghost removed the Needs-Author-Feedback label

carlos-zamora mentioned this pull request

[Windows Powershell/Command Prompt/Azure CloudShell]: The Name property of a focusable element present on the homepage,must not be null. #14385

Closed


          reduce diff size

767cabf

DHowett requested changes

View reviewed changes

src/interactivity/base/InteractivityFactory.cpp Outdated Show resolved Hide resolved

src/interactivity/base/InteractivityFactory.cpp Outdated

+              HRESULT PseudoConsoleWindowAccessibilityProvider::RuntimeClassInitialize(HWND pseudoConsoleHwnd) noexcept
+              {
+                  RETURN_HR_IF_NULL(E_INVALIDARG, pseudoConsoleHwnd);
+                  _pseudoConsoleHwnd = pseudoConsoleHwnd;

Member

DHowett Dec 13, 2022

is there any compelling need for the PCWAP to keep a handle to its owning window?

Member Author

carlos-zamora Dec 14, 2022

Just this:

terminal/src/interactivity/base/InteractivityFactory.cpp

Lines 107 to 112 in 767cabf

    
           IFACEMETHODIMP PseudoConsoleWindowAccessibilityProvider::get_HostRawElementProvider(_COM_Outptr_result_maybenull_ IRawElementProviderSimple** ppProvider) 
        
           { 
        
               RETURN_HR_IF_NULL(E_INVALIDARG, ppProvider); 
        
               RETURN_HR_IF_NULL(gsl::narrow_cast<HRESULT>(UIA_E_ELEMENTNOTAVAILABLE), _pseudoConsoleHwnd); 
        
               return UiaHostProviderFromHwnd(_pseudoConsoleHwnd, ppProvider); 
        
           }

src/interactivity/base/InteractivityFactory.cpp Outdated

Comment on lines 76 to 85

+                  else if (propertyId == UIA_IsControlElementPropertyId)
+                  {
+                      pVariant->vt = VT_BOOL;
+                      pVariant->boolVal = VARIANT_FALSE;
+                  }
+                  else if (propertyId == UIA_IsContentElementPropertyId)
+                  {
+                      pVariant->vt = VT_BOOL;
+                      pVariant->boolVal = VARIANT_FALSE;
+                  }

Member

DHowett Dec 13, 2022

Got it! Thank you!

src/interactivity/base/InteractivityFactory.cpp Outdated Show resolved Hide resolved

src/interactivity/base/InteractivityFactory.hpp Outdated Show resolved Hide resolved

src/interactivity/base/InteractivityFactory.hpp Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback and removed Needs-Author-Feedback labels


          address Dustin's comments

8c2108c

DHowett reviewed

View reviewed changes

src/interactivity/base/PseudoConsoleWindowAccessibilityProvider.cpp Show resolved Hide resolved

DHowett requested changes

View reviewed changes

Member

DHowett left a comment

ghost added Needs-Author-Feedback and removed Needs-Author-Feedback labels


          take a break

afc797a

DHowett dismissed zadjii-msft’s stale review

December 16, 2022 22:17

He gone for the year!

DHowett approved these changes

View reviewed changes

src/interactivity/base/InteractivityFactory.hpp Outdated Show resolved Hide resolved

src/interactivity/base/PseudoConsoleWindowAccessibilityProvider.hpp Outdated Show resolved Hide resolved

src/interactivity/base/lib/InteractivityBase.vcxproj Show resolved Hide resolved


          address Dustin's feedback

0fc13c4

carlos-zamora requested a review from zadjii-msft

January 9, 2023 19:12

carlos-zamora assigned zadjii-msft

carlos-zamora added this to the Terminal v1.17 milestone

zadjii-msft approved these changes

View reviewed changes

Member

zadjii-msft left a comment

Thanks for the explanation yesterday - we need to add UIA support, so that we can tell UIA that we aren't a thing. Crazy, but okay.

DHowett added the AutoMerge label

ghost commented Jan 12, 2023

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost merged commit 09273be into main

ghost deleted the dev/cazamor/a11y/pseudo-console-window branch

January 12, 2023 00:06

DHowett pushed a commit that referenced this pull request


          Introduce PseudoConsoleWindow a11y provider (#14541)

7d401e8

In order to modify the accessibility information for the PseudoConsoleWindow, it needs to have a UIA provider registered. This PR introduces `PseudoConsoleWindowAccessibilityProvider` and registers it as a UIA provider appropriately. The registration process is based on that of the `WindowUiaProvider` for ConHost.

Closes #14385

## Validation Steps Performed
Run Accessibility Insights FastPass on the window. The PseudoConsoleWindow no longer is tagged as missing a name.

(cherry picked from commit 09273be)
Service-Card-Id: 87568605
Service-Version: 1.16

ghost commented Jan 24, 2023

🎉Windows Terminal v1.16.1023 (10231 and 10232) has been released which incorporates this pull request.:tada:

Handy links:

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Accessibility AutoMerge Issue-Bug Product-Terminal