Skip to content
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.

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

Accessibility: NVDA emits errors in WT1.15 preview #13866

Closed
LeonarddeR opened this issue Aug 28, 2022 · 6 comments · Fixed by #13907
Closed

Accessibility: NVDA emits errors in WT1.15 preview #13866

LeonarddeR opened this issue Aug 28, 2022 · 6 comments · Fixed by #13907
Assignees
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.

Comments

@LeonarddeR
Copy link

Windows Terminal version

1.15.2283.0

Windows build number

25188.1000

Other Software

NVDA 2022.2.2

Steps to reproduce

  1. Open bash in windows terminal
  2. type nano /etc/hosts

Expected Behavior

No error in NVDA

Actual Behavior

NVDA emits the following error:

ERROR - NVDAObjects.behaviors.LiveText._monitor (18:21:06.798) - Dynamic_WinTerminalUIAEditableTextWithAutoSelectDetectionUIA._monitorThread (22344):
Error getting or calculating new text
Traceback (most recent call last):
  File "NVDAObjects\behaviors.pyc", line 386, in _monitor
  File "NVDAObjects\behaviors.pyc", line 350, in _getText
  File "diffHandler.pyc", line 62, in _getText
  File "baseObject.pyc", line 26, in __get__
  File "NVDAObjects\UIA\__init__.pyc", line 848, in _get_text
  File "NVDAObjects\UIA\__init__.pyc", line 555, in _getTextFromUIARange
  File "monkeyPatches\comtypesMonkeyPatches.pyc", line 32, in __call__
_ctypes.COMError: (-2147467259, 'Unspecified error', (None, None, None, 0, None))

I wasn't able to reproduce this with WT1.14, so therefore I'm filing this here since there might be a regression in UIA related code. @codeofdusk could you reproduce this?

@LeonarddeR LeonarddeR added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Aug 28, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 28, 2022
@zadjii-msft zadjii-msft added the Area-Accessibility Issues related to accessibility label Aug 29, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.16 milestone Aug 29, 2022
@codeofdusk
Copy link
Contributor

I've also seen this, but not with wtNotifications.

This is probably related to broken assumptions when switching to/from the alt buffer.

@LeonarddeR
Copy link
Author

This error indeed has nothing to do with the notifications implementation.

CC @carlos-zamora just want to make sure that this bug doesn't end up in a release, since that would be annoying for many sr users.

@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Sep 1, 2022
@carlos-zamora
Copy link
Member

@LeonarddeR (or @codeofdusk since you're familiar with the space) I think I need a little more context here. It looks like NVDA is calling the UIA API GetText() off of a TextRange. That corresponds to the following code in Windows Terminal:

IFACEMETHODIMP UiaTextRangeBase::GetText(_In_ int maxLength, _Out_ BSTR* pRetVal) noexcept
try
{
RETURN_HR_IF_NULL(E_INVALIDARG, pRetVal);
RETURN_HR_IF(E_INVALIDARG, maxLength < -1);
*pRetVal = nullptr;
_pData->LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
});
RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized());
const auto text = _getTextValue(maxLength);
Unlock.reset();
*pRetVal = SysAllocString(text.c_str());
RETURN_HR_IF_NULL(E_OUTOFMEMORY, *pRetVal);
UiaTracing::TextRange::GetText(*this, maxLength, text);
return S_OK;
}
CATCH_RETURN();
// Method Description:
// - Helper method for GetText(). Retrieves the text that the UiaTextRange encompasses as a wstring
// Arguments:
// - maxLength - the maximum size of the retrieved text. nullopt means we don't care about the size.
// Return Value:
// - the text that the UiaTextRange encompasses
#pragma warning(push)
#pragma warning(disable : 26447) // compiler isn't filtering throws inside the try/catch
std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const
{
std::wstring textData{};
if (!IsDegenerate())
{
const auto& buffer = _pData->GetTextBuffer();
const auto bufferSize = buffer.GetSize();
// TODO GH#5406: create a different UIA parent object for each TextBuffer
// nvaccess/nvda#11428: Ensure our endpoints are in bounds
// otherwise, we'll FailFast catastrophically
THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));
// convert _end to be inclusive
auto inclusiveEnd = _end;
bufferSize.DecrementInBounds(inclusiveEnd, true);
const auto textRects = buffer.GetTextRects(_start, inclusiveEnd, _blockRange, true);
const auto bufferData = buffer.GetText(true,
false,
textRects);
const size_t textDataSize = bufferData.text.size() * bufferSize.Width();
textData.reserve(textDataSize);
for (const auto& text : bufferData.text)
{
textData += text;
}
}
if (maxLength >= 0)
{
textData.resize(maxLength);
}
return textData;
}
#pragma warning(pop)

It looks like we emit an error if...

  1. the second parameter (the out param) wasn't provided properly
  2. the first parameter (dictating the max length) is less than -1
  3. the newly created terminal content isn't fully initialized yet
  4. we're out of memory and we can't create the string result to return
  5. (as @codeofdusk said) the text range points to content that doesn't exist (this is a quick-n-easy way to invalidate a text range that was on the alt buffer when we're now on the main buffer)

I assume the main culprit here is number 5, but to be certain, could you provide the following:

  • What UIA APIs was NVDA calling? Obviously GetText() is the one that returns an error, but what were the calls leading up to that.
  • If you can explain how I can get a trace from NVDA, that'd be even better.

@carlos-zamora
Copy link
Member

Yeah, that error code is E_FAIL so it's 100% that alt buffer thing.

@ghost ghost added the In-PR This issue has a related PR label Sep 1, 2022
@ghost ghost closed this as completed in #13907 Sep 2, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 2, 2022
ghost pushed a commit that referenced this issue Sep 2, 2022
This reverts commit f785168 (PR #13244)

The error logged to NVDA was caused by the following line of code in `_getTextValue()`:
`THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));`
NVDA would expand a text range to encompass the document in the alt buffer. This means that the "end" would be set to the dangling "endExclusive" point (x = left, y = one past the end of the buffer). This is a valid range!
However, upon extracting the text, we would hit the code above. The exclusive end doesn't actually point to anything in the buffer, so we would falsly throw `E_FAIL`.

Though this could be fixed by adding a special check for the `endExclusive` in the line above, I suspect there are other places throughout the UIA code that hit this problem too. The safest course of action is to revert this commit entirely since it was a code health commit (it doesn't actually close an issue).

Closes #13866
DHowett pushed a commit that referenced this issue Sep 6, 2022
This reverts commit f785168 (PR #13244)

The error logged to NVDA was caused by the following line of code in `_getTextValue()`:
`THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));`
NVDA would expand a text range to encompass the document in the alt buffer. This means that the "end" would be set to the dangling "endExclusive" point (x = left, y = one past the end of the buffer). This is a valid range!
However, upon extracting the text, we would hit the code above. The exclusive end doesn't actually point to anything in the buffer, so we would falsly throw `E_FAIL`.

Though this could be fixed by adding a special check for the `endExclusive` in the line above, I suspect there are other places throughout the UIA code that hit this problem too. The safest course of action is to revert this commit entirely since it was a code health commit (it doesn't actually close an issue).

Closes #13866

(cherry picked from commit bfd5248)
Service-Card-Id: 85405833
Service-Version: 1.15
@ghost
Copy link

ghost commented Sep 13, 2022

🎉This issue was addressed in #13907, which has now been successfully released as Windows Terminal v1.15.252.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 13, 2022

🎉This issue was addressed in #13907, which has now been successfully released as Windows Terminal Preview v1.16.252.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants