Skip to content

Commit

Permalink
Fix UIA RangeFromPoint API (#17695)
Browse files Browse the repository at this point in the history
## 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).

(cherry picked from commit 7b39d24)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSC930 PVTI_lADOAF3p4s4AmhmszgSCpCs
Service-Version: 1.21
  • Loading branch information
carlos-zamora authored and DHowett committed Aug 21, 2024
1 parent 2890e8a commit e866a6c
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 43 deletions.
8 changes: 4 additions & 4 deletions src/interactivity/win32/uiaTextRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/interactivity/win32/uiaTextRange.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
18 changes: 8 additions & 10 deletions src/types/TermControlUiaTextRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ IFACEMETHODIMP TermControlUiaTextRange::Clone(_Outptr_result_maybenull_ ITextRan
// (0,0) is the top-left of the app window
// Return Value:
// - <none>
void TermControlUiaTextRange::_TranslatePointToScreen(til::point* clientPoint) const
void TermControlUiaTextRange::_TranslatePointToScreen(til::point& clientPoint) const
{
const gsl::not_null<TermControlUiaProvider*> provider = static_cast<TermControlUiaProvider*>(_pProvider);

Expand All @@ -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:
Expand All @@ -107,15 +107,13 @@ void TermControlUiaTextRange::_TranslatePointToScreen(til::point* clientPoint) c
// (0,0) is the top-left of the screen
// Return Value:
// - <none>
void TermControlUiaTextRange::_TranslatePointFromScreen(til::point* screenPoint) const
void TermControlUiaTextRange::_TranslatePointFromScreen(til::point& screenPoint) const
{
const gsl::not_null<TermControlUiaProvider*> provider = static_cast<TermControlUiaProvider*>(_pProvider);

const auto includeOffsets = [](long screenPos, double termControlPos, double padding, double scaleFactor) {
auto result = base::ClampedNumeric<double>(padding);
// only the padding is in DIPs now
result /= scaleFactor;
result -= screenPos;
auto result = base::ClampedNumeric<til::CoordType>(screenPos);
result -= padding / scaleFactor;
result -= termControlPos;
return result;
};
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/types/TermControlUiaTextRange.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
}
40 changes: 17 additions & 23 deletions src/types/UiaTextRangeBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,30 +97,24 @@ CATCH_RETURN();

void UiaTextRangeBase::Initialize(_In_ const UiaPoint point)
{
til::point clientPoint;
clientPoint.x = static_cast<LONG>(point.x);
clientPoint.y = static_cast<LONG>(point.y);
// get row that point resides in
til::point clientPoint{ static_cast<til::CoordType>(point.x), static_cast<til::CoordType>(point.y) };

// 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();
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);
_pData->UnlockConsole();

const auto currentFontSize = _getScreenFontSize();
row = clientPoint.y / currentFontSize.height + viewport.top;
}
_start = { 0, row };
_start = { clientPoint.x / currentFontSize.width + viewport.left,
clientPoint.y / currentFontSize.height + viewport.top };
_end = _start;
}

Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/types/UiaTextRangeBase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit e866a6c

Please sign in to comment.