Only access ControlInteractivity through the projection by zadjii-msft · Pull Request #10051 · microsoft/terminal

zadjii-msft · 2021-05-07T17:37:10Z

Summary of the Pull Request

This forces the TermControl to only use ControlCore and ControlInteractivity via their WinRT projections. We want this, because WinRT projections can be used across process boundaries. In the future, ControlCore and ControlInteractivity are going to be living in a different process entirely from TermControl. By enforcing this boundary now, we can make sure that they will work seamlessly in the future.

References

PR Checklist

Closes https://github.com/microsoft/terminal/projects/5#card-50760270
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

Most all this was just converting pure c++ types to winrt types when possible. I've added a couple helper projections with til converters, which made most of this really easy.

The "MouseButtonState needs to be composed of Int32s instead of bools" is MENTAL. I have no idea why this is, but when I had the control OOP in the sample, that would crash when trying to de-marshal the bools. BODGY.

The biggest changes are in the way the UIA stuff is hooked up. The UiaEngine needs to be attached directly to the Renderer, and it can't be easily projected, so it needs to live next to the ControlCore. But the TermControlAutomationPeer needed the UiaEngine to help implement some interfaces.

Now, there's a new layer we've introduced. InteractivityAutomationPeer does the ITextProvider, IControlAccessibilityInfo and the IUiaEventDispatcher thing. TermControlAutomationPeer now has a
InteractivityAutomationPeer stashed inside itself, so that it can ask the interactivity layer to do the real work. We still need the TermControlAutomationPeer though, to be able to attach to the real UI tree.

Validation Steps Performed

The terminal behaves basically the same as before.

Most importantly, I whipped out Accessibility Insights, and the Terminal looks the same as before.

Now we have an InteractivityAutomationPeer, which acts like the implementation for TermControlAutomationPeer. TermControlAutomationPeer is still needed though, because it implements FrameworkElementAutomationPeer, which is needed to hook up the AP to the actual UIA tree. So we split up the work into two halves, the bit that should be done by the core (the ITextProvider, the IControlAccessibilityInfo), vs the stuff that needs to be in the control (FrameworkElementAutomationPeer). IUiaEventDispatcher should probably be in the control as well, but that's a problem for future us.

… remain.

This reverts commit c113b65. That commit reverted 30b8335

…ractivity-projection-000

…ity-projection-000 # Conflicts: # src/cascadia/TerminalControl/ControlInteractivity.cpp # src/cascadia/TerminalControl/ControlInteractivity.h # src/cascadia/TerminalControl/TermControl.cpp # src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

…s are still wrong

…dcomp-handle

…ractivity-projection-000

carlos-zamora

Tested NVDA on this. The output isn't being read out. Here's the repro steps:

turn on NVDA
open powershell core
run echo hello
Expected: hear the words "hello" (verified on v1.9.1252.0)
Actual: no output is read

My guess is that the automation event isn't being fired. You can use accevent.exe to see what events get fired, if you'd like. Or just use NVDA to get the repro above.

src/cascadia/TerminalCore/ICoreAppearance.idl

src/cascadia/TerminalControl/ControlCore.idl

src/cascadia/TerminalControl/ControlCore.h

carlos-zamora · 2021-05-11T20:15:48Z

src/cascadia/TerminalControl/InteractivityAutomationPeer.idl

+namespace Microsoft.Terminal.Control
+{
+    [default_interface] runtimeclass InteractivityAutomationPeer :
+        Windows.UI.Xaml.Automation.Peers.AutomationPeer,


Suggested change

Windows.UI.Xaml.Automation.Peers.AutomationPeer,

Windows.UI.Xaml.Automation.Peers.FrameworkElementAutomationPeer,

I think we should be using FrameworkElementAutomationPeer by default (source). It apparently does some of the work for us, so I guess it's better to be safe than sorry.

So, I was trying to avoid using as much XAML as possible in the Interactivity layer, since I wanted that to ultimately be UI framework independent. Most of the rest of the UIA stuff is however, real XAML-y. So I can't be totally pure, no-Windows.UI.Xaml here unfortunately. I think it's still a good idea to try and move away from XAML for these bits as much as possible.

On the latest commit, I've fixed the signaling for JAWS. That means we definitely don't need the whole FrameworkElementAutomationPeer, so I'd rather use as little XAML as we can

src/cascadia/TerminalControl/InteractivityAutomationPeer.h

src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp

src/cascadia/TerminalControl/TermControlAutomationPeer.h

…ity-projection-000

miniksa · 2021-07-15T22:43:27Z

src/cascadia/TerminalControl/ControlCore.idl

+    // but projectable.
+    // !! LOAD BEARING !! If you make this a struct with Booleans (like they
+    // make the most sense as), then the app will crash trying to toss one of
+    // these across the process boundary. I haven't the damndest idea why.


Well that's weird.

miniksa · 2021-07-15T22:56:05Z

src/cascadia/TerminalControl/InteractivityAutomationPeer.cpp

+    {
+        // transfer ownership of UiaTextRanges to this new vector
+        auto providers = SafeArrayToOwningVector<::Microsoft::Terminal::TermControlUiaTextRange>(textRanges);
+        int count = gsl::narrow<int>(providers.size());


Why is this an int? Can't std::vector and the [] on providers both handle... presumably size_t?

…ity-projection-000

DHowett · 2021-07-19T16:10:27Z

src/cascadia/TerminalControl/ControlCore.cpp

    }

-    std::optional<til::point> ControlCore::GetHoveredCell() const
+    Windows::Foundation::IReference<Core::Point> ControlCore::HoveredCell() const


depending on how often this is called, it could be quite expensive. an IReference is a heap allocation for every .. thing.

DHowett · 2021-07-19T16:12:03Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

 {
+    static constexpr TerminalInput::MouseButtonState toInternalMouseState(const Control::MouseButtonState& state)
+    {
+        return TerminalInput::MouseButtonState{


using a struct for this was weird in the first place. glad we're shoving it further away :)

DHowett · 2021-07-19T16:17:00Z

src/cascadia/TerminalControl/TermControl.cpp

+        _interactivity.UpdateSettings();
+        if (_automationPeer)
+        {
+            _automationPeer.SetControlPadding(Core::Padding{ newMargin.Left,


It doesn't seem like we're calling this when we create the automation peer -- we are not guaranteed to always call UpdateSettings after creating a peer, so the first peer may have no padding.

lemme toss this in the interactivity follow up bugs thread

DHowett

No significant complaints! Well done. Sorry for the slow review.

#### ⚠️ targets #10051 ## Summary of the Pull Request This PR does one big, primary thing. It removes all the constructors from any TerminalConnections, and changes them to use an `Initialize` method that accepts a `ValueSet` of properties. Why? For the upcoming window/content process work, we'll need the content process to be able to initialize the connection _in the content process_. However, the window process will be the one that knows what type of connection to make. Enter `ConnectionInformation`. This class will let us specify the class name of the type we want to create, and a set of settings to use when initializing that connection. **IMPORTANT**: As a part of this, the constructor for a connection must have 0 arguments. `RoActivateInstance` lets you just conjure a WinRT type just by class name, but that class must have a 0 arg ctor. Hence the need for `Initialize`, to actually pass the settings. We're using a `ValueSet` here because it's basically a json blob, with more steps. In the future, when extension authors want to have custom connections, we can always deserialize the json into a `ValueSet`, pass it to their connection's `Initialize`, and let then get what they need out of it. ## References * Tear-out: #1256 * Megathread: #5000 * Project: https://github.com/microsoft/terminal/projects/5 ## PR Checklist * [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760298 * [x] I work here * [n/a] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments `ConnectionInformation` was included as a part of this PR, to demonstrate how this will eventually be used. `ConnectionInformation` is not _currently_ used. ## Validation Steps Performed It still builds and runs.

## Summary of the Pull Request This fixes two bugs related to dragging into the bounds of the `TermControl`. Although the fixes are fairly small, I'm batching them up, because I don't want to stack 2 more PRs on top of #10051. * #9109 - This is fixed by only starting an autoscroll if the click&drag actually started within the bounds of the control. * #4603 - Building on the above change, only modify the selection when the drag started in the control. ## References * srsly go read #10051. ## PR Checklist * [x] Closes #9109 * [x] Closes #4603 * [x] I work here * [x] Test added * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments This is kind of annoying that the auto-scrolling is handled by the TermControl, but it uses a timer that's still a WinUI construct. We only want to start the auto-scrolling behavior when the drag started _inside_ the control. Otherwise, in the tab drag scenario, dragging into the bounds of the TermControl will trick it into thinking it should start a scroll.

#### ⚠️ targets #10051 ## Summary of the Pull Request This updates our `ThrottledFunc`s to take a dispatcher parameter. This means that we can use the `Windows::UI::Core::CoreDispatcher` in the `TermControl`, where there's always a `CoreDispatcher`, and use a `Windows::System::DispatcherQueue` in `ControlCore`/`ControlInteractivity`. When running in-proc, these are always the _same thing_. However, out-of-proc, the core needs a dispatcher queue that's not tied to a UI thread (because the content proces _doesn't have a UI thread!_). This lets us get rid of the output event, because we don't need to bubble that event out to the `TermControl` to let it throttle that update anymore. ## References * Tear-out: #1256 * Megathread: #5000 * Project: https://github.com/microsoft/terminal/projects/5 ## PR Checklist * [x] This is a part of #1256 * [x] I work here * [n/a] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments Fortunately, `winrt::resume_foreground` works the same on both a `CoreDispatcher` and a `DispatcherQueue`, so this wasn't too hard! ## Validation Steps Performed This was validated in `dev/migrie/oop/the-whole-thing` (or `dev/migrie/oop/connection-factory`, I forget which), and I made sure that it worked both in-proc and x-proc. Not only that, _it wasn't any slower_!This reverts commit 04b751f.

## Summary of the Pull Request This was missed in #10051. We need to make sure that the UIA provider can immediately know about the padding in the control, not just after the settings reload. ## PR Checklist * [x] Closes #9955.e * [x] Additionally, this just closes #9955. The only remaining box in there never repro'd, so probably wasn't even root caused by #9820. I think we can close that issue for now, and reactivate if something else was broken. * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Checked before/after in Accessibility Insights. Before the row rectangles were the full width of the control initially. Now they're properly padded.

## Summary of the Pull Request This patch accomplishes what #10971 does, but for the release-1.10 branch. Some things couldn't be carried over because the TermControl split wasn't the complete (or in the same state as it is currently on main). The bug was that Narrator would still read the content of the old tab/pane although a new tab/pane was introduced. This is caused by the automation peer not being created when XAML requests it. Normally, we would prevent the automation peer from being created if the terminal was not fully initialized. This change allows the automation peer to be created regardless of the terminal being fully initialized by... - `ControlCore`: initialize the `_renderer` in the ctor so that we can attach the UIA Engine before `ControlCore::Initialize()` is called (dependent on `SwapChainPanel` loading) As a bonus, this also fixes a locking issue where logging would attempt to get the text range's text and lock twice. The locking fix is very similar to #10937. ## Differences from #10971 This was prior to #10874. _Thankfully_, we don't need to worry about the padding because that bug was introduced as a part of #10051. ## Other references MSFT-33353327 ## Validation Steps Performed - New pane from key binding is announced by Narrator - New tab from key binding is announced by Narrator - Bounding rects are placed on the screen accurately (even when the padding changes)

This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method. In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654 #10051 didn't change this In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before: * [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler * [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d * [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged` ## Validation Steps Performed Print a URL, scroll the wheel: it still works. Closes #11055

This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method. In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654 #10051 didn't change this In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before: * [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler * [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d * [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged` ## Validation Steps Performed Print a URL, scroll the wheel: it still works. Closes #11055 (cherry picked from commit 7423734)

ghost · 2021-08-31T17:10:36Z

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

Handy links:

## Summary of the Pull Request As a part of the Interactivity split, `TermControlAutomationPeer` had to be split into `TermControlAutomationPeer` (TCAP) and `InteractivityAutomationPeer` (IAP). Just about all of the functions in `InterativityAutomationPeer` operate by calling the non-XAML UIA Provider then wrapping the resulting `UIATextRange` into a XAML format (a `XamlUiaTextRange` [XUTR]). As a part of that XUTR constructor, we need a reference to the parent provider. We generally get that via `ProviderFromPeer()`, but IAP's `ProviderFromPeer()` returned null (presumably because IAP isn't in the UI tree, whereas TCAP is directly registered as the automation peer for the `TermControl`). It looks like some screen readers didn't care (like NVDA, though there may be a chance we just didn't encounter an issue just yet), but Narrator definitely did. The fix was to provide XUTR constructors the `ProviderFromPeer` from TCAP, _not_ IAP. To accomplish this, IAP now holds a weak reference to TCAP, and provides the `ProviderFromPeer` when needed. We can't cache this result because there is no guarantee that it won't change. Some miscellaneous changes include: - `TermControl::OnCreateAutomationPeer` now returns the existing auto peer instead of always creating a new one - `TCAP::WrapArrayOfTextRangeProviders` was removed as it was unused (normally, this would be directly affected by the main `ProviderFromPeer` change here) - `XUTR::GetEnclosingElement` is now hooked up to trace logging for debugging purposes ## References Introduced in #10051 Closes #11488 ## Validation Steps Performed ✅ Narrator scan mode now works (verified with character, word, and line navigation) ✅ NVDA movement still works (verified with word and line navigation)

## Summary of the Pull Request As a part of the Interactivity split, `TermControlAutomationPeer` had to be split into `TermControlAutomationPeer` (TCAP) and `InteractivityAutomationPeer` (IAP). Just about all of the functions in `InterativityAutomationPeer` operate by calling the non-XAML UIA Provider then wrapping the resulting `UIATextRange` into a XAML format (a `XamlUiaTextRange` [XUTR]). As a part of that XUTR constructor, we need a reference to the parent provider. We generally get that via `ProviderFromPeer()`, but IAP's `ProviderFromPeer()` returned null (presumably because IAP isn't in the UI tree, whereas TCAP is directly registered as the automation peer for the `TermControl`). It looks like some screen readers didn't care (like NVDA, though there may be a chance we just didn't encounter an issue just yet), but Narrator definitely did. The fix was to provide XUTR constructors the `ProviderFromPeer` from TCAP, _not_ IAP. To accomplish this, IAP now holds a weak reference to TCAP, and provides the `ProviderFromPeer` when needed. We can't cache this result because there is no guarantee that it won't change. Some miscellaneous changes include: - `TermControl::OnCreateAutomationPeer` now returns the existing auto peer instead of always creating a new one - `TCAP::WrapArrayOfTextRangeProviders` was removed as it was unused (normally, this would be directly affected by the main `ProviderFromPeer` change here) - `XUTR::GetEnclosingElement` is now hooked up to trace logging for debugging purposes ## References Introduced in #10051 Closes #11488 ## Validation Steps Performed ✅ Narrator scan mode now works (verified with character, word, and line navigation) ✅ NVDA movement still works (verified with word and line navigation) (cherry picked from commit 02ac246)

zadjii-msft added 22 commits April 29, 2021 12:03

Only access ControlInteractivity through the projection

e4478ee

Only 24 more errors to go

400b35f

some comments that should have been in the previous commit. 16 errors…

4578c13

… remain.

GetHoveredCell/UpdateHoveredCell. 12 remain.

d8cc047

Revert "Revert "Use DComp surface handle for Swap Chain management.""

21a97b6

This reverts commit c113b65. That commit reverted 30b8335

some minor cleanup

2e861d8

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

e20caae

…ractivity-projection-000

Use the HANDLE for the swapchainin the core projection

d667f3b

it builds and runs so I guess it's done now

62cbf30

Do the delayload dance

445bdf6

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

1376721

…ractivity-projection-000

What a ridiculous hack

a1ce7cb

fix accessibility

03fb764

I want to get the padding out of interactivity, but the control bound…

6e031e5

…s are still wrong

now this works!

47410c9

Some cleanup

0869456

a bit more cleanup

dace0c4

Merge remote-tracking branch 'origin/main' into dev/migrie/f/oop/use-…

12b78f5

…dcomp-handle

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

e340fe9

…ractivity-projection-000

god I knew I left these behind

b7b23a8

zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels May 7, 2021

This comment has been minimized.

Sign in to view

This was referenced May 7, 2021

Scenario: Windows Terminal 2.0 Process Model Improvements #5000

Closed

Add a Scratch.sln for prototyping #10067

Merged

carlos-zamora requested changes May 11, 2021

View reviewed changes

Base automatically changed from dev/migrie/f/oop/use-dcomp-handle to main May 12, 2021 16:54

Merge remote-tracking branch 'origin/main' into dev/migrie/interactiv…

5a598f7

…ity-projection-000

zadjii-msft mentioned this pull request Jul 13, 2021

Fix a pair of TermControl dragging bugs #10650

Merged

4 tasks

cinnamon-msft modified the milestones: Terminal v1.10, Terminal v1.11 Jul 14, 2021

miniksa approved these changes Jul 15, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/migrie/interactiv…

3797d52

…ity-projection-000

DHowett reviewed Jul 19, 2021

View reviewed changes

DHowett approved these changes Jul 19, 2021

View reviewed changes

zadjii-msft mentioned this pull request Jul 19, 2021

Interactivity bugs post-#9820 #9955

Closed

5 tasks

zadjii-msft merged commit 7f3bc3c into main Jul 19, 2021

zadjii-msft deleted the dev/migrie/interactivity-projection-000 branch July 19, 2021 16:59

zadjii-msft mentioned this pull request Aug 4, 2021

Initialize the padding for the Control UIA provider #10874

Merged

4 tasks

carlos-zamora mentioned this pull request Aug 25, 2021

[1.10] Ensure auto peer is created regardless of term init #11042

Merged

This was referenced Aug 26, 2021

1.11: scrolling at all breaks the URL pattern buffer (no URLs work) #11055

Closed

Update pattern locations again after scrolling #11059

Merged

carlos-zamora mentioned this pull request Oct 13, 2021

Properly initialize XamlUiaTextRange with ProviderFromPeer #11501

Merged

zadjii-msft mentioned this pull request Jan 3, 2022

Feature Request: Mouse/Touch/Pointer Bindings (like middle-click paste, right-click context menu, etc.) #1553

Open

14 tasks

	Windows.UI.Xaml.Automation.Peers.AutomationPeer,
	Windows.UI.Xaml.Automation.Peers.FrameworkElementAutomationPeer,

Conversation

zadjii-msft commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zadjii-msft commented May 7, 2021 •

edited

Loading