From 7b39d24913ec9a6b9fd7026bfd47dc4c7363ccdf Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 20 Aug 2024 15:58:05 -0700 Subject: [PATCH] Fix UIA RangeFromPoint API (#17695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Fixes the `RangeFromPoint` API such that we're now properly locking when we attempt to retrieve the viewport data. This also corrects the conversion from `UiaPoint` (screen position) to buffer coordinates (buffer cell). Closes #17579 ## Detailed Description of the Pull Request / Additional comments - `UiaTextRangeBase::Initialize(UiaPoint)`: - reordered logic to clamp to client area first, then begin conversion to buffer coordinates - properly lock when retrieving the viewport data - updated `_TranslatePointToScreen` and `_TranslatePointFromScreen` to use `&` instead of `*` - we weren't properly updating the parameter before - `TermControlUiaTextRange::_TranslatePointFromScreen()` - `includeOffsets` was basically copied over from `_TranslatePointToScreen`. The math itself was straight up wrong since we had to do it backwards. ## Validation Steps Performed ✅ Moved WT to top-left of monitor, then used inspect.exe to call `RangeFromPoint` API when mouse cursor is on top-left buffer cell (also meticulously stepped through the two functions ensuring everything was correct). --- src/interactivity/win32/uiaTextRange.cpp | 8 ++--- src/interactivity/win32/uiaTextRange.hpp | 4 +-- src/types/TermControlUiaTextRange.cpp | 18 +++++----- src/types/TermControlUiaTextRange.hpp | 4 +-- src/types/UiaTextRangeBase.cpp | 44 ++++++++++-------------- src/types/UiaTextRangeBase.hpp | 4 +-- 6 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/interactivity/win32/uiaTextRange.cpp b/src/interactivity/win32/uiaTextRange.cpp index eb78d090cf..028e181b38 100644 --- a/src/interactivity/win32/uiaTextRange.cpp +++ b/src/interactivity/win32/uiaTextRange.cpp @@ -69,14 +69,14 @@ IFACEMETHODIMP UiaTextRange::Clone(_Outptr_result_maybenull_ ITextRangeProvider* return S_OK; } -void UiaTextRange::_TranslatePointToScreen(til::point* clientPoint) const +void UiaTextRange::_TranslatePointToScreen(til::point& clientPoint) const { - ClientToScreen(_getWindowHandle(), clientPoint->as_win32_point()); + ClientToScreen(_getWindowHandle(), clientPoint.as_win32_point()); } -void UiaTextRange::_TranslatePointFromScreen(til::point* screenPoint) const +void UiaTextRange::_TranslatePointFromScreen(til::point& screenPoint) const { - ScreenToClient(_getWindowHandle(), screenPoint->as_win32_point()); + ScreenToClient(_getWindowHandle(), screenPoint.as_win32_point()); } HWND UiaTextRange::_getWindowHandle() const diff --git a/src/interactivity/win32/uiaTextRange.hpp b/src/interactivity/win32/uiaTextRange.hpp index 7ea79364d5..84f8b5a539 100644 --- a/src/interactivity/win32/uiaTextRange.hpp +++ b/src/interactivity/win32/uiaTextRange.hpp @@ -56,8 +56,8 @@ namespace Microsoft::Console::Interactivity::Win32 IFACEMETHODIMP Clone(_Outptr_result_maybenull_ ITextRangeProvider** ppRetVal) override; protected: - void _TranslatePointToScreen(til::point* clientPoint) const override; - void _TranslatePointFromScreen(til::point* screenPoint) const override; + void _TranslatePointToScreen(til::point& clientPoint) const override; + void _TranslatePointFromScreen(til::point& screenPoint) const override; private: HWND _getWindowHandle() const; diff --git a/src/types/TermControlUiaTextRange.cpp b/src/types/TermControlUiaTextRange.cpp index 6f3c82e818..5b26dd5189 100644 --- a/src/types/TermControlUiaTextRange.cpp +++ b/src/types/TermControlUiaTextRange.cpp @@ -73,7 +73,7 @@ IFACEMETHODIMP TermControlUiaTextRange::Clone(_Outptr_result_maybenull_ ITextRan // (0,0) is the top-left of the app window // Return Value: // - -void TermControlUiaTextRange::_TranslatePointToScreen(til::point* clientPoint) const +void TermControlUiaTextRange::_TranslatePointToScreen(til::point& clientPoint) const { const gsl::not_null provider = static_cast(_pProvider); @@ -96,8 +96,8 @@ void TermControlUiaTextRange::_TranslatePointToScreen(til::point* clientPoint) c // Get scale factor for display const auto scaleFactor = provider->GetScaleFactor(); - clientPoint->x = includeOffsets(clientPoint->x, boundingRect.left, padding.left, scaleFactor); - clientPoint->y = includeOffsets(clientPoint->y, boundingRect.top, padding.top, scaleFactor); + clientPoint.x = includeOffsets(clientPoint.x, boundingRect.left, padding.left, scaleFactor); + clientPoint.y = includeOffsets(clientPoint.y, boundingRect.top, padding.top, scaleFactor); } // Method Description: @@ -107,15 +107,13 @@ void TermControlUiaTextRange::_TranslatePointToScreen(til::point* clientPoint) c // (0,0) is the top-left of the screen // Return Value: // - -void TermControlUiaTextRange::_TranslatePointFromScreen(til::point* screenPoint) const +void TermControlUiaTextRange::_TranslatePointFromScreen(til::point& screenPoint) const { const gsl::not_null provider = static_cast(_pProvider); const auto includeOffsets = [](long screenPos, double termControlPos, double padding, double scaleFactor) { - auto result = base::ClampedNumeric(padding); - // only the padding is in DIPs now - result /= scaleFactor; - result -= screenPos; + auto result = base::ClampedNumeric(screenPos); + result -= padding / scaleFactor; result -= termControlPos; return result; }; @@ -130,8 +128,8 @@ void TermControlUiaTextRange::_TranslatePointFromScreen(til::point* screenPoint) // Get scale factor for display const auto scaleFactor = provider->GetScaleFactor(); - screenPoint->x = includeOffsets(screenPoint->x, boundingRect.left, padding.left, scaleFactor); - screenPoint->y = includeOffsets(screenPoint->y, boundingRect.top, padding.top, scaleFactor); + screenPoint.x = includeOffsets(screenPoint.x, boundingRect.left, padding.left, scaleFactor); + screenPoint.y = includeOffsets(screenPoint.y, boundingRect.top, padding.top, scaleFactor); } til::size TermControlUiaTextRange::_getScreenFontSize() const noexcept diff --git a/src/types/TermControlUiaTextRange.hpp b/src/types/TermControlUiaTextRange.hpp index 70ba9cac97..e619583a9e 100644 --- a/src/types/TermControlUiaTextRange.hpp +++ b/src/types/TermControlUiaTextRange.hpp @@ -56,8 +56,8 @@ namespace Microsoft::Terminal IFACEMETHODIMP Clone(_Outptr_result_maybenull_ ITextRangeProvider** ppRetVal) override; protected: - void _TranslatePointToScreen(til::point* clientPoint) const override; - void _TranslatePointFromScreen(til::point* screenPoint) const override; + void _TranslatePointToScreen(til::point& clientPoint) const override; + void _TranslatePointFromScreen(til::point& screenPoint) const override; til::size _getScreenFontSize() const noexcept override; }; } diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 8e83ef47aa..0ce958e698 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -97,30 +97,24 @@ CATCH_RETURN(); void UiaTextRangeBase::Initialize(_In_ const UiaPoint point) { - til::point clientPoint; - clientPoint.x = static_cast(point.x); - clientPoint.y = static_cast(point.y); - // get row that point resides in - const auto windowRect = _getTerminalRect(); - const auto viewport = _pData->GetViewport().ToInclusive(); - til::CoordType row = 0; - if (clientPoint.y <= windowRect.top) - { - row = viewport.top; - } - else if (clientPoint.y >= windowRect.bottom) - { - row = viewport.bottom; - } - else - { - // change point coords to pixels relative to window - _TranslatePointFromScreen(&clientPoint); + til::point clientPoint{ static_cast(point.x), static_cast(point.y) }; - const auto currentFontSize = _getScreenFontSize(); - row = clientPoint.y / currentFontSize.height + viewport.top; - } - _start = { 0, row }; + // clamp the point to be within the client area + const auto windowRect = _getTerminalRect(); + clientPoint.x = std::clamp(clientPoint.x, windowRect.left, windowRect.right); + clientPoint.y = std::clamp(clientPoint.y, windowRect.top, windowRect.bottom); + + // convert point to be relative to client area + _TranslatePointFromScreen(clientPoint); + + // convert the point to screen buffer coordinates + _pData->LockConsole(); + const auto currentFontSize = _getScreenFontSize(); + const auto viewport = _pData->GetViewport().ToInclusive(); + _pData->UnlockConsole(); + + _start = { clientPoint.x / currentFontSize.width + viewport.left, + clientPoint.y / currentFontSize.height + viewport.top }; _end = _start; } @@ -1381,8 +1375,8 @@ void UiaTextRangeBase::_getBoundingRect(const til::rect& textRect, _Inout_ std:: // convert the coords to be relative to the screen instead of // the client window - _TranslatePointToScreen(&topLeft); - _TranslatePointToScreen(&bottomRight); + _TranslatePointToScreen(topLeft); + _TranslatePointToScreen(bottomRight); const long width = bottomRight.x - topLeft.x; const long height = bottomRight.y - topLeft.y; diff --git a/src/types/UiaTextRangeBase.hpp b/src/types/UiaTextRangeBase.hpp index eec30a3dcb..2b36ccc0ef 100644 --- a/src/types/UiaTextRangeBase.hpp +++ b/src/types/UiaTextRangeBase.hpp @@ -123,8 +123,8 @@ namespace Microsoft::Console::Types std::wstring _wordDelimiters{}; ::Search _searcher; - virtual void _TranslatePointToScreen(til::point* clientPoint) const = 0; - virtual void _TranslatePointFromScreen(til::point* screenPoint) const = 0; + virtual void _TranslatePointToScreen(til::point& clientPoint) const = 0; + virtual void _TranslatePointFromScreen(til::point& screenPoint) const = 0; void Initialize(_In_ const UiaPoint point);