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

Revert "Remove most uses of CompareInBounds (#13244)" #13907

Merged
1 commit merged into from
Sep 2, 2022

Conversation

carlos-zamora
Copy link
Member

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

@ghost ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Blocking We won't ship a release like this! No-siree. labels Sep 1, 2022
@carlos-zamora
Copy link
Member Author

FYI @DHowett

@LeonarddeR
Copy link

thanks a lot @carlos-zamora! What would be the best way to test this?

@codeofdusk
Copy link
Contributor

codeofdusk commented Sep 2, 2022

@LeonarddeR The simplest way that I've found to test terminal PRs:

  1. Click "details" under "Terminal CI (Build x64 Build x64 Release Dev) " in the CI results.
  2. On the page that opens, click "View more details on Azure Pipelines".
  3. Search the page for "artifacts" (note spelled with an i). You'll find a link labelled n artifacts produced where n is some number. Click that link.
  4. This page is a bit fiddly. Find the table on the page, then switch to browse mode. Down arrow through the rows until you reach one labelled "drop", then hit right arrow.
  5. In the menu that opens, down arrow to "download artifact" and press enter.
  6. Extract the zip you downloaded. Inside the drop\appx folder, you'll find a CascadiaPackage_0.0.1.0_x64.msix. Rename it to a .zip and extract.
  7. From that directory, run WindowsTerminal.exe to test wt, or OpenConsole.exe to test conhost.

@zadjii-msft
Copy link
Member

@carlos-zamora moving the cursor around in a WSL nano or vim session doesn't explode after this, right (ala #13183)? I suppose that was fixed in #13250, not #13866...

@lhecker
Copy link
Member

lhecker commented Sep 2, 2022

Wait, but how am I supposed to remove the Viewport class in my upcoming commits then. ._.

To be precise, I don't see how this

bufferSize.CompareInBounds(A, B, true) <=> 0

is any different mathematically from this

A <=> B

since both do the exact same thing.

@carlos-zamora
Copy link
Member Author

Wait, but how am I supposed to remove the Viewport class in my upcoming commits then. ._.

To be precise, I don't see how this

bufferSize.CompareInBounds(A, B, true) <=> 0

is any different mathematically from this

A <=> B

since both do the exact same thing.

Replacing CompareInBounds() with <=> is fine.

The regression was mainly caused by the IsInBounds() check because endExclusive was no longer accepted after #13244 went through. I can create a separate commit solely replacing CompareInBounds() if you'd like, but I figured reverting #13244 entirely would be the safest thing to do for 1.15/1.16 since we're approaching release. You could also just replace CompareInBounds() in your PR yourself, up to you haha

@carlos-zamora
Copy link
Member Author

@msftbot merge this in 10 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 2, 2022
@ghost
Copy link

ghost commented Sep 2, 2022

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Fri, 02 Sep 2022 21:24:31 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit bfd5248 into main Sep 2, 2022
@ghost ghost deleted the dev/cazamor/a11y/revert-13244 branch September 2, 2022 21:25
DHowett pushed a commit that referenced this pull request 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

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

Handy links:

@ghost
Copy link

ghost commented Sep 13, 2022

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

Handy links:

This pull request 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 AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility: NVDA emits errors in WT1.15 preview
5 participants