-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Fix UIA ScrollIntoView at EndExclusive #7868
Conversation
The change that I am going to bring to Windows: diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp
index e26415999..99589eeb0 100644
--- a/src/types/UiaTextRangeBase.cpp
+++ b/src/types/UiaTextRangeBase.cpp
@@ -796,9 +796,13 @@ try
// check if we can align to the bottom
if (static_cast<unsigned int>(endScreenInfoRow) >= viewportHeight)
{
+ // GH#7839: endScreenInfoRow may be ExclusiveEnd
+ // ExclusiveEnd is past the bottomRow
+ // so we need to clamp to the bottom row to stay in bounds
+
// we can align to bottom
- newViewport.Bottom = endScreenInfoRow;
- newViewport.Top = endScreenInfoRow - viewportHeight + 1;
+ newViewport.Bottom = std::min(endScreenInfoRow, bottomRow);
+ newViewport.Top = base::ClampedNumeric<short>(newViewport.Bottom) - viewportHeight + 1;
}
else
{ |
You accidentally a word in the description. |
@msftbot merge this in 5 minutes |
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:
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". |
`ScrollIntoView` is responsible for scrolling the viewport to include the UTR's start endpoint. The crash was caused by `start` being at the exclusive end, and attempting to scroll to it. This is now fixed by clamping the result to the bottom of the buffer. Most of the work here is to allow a test for this. `ScrollIntoView` relied on a virtual `ChangeViewport` function. By making that non-virtual, the `DummyElementProvider` in the tests can now be a `ScreenInfoUiaProviderBase`. This opens up the possibility of more UiaTextRange tests in the future too. Closes microsoft#7839
@DHowett Guessing this PR is also supposed to have a merged into Windows comment? |
You'd be guessing wrong! 😄 |
I'll let folks on this PR know what build it's available in, when it's available in a public build. |
`ScrollIntoView` is responsible for scrolling the viewport to include the UTR's start endpoint. The crash was caused by `start` being at the exclusive end, and attempting to scroll to it. This is now fixed by clamping the result to the bottom of the buffer. Most of the work here is to allow a test for this. `ScrollIntoView` relied on a virtual `ChangeViewport` function. By making that non-virtual, the `DummyElementProvider` in the tests can now be a `ScreenInfoUiaProviderBase`. This opens up the possibility of more UiaTextRange tests in the future too. Closes #7839 (cherry picked from commit 7a1932c)
🎉 Handy links: |
🎉 Handy links: |
ScrollIntoView
is responsible for scrolling the viewport to includethe UTR's start endpoint. The crash was caused by
start
being at theexclusive end, and attempting to scroll to it. This is now fixed by
clamping the result to the bottom of the buffer.
Most of the work here is to allow a test for this.
ScrollIntoView
relied on a virtual
ChangeViewport
function. By making thatnon-virtual, the
DummyElementProvider
in the tests can now be aScreenInfoUiaProviderBase
. This opens up the possibility of moreUiaTextRange tests in the future too.
Closes #7839