Move Terminal UIA provider into Types & hook it up to the WPF control #4548

ZoeyR · 2020-02-12T17:51:26Z

This PR hooks up the existing UIA implementation to the WPF control. Some existing code that was specific to the UWP terminal control could be shared so that has been refactored to a common location as well.

Validation Steps Performed

WPF control was brought up in UISpy and the UIA tree was verified. NVDA was then used to check that screen readers were operating properly.

DHowett-MSFT

This seems reasonable, but I have some minor questions and I need @carlos-zamora and @miniksa to take a good long look at it. I'll red-x for now, but I love where it's going.

src/cascadia/TerminalControl/TermControlAutomationPeer.cpp

src/types/IControlInfo.h

src/types/TermControlUiaProvider.cpp

src/types/TermControlUiaTextRange.cpp

ZoeyR · 2020-02-12T18:56:23Z

Can someone help me with this precomp.h issue with the header files? Shouldn't the header files not need it?

Edit: Nevermind, figured out the issue, I had something <ClCompile> and not <ClInclude>

ZoeyR · 2020-02-12T20:38:22Z

I think I hit some new static analysis issues with code that wasn't covered before.

miniksa

Just some comments. Looks pretty alright to me.

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

ZoeyR · 2020-02-14T18:11:53Z

@miniksa or @DHowett Why would I be getting static analysis issues with UiaTextRangeBase.hpp? I didn't edit that at all.

carlos-zamora · 2020-02-14T19:28:03Z

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

+
+double HwndTerminal::GetScaleFactor() const
+{
+    return static_cast<double>(_currentDpi) / static_cast<double>(USER_DEFAULT_SCREEN_DPI);


nit: you could probably use base::ClampDiv() here.

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

carlos-zamora

I get why we moved UiaTextRange to Types. But can we keep the same name? At least to reduce merge conflicts? We can probably take on the renaming once we merge IControlAccessibilityInfo with UiaTextRangeBase.
Would you be able to just have IControlAccessibilityInfo needed at the ScreenInfoUiaProviderBase layer instead of the TermControlUiaProvider?

ZoeyR · 2020-02-14T19:34:22Z

If we keep the same name wouldn't it conflict with the names in src\interactivity\win32?

As for the IControlAccessibilityInfo comment, We'd again run into issues migrating over the stuff in win32

carlos-zamora · 2020-02-14T21:20:07Z

If we keep the same name wouldn't it conflict with the names in src\interactivity\win32?

As for the IControlAccessibilityInfo comment, We'd again run into issues migrating over the stuff in win32

Ugh. Yeah. Win32 is the problem here. That'll all have to be fixed as a part of resolving the "merge IControlAccesibilityInfo with UiaTextRangeBase" issue. That's annoying but, other than that, I don't think I have any blocking comments.

DHowett-MSFT · 2020-02-18T18:45:55Z

It looks like, by rights, these issues should have tripped up SA before your PR. Hmm.

DHowett-MSFT

Alright, this looks good to me. Thanks so much for abstracting the things that made it TerminalControl-specific. This'll be a good platform on which we can re-merge the disparate UiaTextRangeBase derivations into a single class.

We should eventually crush together all the interfaces, where possible, but I won't ask you to do that.

DHowett-MSFT · 2020-02-24T23:05:22Z

src/cascadia/TerminalControl/TermControlAutomationPeer.h

+        virtual COORD GetFontSize() const override;
+        virtual RECT GetBounds() const override;
+        virtual RECT GetPadding() const override;
+        virtual double GetScaleFactor() const override;
+        virtual void ChangeViewport(SMALL_RECT NewWindow) override;
+        virtual HRESULT GetHostUiaProvider(IRawElementProviderSimple** provider) override;


nit: SA would complain about the virtual here because override implies it

ghost · 2020-02-24T23:17:42Z

Hello @DHowett-MSFT!

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.

PR #4548 inadvertantly broke mouse button input in the WPF control. This happened due to the extra layer of HWND indirection. The fix is to move the mouse button handling down into the native control where the window messages are now being sent.

PR #4548 inadvertantly broke mouse button input in the WPF control. This happened due to the extra layer of HWND indirection. The fix is to move the mouse button handling down into the native control where the window messages are now being sent. (cherry picked from commit 4393fef)

DHowett-MSFT suggested changes Feb 12, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Feb 12, 2020

ZoeyR force-pushed the dev/zorio/wpf-uia-tree branch from 2027c0a to 9e54c16 Compare February 12, 2020 18:47

miniksa reviewed Feb 12, 2020

View reviewed changes

carlos-zamora reviewed Feb 14, 2020

View reviewed changes

src/cascadia/PublicTerminalCore/HwndTerminal.cpp Show resolved Hide resolved

carlos-zamora requested changes Feb 14, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 14, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 14, 2020

carlos-zamora approved these changes Feb 14, 2020

View reviewed changes

ZoeyR force-pushed the dev/zorio/wpf-uia-tree branch from f8710f0 to ad7d72d Compare February 24, 2020 20:30

hook up UIA tree to WPF control

ac4f4a1

ZoeyR force-pushed the dev/zorio/wpf-uia-tree branch from df8fcdc to ac4f4a1 Compare February 24, 2020 22:49

ZoeyR requested a review from DHowett-MSFT February 24, 2020 22:51

DHowett-MSFT approved these changes Feb 24, 2020

View reviewed changes

DHowett-MSFT changed the title ~~hook up UIA tree to WPF control~~ Move Control UIA provider into Types and hook it up to the WPF control Feb 24, 2020

DHowett-MSFT changed the title ~~Move Control UIA provider into Types and hook it up to the WPF control~~ Move Terminal UIA provider into Types & hook it up to the WPF control Feb 24, 2020

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 24, 2020

ghost merged commit 4def49c into master Feb 24, 2020

ghost deleted the dev/zorio/wpf-uia-tree branch February 24, 2020 23:18

ZoeyR mentioned this pull request Feb 25, 2020

fix mouse events in the wpf control #4720

Merged

5 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Terminal UIA provider into Types & hook it up to the WPF control #4548

Move Terminal UIA provider into Types & hook it up to the WPF control #4548

ZoeyR commented Feb 12, 2020 •

edited by DHowett-MSFT

Loading

DHowett-MSFT left a comment

ZoeyR commented Feb 12, 2020 •

edited

Loading

ZoeyR commented Feb 12, 2020

miniksa left a comment

ZoeyR commented Feb 14, 2020

carlos-zamora Feb 14, 2020

carlos-zamora left a comment

ZoeyR commented Feb 14, 2020

carlos-zamora commented Feb 14, 2020

DHowett-MSFT commented Feb 18, 2020

DHowett-MSFT left a comment

DHowett-MSFT Feb 24, 2020

ghost commented Feb 24, 2020

Move Terminal UIA provider into Types & hook it up to the WPF control #4548

Move Terminal UIA provider into Types & hook it up to the WPF control #4548

Conversation

ZoeyR commented Feb 12, 2020 • edited by DHowett-MSFT Loading

Validation Steps Performed

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ZoeyR commented Feb 12, 2020 • edited Loading

ZoeyR commented Feb 12, 2020

miniksa left a comment

Choose a reason for hiding this comment

ZoeyR commented Feb 14, 2020

carlos-zamora Feb 14, 2020

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

ZoeyR commented Feb 14, 2020

carlos-zamora commented Feb 14, 2020

DHowett-MSFT commented Feb 18, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT Feb 24, 2020

Choose a reason for hiding this comment

ghost commented Feb 24, 2020

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.

ZoeyR commented Feb 12, 2020 •

edited by DHowett-MSFT

Loading

ZoeyR commented Feb 12, 2020 •

edited

Loading

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.