Skip to content

Commit

Permalink
Fix UIA ScrollIntoView at EndExclusive (microsoft#7868)
Browse files Browse the repository at this point in the history
`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
  • Loading branch information
carlos-zamora authored and skyline75489 committed Oct 10, 2020
1 parent 1014d3a commit 80a72dc
Show file tree
Hide file tree
Showing 7 changed files with 89 additions and 37 deletions.
6 changes: 0 additions & 6 deletions src/interactivity/win32/uiaTextRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<ScreenInfoUiaProvider*>(_pProvider);
provider->ChangeViewport(NewWindow);
}

void UiaTextRange::_TranslatePointToScreen(LPPOINT clientPoint) const
{
ClientToScreen(_getWindowHandle(), clientPoint);
Expand Down
1 change: 0 additions & 1 deletion src/interactivity/win32/uiaTextRange.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "CommonState.hpp"

#include "uiaTextRange.hpp"
#include "../types/ScreenInfoUiaProviderBase.h"
#include "../../../buffer/out/textBuffer.hpp"

using namespace WEX::Common;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<ScrollTest> 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<UiaTextRange> 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<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, pos, pos));
VERIFY_SUCCEEDED(utr->ScrollIntoView(alignToTop));
}
}
};
6 changes: 0 additions & 6 deletions src/types/TermControlUiaTextRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<TermControlUiaProvider*> provider = static_cast<TermControlUiaProvider*>(_pProvider);
provider->ChangeViewport(NewWindow);
}

// Method Description:
// - Transform coordinates relative to the client to relative to the screen
// Arguments:
Expand Down
1 change: 0 additions & 1 deletion src/types/TermControlUiaTextRange.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 9 additions & 4 deletions src/types/UiaTextRangeBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{
Expand All @@ -815,7 +819,8 @@ try

Unlock.reset();

_ChangeViewport(newViewport);
const gsl::not_null<ScreenInfoUiaProviderBase*> provider = static_cast<ScreenInfoUiaProviderBase*>(_pProvider);
provider->ChangeViewport(newViewport);

UiaTracing::TextRange::ScrollIntoView(alignToTop, *this);
return S_OK;
Expand Down
1 change: 0 additions & 1 deletion src/types/UiaTextRangeBase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit 80a72dc

Please sign in to comment.