Fix A11y EndExclusive Error for Move & Expand #7677
Conversation
ghost
added
Area-Accessibility
|
|
Build veyr mad |
ghost
added
Needs-Author-Feedback
|
CC @Simon818 |
| @@ -2065,7 +2065,7 @@ void TextBufferTests::GetWordBoundaries() | |||
| { { 79, 0 }, {{ 10, 0 }, { 5, 0 }} }, | |||
|
|
|||
| // tests for second line of text | |||
| { { 0, 1 }, {{ 0, 1 }, { 0, 1 }} }, | |||
| { { 0, 1 }, {{ 0, 1 }, { 5, 0 }} }, | |||
There was a problem hiding this comment.
I'm not sure which of the code changes made this change. Is this good and righteous? It seems like it is because a11y defines the word to be the word + all the spaces after it, so you're inside the spaces at 0,1... so the word starts at 5,0... but why was this broken before?
There was a problem hiding this comment.
This one was a mistake on my part back when I wrote these tests. You're right to be concerned but here's my reasoning on why this is the right fix:
You have the following text:
word other
more words
You're current position is where the X is (0,1):
word other
X more words
When we get the previous word, we're supposed to wrap around to encompass "other". Before, on accident, we would hit the left boundary and not wrap. So we would actually return the whitespace on the second line between the left boundary and "more".
There was a problem hiding this comment.
how did this test not fail then? o_O
There was a problem hiding this comment.
The test didn't fail because I wrote it incorrectly. It used to expect the whitespace. It should have expected the word on the previous line.
There was a problem hiding this comment.
I can't figure out how to permalink code, but you want to look at textbuffer.cpp line 1000
if (target.X == GetSize().Left())We were treating "Left" as "Origin" and refusing to expand left.
carlos-zamora
added
the
AutoMerge
|
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 (
|
ghost
merged commit
@carlos-zamora presumably in a follow-up PR? |
Just a thought to play around with. This could be a part of #6453, since we'll hit that case often then. |
|
This fix was just released as part of Windows in Insider Build 20236! |
`EndExclusive` represents the end of the buffer. This is designed to not point to any data on the buffer. UiaTextRange would point to this `EndExclusive` and then attempt to move based on it. However, since it does not point to any data, it could experience undefined behavior or (inevitably) crash from running out of bounds. This PR specifically checks for expansion and movement at that point, and prevents us from moving beyond it. There are plans in the future to define the "end" as the last character in the buffer. Until then, this solution will suffice and provide correct behavior that doesn't crash. ## Validation Steps Performed Performed the referenced bugs' repro steps and added test coverage. Closes MSFT-20458595 Closes #7663 Closes #7664 (cherry picked from commit 40893b2)
In nvaccess/nvda#11428 (comment), Andre9642 reported a Conhost crash when switching to/from the alt buffer a few times with a Braille display connected. Upon further investigation, @carlos-zamora and I discovered that the FailFast was in `GetText`: more checks similar to #7677 were needed for this case. Tested with NVDA using a [Focus](https://www.freedomscientific.com/products/blindness/focus40brailledisplay/) Braille display. Improves nvaccess/nvda#11428
In nvaccess/nvda#11428 (comment), Andre9642 reported a Conhost crash when switching to/from the alt buffer a few times with a Braille display connected. Upon further investigation, @carlos-zamora and I discovered that the FailFast was in `GetText`: more checks similar to #7677 were needed for this case. Tested with NVDA using a [Focus](https://www.freedomscientific.com/products/blindness/focus40brailledisplay/) Braille display. Improves nvaccess/nvda#11428 (cherry picked from commit 60437b8)
|
🎉 Handy links: |
|
🎉 Handy links: |
EndExclusiverepresents the end of the buffer. This is designed to notpoint to any data on the buffer. UiaTextRange would point to this
EndExclusiveand then attempt to move based on it. However, since itdoes not point to any data, it could experience undefined behavior or
(inevitably) crash from running out of bounds.
This PR specifically checks for expansion and movement at that point,
and prevents us from moving beyond it. There are plans in the future to
define the "end" as the last character in the buffer. Until then, this
solution will suffice and provide correct behavior that doesn't crash.
Validation Steps Performed
Performed the referenced bugs' repro steps and added test coverage.
Closes MSFT-20458595
Closes #7663
Closes #7664