Ensure automation peer is created regardless of terminal initialization #10971
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carlos-zamora
added
Area-Accessibility
ghost
requested review from
lhecker
reviewed
Aug 18, 2021
lhecker
approved these changes
Aug 18, 2021
carlos-zamora
added
zPreview-Service-Queued-1.13
|
Hello @carlos-zamora! Because this pull request has the 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 (
|
DHowett
reviewed
Aug 18, 2021
| // to paint itself *after* we hand off its ownership to the renderer. | ||
| // We split up construction and initialization of the render thread object this way | ||
| // because the renderer and render thread have circular references to each other. | ||
| auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>(); |
There was a problem hiding this comment.
I thought there was a reason we could not do this. We're totally spinning up a new thread during a constructor (whoa), which i guess isn't weird... but this might bring about some regressions. We should be ready to scrutinize Watson for regressions here.
There was a problem hiding this comment.
Trick is though, how could we possibly confirm that this is the root cause for #11135 without a live repro?
ghost
removed
the
DHowett
removed
the
zPreview-Service-Queued-1.13
DHowett
pushed a commit
that referenced
this pull request
Aug 26, 2021
## 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)
DHowett
removed
the
zStable-Service-Queued-1.12
|
🎉 Handy links: |
|
🎉 Handy links: |
2 tasks
carlos-zamora
added a commit
that referenced
this pull request
Sep 23, 2021
Adds a check before every UIA function call to ensure the terminal (specifically the buffer) is initialized before doing work. Both the `ScreenInfoUiaProvider` and the `UiaTextRange` are now covered. ## References Closes #11135 #10971 & #11042 ## Detailed Description of the Pull Request / Additional comments Originally, I tried applying this heuristic to all the `RuntimeClassInitialize` on `UiaTextRangeBase` with the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on [MSFT 33353327](https://microsoft.visualstudio.com/OS/_workitems/edit/33353327). `IUiaData` also has `GetTextBuffer()` return a `TextBuffer&`, which cannot be checked for nullness. Instead, I decided to add a function to `IUiaData` that checks if we have a valid state. Since this is shared with Conhost and Conhost doesn't have this issue, I simply make that function say that it's always in a valid state. ## Validation Steps Performed - [X] Narrator can detect newly created terminals - [X] (On Windows Server 2022) Windows Terminal does not hang on launch
carlos-zamora
added a commit
that referenced
this pull request
Sep 23, 2021
Adds a check before every UIA function call to ensure the terminal (specifically the buffer) is initialized before doing work. Both the `ScreenInfoUiaProvider` and the `UiaTextRange` are now covered. ## References Closes #11135 #10971 & #11042 ## Detailed Description of the Pull Request / Additional comments Originally, I tried applying this heuristic to all the `RuntimeClassInitialize` on `UiaTextRangeBase` with the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on [MSFT 33353327](https://microsoft.visualstudio.com/OS/_workitems/edit/33353327). `IUiaData` also has `GetTextBuffer()` return a `TextBuffer&`, which cannot be checked for nullness. Instead, I decided to add a function to `IUiaData` that checks if we have a valid state. Since this is shared with Conhost and Conhost doesn't have this issue, I simply make that function say that it's always in a valid state. ## Validation Steps Performed - [X] Narrator can detect newly created terminals - [X] (On Windows Server 2022) Windows Terminal does not hang on launch
carlos-zamora
added a commit
that referenced
this pull request
Sep 23, 2021
Adds a check before every UIA function call to ensure the terminal (specifically the buffer) is initialized before doing work. Both the `ScreenInfoUiaProvider` and the `UiaTextRange` are now covered. ## References Closes #11135 #10971 & #11042 ## Detailed Description of the Pull Request / Additional comments Originally, I tried applying this heuristic to all the `RuntimeClassInitialize` on `UiaTextRangeBase` with the philosophy of "a range pointing to an invalid buffer is invalid itself", but that caused a regression on [MSFT 33353327](https://microsoft.visualstudio.com/OS/_workitems/edit/33353327). `IUiaData` also has `GetTextBuffer()` return a `TextBuffer&`, which cannot be checked for nullness. Instead, I decided to add a function to `IUiaData` that checks if we have a valid state. Since this is shared with Conhost and Conhost doesn't have this issue, I simply make that function say that it's always in a valid state. ## Validation Steps Performed - [X] Narrator can detect newly created terminals - [X] (On Windows Server 2022) Windows Terminal does not hang on launch
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
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...
TermControl:_InitializeTerminalupdates the padding (dependent on theSwapChainPanel) upon full initializationControlCore: initialize the_rendererin the ctor so that we can attach the UIA Engine beforeControlCore::Initialize()is called (dependent onSwapChainPanelloading)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.
PR Checklist
Closes MSFT 33353327
Validation Steps Performed