Fix UIA and marks regressions due to cooked read #16105

lhecker · 2023-10-05T17:16:35Z

The initial cooked read (= conhost readline) rewrite had two flaws:

Using viewport scrolls under ConPTY to avoid emitting newlines resulted in various bugs around marks, coloring, etc. It's still somewhat unclear why this happened, but the next issue is related and much worse.
Rewriting the input line every time causes problems with accessibility tools, as they'll re-announce unchanged parts again and again.

The solution to these is to simply stop writing the unchanged parts of the prompt. To do this, code was added to measure the size of text without actually inserting them into the buffer. Since this meant that the "interactive" mode of WriteCharsLegacy would need to be duplicated for the new code, I instead moved those parts into COOKED_READ_DATA. That way we can now have the interactive transform of the prompt (= Ctrl+C -> ^C) and the two text functions (measure text & actually write text) are now agnostic to this transformation.

Closes #16034
Closes #16044

Validation Steps Performed

A vision impaired user checked it out and it seemed fine ✅

carlos-zamora · 2023-10-11T19:51:54Z

Tested output from ls and ping bing.com and it seems to work fine with Narrator and NVDA. My build of NVDA might not be using UIA notifications though.

Sent a build to @codeofdusk so he can test it just in case. If you find anything, feel free to throw the feedback in here 😊

DHowett · 2023-10-11T20:29:38Z

Tested output from ls and ping bing.com and it seems to work fine

For what it's worth, this change (and the bug it fixes) is in input in CMD/python/etc. and not output.

carlos-zamora · 2023-10-11T20:02:29Z

src/host/readDataCooked.cpp

+#define _buffer DONT_USE_ME
+#define _bufferCursor DONT_USE_ME


I was gonna say that you should remove these, but actually, it's kinda a neat trick to make sure we don't mess this up later. Cool!

clever! wil also added a special thing you can use to override a local.

hide_name _buffer;

except it applies inside a function scope, so it's not applicable here

src/host/readDataCooked.cpp

carlos-zamora · 2023-10-12T02:26:45Z

Tested output from ls and ping bing.com and it seems to work fine with Narrator and NVDA. My build of NVDA might not be using UIA notifications though.

Sent a build to @codeofdusk so he can test it just in case. If you find anything, feel free to throw the feedback in here 😊

I'm going on vacation for a few days, but I got a chance to chat with Bill so I want to share some notes from our chat:

What is the problem?
- A while back (this PR) we made Windows Terminal dispatch a UIA notification on new output. This is great! It fixed Narrator and it provided a glimpse of a future where screen readers wouldn't necessarily need to rely on diffing the buffer so much (which is slow and expensive, especially when Terminal already knows what's being output). NVDA has specific logic/settings to handle these notifications, and is even experimenting with ignoring text changed events (events that signify that the text buffer changed contents, without providing specific details of what exactly changed). It still has some quirks to figure out (like it reads passwords out as you type them, etc.), which is why it's currently hidden behind a setting.
- Anyways, one problem that this PR had to work around was a "local echo". Basically, as you type in chars, we would send out a UIA notification for the character being rendered to the screen, and NVDA would additionally read the typed character. This would result in each character being read twice. There's some special logic in the PR here that tries to find the common prefix and suppresses those notifications (or at least some of the content from the notification).
Long Term recommendation (not as relevant, but still worth including somewhere):
- long-term, an idea is to add a side channel (either via a custom event or UIA pattern) to tell NVDA what chars are echoed
Regression-specific notes:
- I sat down and tested this build with him. We found that it's fixed in CMD but broken in PowerShell.
- Good way to test it:
  - Get NVDA (I recommend you get the latest beta here, it'll update and stay on that channel when there's one available; you'll get a notification when NVDA starts up)
    - during installation, one of the popups may ask if you want to set Caps Lock as the NVDA key. Make sure you check that box!
  - After installing it, run NVDA and open the settings and change them as described here:
    - in system menu (taskbar), right click on NVDA > Preferences > Settings...
    - Go to Advanced page
    - Check the "I understand that changing these settings..." box
    - Under "Terminal programs" set "Speak new text in Windows Terminal via:" to UIA notifications
  - Now, let's actually test it! Open up Terminal and the shell you want to test
  - NOTE: when I say "NVDA" as a key, I mean press the Caps Lock key (Caps Lock == NVDA key)
  - press NVDA+2 until NVDA says "<something> off"
  - press NVDA+3 until NVDA says "<something> off"
  - Now, when you type, you should get no feedback! We essentially disabled the NVDA side of the "local echo" (the one that isn't from the UIA notification)
  - Additionally, when you press backspace, it should still read out what got deleted
  - if you want to know what the expected behavior should be, open up notepad or ms word. It does just that!

Hope this helps!

lhecker · 2023-10-17T16:12:09Z

Thanks for testing this PR in my absence! And thanks for the NVDA instructions. :)

I sat down and tested this build with him. We found that it's fixed in CMD but broken in PowerShell.

That means the issue is fixed by this PR, right? Since PowerShell has its own readline implementation...

DHowett

I think I understand this! Which is a miracle.

zadjii-msft

8/11. Sorry, should have gotten to this sooner. Got to 5pm and ran out of time

zadjii-msft · 2023-10-19T22:00:44Z

src/host/_stream.cpp

-        // because the entire prompt is supposed to fit into the VT viewport, with no scrollback. If we didn't do that,
-        // any prompt larger than the viewport will cause >1 lines to be added to scrollback, even if typing backspaces.
-        // You can test this by removing this branch, launch Windows Terminal, and fill the entire viewport with text in cmd.exe.
-        if (needsConPTYWorkaround)


note to self: this branch was removed. That seems good

zadjii-msft · 2023-10-19T22:01:03Z

src/host/_stream.cpp

            }
-        }
-        else


this branch is the same

zadjii-msft · 2023-10-19T22:03:26Z

src/host/_stream.cpp

@@ -128,15 +105,11 @@ static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point
        LOG_IF_FAILED(screenInfo.SetViewportOrigin(false, WindowOrigin, true));
    }

-    if (interactive)
-    {
-        screenInfo.MakeCursorVisible(coordCursor);


this was removed, but was only ever used for

COOKED_READ_DATA::_writeChars

COOKED_READ_DATA::_handlePostCharInputLoop

zadjii-msft · 2023-10-19T22:05:38Z

src/host/_stream.cpp

-                if (interactive)
-                {
-                    break;
-                }
                std::ignore = screenInfo.SendNotifyBeep();


note: interesting, we didn't beep if we were interactive before. Interesting?

same with the null above, obv.

zadjii-msft

okay you're right this was quite repetitive even if you really have to squint a lot

zadjii-msft · 2023-10-20T15:50:11Z

src/buffer/out/textBuffer.cpp

+
+        // But if we simply ran out of text we just need to return the actual number of columns.
+        it += len;
+        if (it == end)


any chance we accidentally step over end? (like, should we have a it>end assert or bail or something)

len is either 1 or 2 depending on whether the current code point is a surrogate pair. A few lines above, len is only set to 2 if it + 1 != end which means that it += 2 is safe.

zadjii-msft · 2023-10-20T15:53:45Z

src/host/readDataCooked.cpp

@@ -11,6 +11,9 @@
 #include "_stream.h"
 #include "../interactivity/inc/ServiceLocator.hpp"

+#define _buffer DONT_USE_ME


i mean, I'm not gonna block over it (cause this is a hotfix), but theoretically wouldn't the usual way of doing this be to introduce a class/struct to wrap _buffer and then actually only expose the setters as public methods on that class?

oh I see, _handlePostCharInputLoop. gotcha

I could do that. I've been thinking about doing it for a bit and there's no particular reason why I picked this approach over writing a wrapper class. It was just what came to my mind first.

If this PR isn't accidentally merged in the next day, I'll try to write a wrapper class and push it. :)

I've just pushed a commit where I'm now using a wrapper class to track dirtyBeg.

DHowett · 2023-10-23T19:09:49Z

src/host/readDataCooked.hpp

+    struct BufferState
+    {
+        const std::wstring& Get() const noexcept;
+        std::wstring Extract() noexcept


does this signature offer the right move guarantees? you don't need to return an rvalue ref, do you?

Due to RVO this should be more efficient I think. It's also probably a bit safer since it cannot result in stale pointers.

DHowett · 2023-10-23T19:10:20Z

src/host/readDataCooked.cpp

 // Handles any tasks that need to be completed after the read input loop finishes,
 // like handling doskey aliases and converting the input to non-UTF16.
 void COOKED_READ_DATA::_handlePostCharInputLoop(const bool isUnicode, size_t& numBytes, ULONG& controlKeyState)
 {
    auto writer = _userBuffer;
-    std::wstring_view input{ _buffer };
+    auto buffer = _buffer.Extract();


this code didn't used to destroy _buffer on all codepaths. now it does! is it OK to do so?

Yes, because this function can only be called once during the lifetime of the object instance.

The initial cooked read (= conhost readline) rewrite had two flaws: * Using viewport scrolls under ConPTY to avoid emitting newlines resulted in various bugs around marks, coloring, etc. It's still somewhat unclear why this happened, but the next issue is related and much worse. * Rewriting the input line every time causes problems with accessibility tools, as they'll re-announce unchanged parts again and again. The solution to these is to simply stop writing the unchanged parts of the prompt. To do this, code was added to measure the size of text without actually inserting them into the buffer. Since this meant that the "interactive" mode of `WriteCharsLegacy` would need to be duplicated for the new code, I instead moved those parts into `COOKED_READ_DATA`. That way we can now have the interactive transform of the prompt (= Ctrl+C -> ^C) and the two text functions (measure text & actually write text) are now agnostic to this transformation. Closes #16034 Closes #16044 ## Validation Steps Performed * A vision impaired user checked it out and it seemed fine ✅ (cherry picked from commit e1c69a9) Service-Card-Id: 90891693 Service-Version: 1.19

A late change in #16105 wrapped `_buffer` into a class to better track its dirty state, but I failed to notice that in this one instance we intentionally manipulated `_buffer` without marking it as dirty. This fixes the issue by adding a call to `MarkAsClean()`. This changeset also adds the test instructions from #15783 as a document to this repository. I've extended the list with two bugs we've found in the implementation since then. ## Validation Steps Performed * In cmd.exe, with an empty prompt in an empty directory: Pressing tab produces an audible bing and prints no text ✅

A late change in #16105 wrapped `_buffer` into a class to better track its dirty state, but I failed to notice that in this one instance we intentionally manipulated `_buffer` without marking it as dirty. This fixes the issue by adding a call to `MarkAsClean()`. This changeset also adds the test instructions from #15783 as a document to this repository. I've extended the list with two bugs we've found in the implementation since then. ## Validation Steps Performed * In cmd.exe, with an empty prompt in an empty directory: Pressing tab produces an audible bing and prints no text ✅ (cherry picked from commit 7a8dd90) Service-Card-Id: 91033502 Service-Version: 1.19

The initial cooked read (= conhost readline) rewrite had two flaws: * Using viewport scrolls under ConPTY to avoid emitting newlines resulted in various bugs around marks, coloring, etc. It's still somewhat unclear why this happened, but the next issue is related and much worse. * Rewriting the input line every time causes problems with accessibility tools, as they'll re-announce unchanged parts again and again. The solution to these is to simply stop writing the unchanged parts of the prompt. To do this, code was added to measure the size of text without actually inserting them into the buffer. Since this meant that the "interactive" mode of `WriteCharsLegacy` would need to be duplicated for the new code, I instead moved those parts into `COOKED_READ_DATA`. That way we can now have the interactive transform of the prompt (= Ctrl+C -> ^C) and the two text functions (measure text & actually write text) are now agnostic to this transformation. Closes microsoft#16034 Closes microsoft#16044 ## Validation Steps Performed * A vision impaired user checked it out and it seemed fine ✅

Fix UIA and marks regressions due to cooked read

66c810e

This comment has been minimized.

Sign in to view

lhecker added 2 commits October 9, 2023 18:20

Fix it even harder

59fae2e

Fix AuditMode failures

8b04c34

lhecker marked this pull request as ready for review October 9, 2023 17:00

carlos-zamora reviewed Oct 12, 2023

View reviewed changes

Fix spelling

74da0a7

DHowett approved these changes Oct 19, 2023

View reviewed changes

DHowett requested a review from zadjii-msft October 19, 2023 21:11

zadjii-msft reviewed Oct 19, 2023

View reviewed changes

zadjii-msft approved these changes Oct 20, 2023

View reviewed changes

lhecker added the Automerge-Not-Compatible label Oct 20, 2023

Use a wrapper class to track dirty state

9bb7df3

lhecker removed the Automerge-Not-Compatible label Oct 23, 2023

DHowett approved these changes Oct 23, 2023

View reviewed changes

DHowett merged commit e1c69a9 into main Oct 24, 2023
17 checks passed

DHowett deleted the dev/lhecker/16044-cooked-uia branch October 24, 2023 00:27

lhecker mentioned this pull request Nov 7, 2023

Fix tabs being printed in cmd.exe prompts #16273

Merged

adonis-jimenez mentioned this pull request Nov 18, 2023

Windows Terminal Preview v1.19.3172.0 #16337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UIA and marks regressions due to cooked read #16105

Fix UIA and marks regressions due to cooked read #16105

lhecker commented Oct 5, 2023 •

edited

Loading

This comment has been minimized.

carlos-zamora commented Oct 11, 2023

DHowett commented Oct 11, 2023

carlos-zamora Oct 11, 2023

DHowett Oct 19, 2023

carlos-zamora commented Oct 12, 2023

lhecker commented Oct 17, 2023

DHowett left a comment

zadjii-msft left a comment

zadjii-msft Oct 19, 2023

zadjii-msft Oct 19, 2023

zadjii-msft Oct 19, 2023

zadjii-msft Oct 19, 2023

zadjii-msft Oct 19, 2023

zadjii-msft left a comment

zadjii-msft Oct 20, 2023

lhecker Oct 20, 2023

zadjii-msft Oct 20, 2023

zadjii-msft Oct 20, 2023

lhecker Oct 20, 2023

lhecker Oct 23, 2023

DHowett Oct 23, 2023

lhecker Oct 23, 2023

DHowett Oct 23, 2023

lhecker Oct 23, 2023

		#define _buffer DONT_USE_ME
		#define _bufferCursor DONT_USE_ME

Fix UIA and marks regressions due to cooked read #16105

Fix UIA and marks regressions due to cooked read #16105

Conversation

lhecker commented Oct 5, 2023 • edited Loading

Validation Steps Performed

This comment has been minimized.

carlos-zamora commented Oct 11, 2023

DHowett commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Oct 12, 2023

lhecker commented Oct 17, 2023

DHowett left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Oct 5, 2023 •

edited

Loading