From 62d22c2a57be53e5b444ce0a3337f7a704f69fe1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 9 Oct 2020 13:27:13 -0700 Subject: [PATCH] Fix UIA ScrollIntoView at EndExclusive (#7868) `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 7a1932c5568057bb25e6bf26a628ad3d511506ce) --- src/interactivity/win32/uiaTextRange.cpp | 6 -- src/interactivity/win32/uiaTextRange.hpp | 1 - .../UiaTextRangeTests.cpp | 98 +++++++++++++++---- src/types/TermControlUiaTextRange.cpp | 6 -- src/types/TermControlUiaTextRange.hpp | 1 - src/types/UiaTextRangeBase.cpp | 13 ++- src/types/UiaTextRangeBase.hpp | 1 - 7 files changed, 89 insertions(+), 37 deletions(-) diff --git a/src/interactivity/win32/uiaTextRange.cpp b/src/interactivity/win32/uiaTextRange.cpp index ba3a0109d59..a33957bfc92 100644 --- a/src/interactivity/win32/uiaTextRange.cpp +++ b/src/interactivity/win32/uiaTextRange.cpp @@ -77,12 +77,6 @@ IFACEMETHODIMP UiaTextRange::Clone(_Outptr_result_maybenull_ ITextRangeProvider* return S_OK; } -void UiaTextRange::_ChangeViewport(const SMALL_RECT NewWindow) -{ - auto provider = static_cast(_pProvider); - provider->ChangeViewport(NewWindow); -} - void UiaTextRange::_TranslatePointToScreen(LPPOINT clientPoint) const { ClientToScreen(_getWindowHandle(), clientPoint); diff --git a/src/interactivity/win32/uiaTextRange.hpp b/src/interactivity/win32/uiaTextRange.hpp index 5d83f01e8de..056e013181a 100644 --- a/src/interactivity/win32/uiaTextRange.hpp +++ b/src/interactivity/win32/uiaTextRange.hpp @@ -56,7 +56,6 @@ namespace Microsoft::Console::Interactivity::Win32 IFACEMETHODIMP Clone(_Outptr_result_maybenull_ ITextRangeProvider** ppRetVal) override; protected: - void _ChangeViewport(const SMALL_RECT NewWindow) override; void _TranslatePointToScreen(LPPOINT clientPoint) const override; void _TranslatePointFromScreen(LPPOINT screenPoint) const override; diff --git a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp index 9faff4c0a64..e390201f810 100644 --- a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp +++ b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp @@ -7,6 +7,7 @@ #include "CommonState.hpp" #include "uiaTextRange.hpp" +#include "../types/ScreenInfoUiaProviderBase.h" #include "../../../buffer/out/textBuffer.hpp" using namespace WEX::Common; @@ -24,46 +25,65 @@ using namespace Microsoft::Console::Interactivity::Win32; // it not doing anything for its implementation because it is not used // during the unit tests below. -class DummyElementProvider final : public IRawElementProviderSimple +class DummyElementProvider final : public ScreenInfoUiaProviderBase { public: - // IUnknown methods - IFACEMETHODIMP_(ULONG) - AddRef() + IFACEMETHODIMP Navigate(_In_ NavigateDirection /*direction*/, + _COM_Outptr_result_maybenull_ IRawElementProviderFragment** /*ppProvider*/) override { - return 1; + return E_NOTIMPL; } - IFACEMETHODIMP_(ULONG) - Release() + IFACEMETHODIMP get_BoundingRectangle(_Out_ UiaRect* /*pRect*/) override { - return 1; + return E_NOTIMPL; } - IFACEMETHODIMP QueryInterface(_In_ REFIID /*riid*/, - _COM_Outptr_result_maybenull_ void** /*ppInterface*/) + + IFACEMETHODIMP get_FragmentRoot(_COM_Outptr_result_maybenull_ IRawElementProviderFragmentRoot** /*ppProvider*/) override { return E_NOTIMPL; - }; + } + + void ChangeViewport(const SMALL_RECT /*NewWindow*/) + { + return; + } - // IRawElementProviderSimple methods - IFACEMETHODIMP get_ProviderOptions(_Out_ ProviderOptions* /*pOptions*/) + HRESULT GetSelectionRange(_In_ IRawElementProviderSimple* /*pProvider*/, const std::wstring_view /*wordDelimiters*/, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** /*ppUtr*/) override { return E_NOTIMPL; } - IFACEMETHODIMP GetPatternProvider(_In_ PATTERNID /*iid*/, - _COM_Outptr_result_maybenull_ IUnknown** /*ppInterface*/) + // degenerate range + HRESULT CreateTextRange(_In_ IRawElementProviderSimple* const /*pProvider*/, const std::wstring_view /*wordDelimiters*/, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** /*ppUtr*/) override { return E_NOTIMPL; } - IFACEMETHODIMP GetPropertyValue(_In_ PROPERTYID /*idProp*/, - _Out_ VARIANT* /*pVariant*/) + // degenerate range at cursor position + HRESULT CreateTextRange(_In_ IRawElementProviderSimple* const /*pProvider*/, + const Cursor& /*cursor*/, + const std::wstring_view /*wordDelimiters*/, + _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** /*ppUtr*/) override { return E_NOTIMPL; } - IFACEMETHODIMP get_HostRawElementProvider(_COM_Outptr_result_maybenull_ IRawElementProviderSimple** /*ppProvider*/) + // specific endpoint range + HRESULT CreateTextRange(_In_ IRawElementProviderSimple* const /*pProvider*/, + const COORD /*start*/, + const COORD /*end*/, + const std::wstring_view /*wordDelimiters*/, + _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** /*ppUtr*/) override + { + return E_NOTIMPL; + } + + // range from a UiaPoint + HRESULT CreateTextRange(_In_ IRawElementProviderSimple* const /*pProvider*/, + const UiaPoint /*point*/, + const std::wstring_view /*wordDelimiters*/, + _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** /*ppUtr*/) override { return E_NOTIMPL; } @@ -106,6 +126,12 @@ class UiaTextRangeTests ExpectedResult expected; }; + struct ScrollTest + { + std::wstring comment; + short yPos; + }; + static constexpr wchar_t* toString(TextUnit unit) noexcept { // if a format is not supported, it goes to the next largest text unit @@ -1284,4 +1310,40 @@ class UiaTextRangeTests THROW_IF_FAILED(utr->GetText(-1, &text)); VERIFY_ARE_EQUAL(L"M", std::wstring_view{ text }); } + + TEST_METHOD(ScrollIntoView) + { + const auto bufferSize{ _pTextBuffer->GetSize() }; + const auto viewportSize{ _pUiaData->GetViewport() }; + + const std::vector testData{ + { L"Origin", bufferSize.Top() }, + { L"ViewportHeight From Top - 1", bufferSize.Top() + viewportSize.Height() - 1 }, + { L"ViewportHeight From Top", bufferSize.Top() + viewportSize.Height() }, + { L"ViewportHeight From Top + 1", bufferSize.Top() + viewportSize.Height() + 1 }, + { L"ViewportHeight From Bottom - 1", bufferSize.BottomInclusive() - viewportSize.Height() - 1 }, + { L"ViewportHeight From Bottom", bufferSize.BottomInclusive() - viewportSize.Height() }, + { L"ViewportHeight From Bottom + 1", bufferSize.BottomInclusive() - viewportSize.Height() + 1 }, + + // GH#7839: ExclusiveEnd is a non-existent space, + // so scrolling to it when !alignToTop used to crash + { L"Exclusive End", bufferSize.BottomExclusive() } + }; + + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:alignToTop", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + bool alignToTop; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"alignToTop", alignToTop), L"Get alignToTop variant"); + + Microsoft::WRL::ComPtr utr; + for (const auto test : testData) + { + Log::Comment(test.comment.c_str()); + const til::point pos{ bufferSize.Left(), test.yPos }; + THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize(&utr, _pUiaData, &_dummyProvider, pos, pos)); + VERIFY_SUCCEEDED(utr->ScrollIntoView(alignToTop)); + } + } }; diff --git a/src/types/TermControlUiaTextRange.cpp b/src/types/TermControlUiaTextRange.cpp index e2bfa950407..ba512888552 100644 --- a/src/types/TermControlUiaTextRange.cpp +++ b/src/types/TermControlUiaTextRange.cpp @@ -80,12 +80,6 @@ IFACEMETHODIMP TermControlUiaTextRange::Clone(_Outptr_result_maybenull_ ITextRan return S_OK; } -void TermControlUiaTextRange::_ChangeViewport(const SMALL_RECT NewWindow) -{ - const gsl::not_null provider = static_cast(_pProvider); - provider->ChangeViewport(NewWindow); -} - // Method Description: // - Transform coordinates relative to the client to relative to the screen // Arguments: diff --git a/src/types/TermControlUiaTextRange.hpp b/src/types/TermControlUiaTextRange.hpp index 696b876dff4..82c798431c6 100644 --- a/src/types/TermControlUiaTextRange.hpp +++ b/src/types/TermControlUiaTextRange.hpp @@ -55,7 +55,6 @@ namespace Microsoft::Terminal IFACEMETHODIMP Clone(_Outptr_result_maybenull_ ITextRangeProvider** ppRetVal) override; protected: - void _ChangeViewport(const SMALL_RECT NewWindow) override; void _TranslatePointToScreen(LPPOINT clientPoint) const override; void _TranslatePointFromScreen(LPPOINT screenPoint) const override; const COORD _getScreenFontSize() const override; diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index e264159997d..99589eeb0c4 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -784,7 +784,7 @@ try } else { - // we can align to the top so we'll just move the viewport + // we can't align to the top so we'll just move the viewport // to the bottom of the screen buffer newViewport.Bottom = bottomRow; newViewport.Top = bottomRow - viewportHeight + 1; @@ -796,9 +796,13 @@ try // check if we can align to the bottom if (static_cast(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(newViewport.Bottom) - viewportHeight + 1; } else { @@ -815,7 +819,8 @@ try Unlock.reset(); - _ChangeViewport(newViewport); + const gsl::not_null provider = static_cast(_pProvider); + provider->ChangeViewport(newViewport); UiaTracing::TextRange::ScrollIntoView(alignToTop, *this); return S_OK; diff --git a/src/types/UiaTextRangeBase.hpp b/src/types/UiaTextRangeBase.hpp index ab62200e152..16b1e4be3d0 100644 --- a/src/types/UiaTextRangeBase.hpp +++ b/src/types/UiaTextRangeBase.hpp @@ -127,7 +127,6 @@ namespace Microsoft::Console::Types std::wstring _wordDelimiters{}; - virtual void _ChangeViewport(const SMALL_RECT NewWindow) = 0; virtual void _TranslatePointToScreen(LPPOINT clientPoint) const = 0; virtual void _TranslatePointFromScreen(LPPOINT screenPoint) const = 0;