Make ScreenInfoUiaProvider::GetSelection() Return One Selection (#4466)

## Summary of the Pull Request We used to return multiple text ranges to represent one selection. We only support one selection at a time, so we should only return one range. Additionally, I moved all TriggerSelection() calls to the renderer from Terminal to TermControl for consistency. This ensures we only call it _once_ when we make a change to our selection state. ## References #2447 - helps polish Signaling for Selection #4465 - This is more apparent as the problem holding back Signaling for Selection ## PR Checklist * [x] Closes #4452 Tested using Accessibility Insights.
microsoft · Feb 20, 2020 · d0c8221 · d0c8221
1 parent ce39b63
commit d0c8221
Show file tree

Hide file tree

Showing 18 changed files with 130 additions and 167 deletions.
diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp
@@ -852,6 +852,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         if (_terminal->IsSelectionActive())
         {
             _terminal->ClearSelection();
+            _renderer->TriggerSelection();
 
             if (vkey == VK_ESCAPE)
             {
@@ -1744,6 +1745,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         if (!_terminal->IsCopyOnSelectActive())
         {
             _terminal->ClearSelection();
+            _renderer->TriggerSelection();
         }
 
         // send data up for clipboard

diff --git a/src/cascadia/TerminalControl/TermControlUiaProvider.cpp b/src/cascadia/TerminalControl/TermControlUiaProvider.cpp
@@ -100,26 +100,22 @@ void TermControlUiaProvider::ChangeViewport(const SMALL_RECT NewWindow)
     _termControl->ScrollViewport(NewWindow.Top);
 }
 
-HRESULT TermControlUiaProvider::GetSelectionRanges(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _Out_ std::deque<ComPtr<UiaTextRangeBase>>& result)
+HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ UiaTextRangeBase** ppUtr)
 {
-    try
-    {
-        result.clear();
-        typename std::remove_reference<decltype(result)>::type temporaryResult;
+    RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr);
+    *ppUtr = nullptr;
 
-        std::deque<ComPtr<UiaTextRange>> ranges;
-        RETURN_IF_FAILED(UiaTextRange::GetSelectionRanges(_pData, pProvider, wordDelimiters, ranges));
+    const auto start = _pData->GetSelectionAnchor();
 
-        while (!ranges.empty())
-        {
-            temporaryResult.emplace_back(std::move(ranges.back()));
-            ranges.pop_back();
-        }
+    // we need to make end exclusive
+    auto end = _pData->GetEndSelectionPosition();
+    _pData->GetTextBuffer().GetSize().IncrementInBounds(end, true);
 
-        std::swap(result, temporaryResult);
-        return S_OK;
-    }
-    CATCH_RETURN();
+    // TODO GH #4509: Box Selection is misrepresented here as a line selection.
+    UiaTextRange* result = nullptr;
+    RETURN_IF_FAILED(MakeAndInitialize<UiaTextRange>(&result, _pData, pProvider, start, end, wordDelimiters));
+    *ppUtr = result;
+    return S_OK;
 }
 
 HRESULT TermControlUiaProvider::CreateTextRange(_In_ IRawElementProviderSimple* const pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ UiaTextRangeBase** ppUtr)

diff --git a/src/cascadia/TerminalControl/TermControlUiaProvider.hpp b/src/cascadia/TerminalControl/TermControlUiaProvider.hpp
@@ -48,7 +48,7 @@ namespace Microsoft::Terminal
         void ChangeViewport(const SMALL_RECT NewWindow) override;
 
     protected:
-        HRESULT GetSelectionRanges(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _Out_ std::deque<WRL::ComPtr<Microsoft::Console::Types::UiaTextRangeBase>>& selectionRanges) override;
+        HRESULT GetSelectionRange(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** ppUtr) override;
 
         // degenerate range
         HRESULT CreateTextRange(_In_ IRawElementProviderSimple* const pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** ppUtr) override;

diff --git a/src/cascadia/TerminalControl/UiaTextRange.cpp b/src/cascadia/TerminalControl/UiaTextRange.cpp
@@ -10,34 +10,6 @@ using namespace Microsoft::Console::Types;
 using namespace Microsoft::WRL;
 using namespace winrt::Windows::Graphics::Display;
 
-HRESULT UiaTextRange::GetSelectionRanges(_In_ IUiaData* pData,
-                                         _In_ IRawElementProviderSimple* pProvider,
-                                         _In_ const std::wstring_view wordDelimiters,
-                                         _Out_ std::deque<ComPtr<UiaTextRange>>& ranges)
-{
-    try
-    {
-        typename std::remove_reference<decltype(ranges)>::type temporaryResult;
-
-        // get the selection rects
-        const auto rectangles = pData->GetSelectionRects();
-
-        // create a range for each row
-        for (const auto& rect : rectangles)
-        {
-            const auto start = rect.Origin();
-            const auto end = rect.EndExclusive();
-
-            ComPtr<UiaTextRange> range;
-            RETURN_IF_FAILED(MakeAndInitialize<UiaTextRange>(&range, pData, pProvider, start, end, wordDelimiters));
-            temporaryResult.emplace_back(std::move(range));
-        }
-        std::swap(temporaryResult, ranges);
-        return S_OK;
-    }
-    CATCH_RETURN();
-}
-
 // degenerate range constructor.
 HRESULT UiaTextRange::RuntimeClassInitialize(_In_ IUiaData* pData, _In_ IRawElementProviderSimple* const pProvider, _In_ const std::wstring_view wordDelimiters)
 {

diff --git a/src/cascadia/TerminalControl/UiaTextRange.hpp b/src/cascadia/TerminalControl/UiaTextRange.hpp
@@ -23,11 +23,6 @@ namespace Microsoft::Terminal
     class UiaTextRange final : public Microsoft::Console::Types::UiaTextRangeBase
     {
     public:
-        static HRESULT GetSelectionRanges(_In_ Microsoft::Console::Types::IUiaData* pData,
-                                          _In_ IRawElementProviderSimple* pProvider,
-                                          _In_ const std::wstring_view wordDelimiters,
-                                          _Out_ std::deque<WRL::ComPtr<UiaTextRange>>& ranges);
-
         UiaTextRange() = default;
 
         // degenerate range

diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp
@@ -141,7 +141,8 @@ class Microsoft::Terminal::Core::Terminal final :
     const bool IsSelectionActive() const noexcept override;
     void ClearSelection() override;
     void SelectNewRegion(const COORD coordStart, const COORD coordEnd) override;
-    const COORD GetSelectionAnchor() const override;
+    const COORD GetSelectionAnchor() const noexcept override;
+    const COORD GetEndSelectionPosition() const noexcept override;
     const std::wstring GetConsoleTitle() const noexcept override;
     void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute) override;
 #pragma endregion

diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp
@@ -113,14 +113,26 @@ SMALL_RECT Terminal::_GetSelectionRow(const SHORT row, const COORD higherCoord,
 // - None
 // Return Value:
 // - None
-const COORD Terminal::GetSelectionAnchor() const
+const COORD Terminal::GetSelectionAnchor() const noexcept
 {
     COORD selectionAnchorPos{ _selectionAnchor };
-    THROW_IF_FAILED(ShortAdd(selectionAnchorPos.Y, _selectionVerticalOffset, &selectionAnchorPos.Y));
-
+    selectionAnchorPos.Y = base::ClampAdd(selectionAnchorPos.Y, _selectionVerticalOffset);
     return selectionAnchorPos;
 }
 
+// Method Description:
+// - Get the current end anchor position relative to the whole text buffer
+// Arguments:
+// - None
+// Return Value:
+// - None
+const COORD Terminal::GetEndSelectionPosition() const noexcept
+{
+    COORD endSelectionPos{ _endSelectionPosition };
+    endSelectionPos.Y = base::ClampAdd(endSelectionPos.Y, _selectionVerticalOffset);
+    return endSelectionPos;
+}
+
 // Method Description:
 // - Expand the selection row according to selection mode and wide glyphs
 // - this is particularly useful for box selections (ALT + selection)
@@ -325,15 +337,14 @@ void Terminal::SetBoxSelection(const bool isEnabled) noexcept
 
 // Method Description:
 // - clear selection data and disable rendering it
+#pragma warning(disable : 26440) // changing this to noexcept would require a change to ConHost's selection model
 void Terminal::ClearSelection()
 {
     _selectionActive = false;
     _allowSingleCharSelection = false;
     _selectionAnchor = { 0, 0 };
     _endSelectionPosition = { 0, 0 };
     _selectionVerticalOffset = 0;
-
-    _buffer->GetRenderTarget().TriggerSelection();
 }
 
 // Method Description:

diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp
@@ -174,7 +174,6 @@ void Terminal::SelectNewRegion(const COORD coordStart, const COORD coordEnd)
 
     SetSelectionAnchor(realCoordStart);
     SetEndSelectionPosition(realCoordEnd);
-    _buffer->GetRenderTarget().TriggerSelection();
 }
 
 const std::wstring Terminal::GetConsoleTitle() const noexcept

diff --git a/src/host/renderData.cpp b/src/host/renderData.cpp
@@ -389,11 +389,50 @@ void RenderData::SelectNewRegion(const COORD coordStart, const COORD coordEnd)
 // - none
 // Return Value:
 // - current selection anchor
-const COORD RenderData::GetSelectionAnchor() const
+const COORD RenderData::GetSelectionAnchor() const noexcept
 {
     return Selection::Instance().GetSelectionAnchor();
 }
 
+// Routine Description:
+// - Gets the current end selection anchor position
+// Arguments:
+// - none
+// Return Value:
+// - current selection anchor
+const COORD RenderData::GetEndSelectionPosition() const noexcept
+{
+    // The selection area in ConHost is encoded as two things...
+    //  - SelectionAnchor: the initial position where the selection was started
+    //  - SelectionRect: the rectangular region denoting a portion of the buffer that is selected
+
+    // The following is an exerpt from Selection::s_GetSelectionRects
+    // if the anchor (start of select) was in the top right or bottom left of the box,
+    // we need to remove rectangular overlap in the middle.
+    // e.g.
+    // For selections with the anchor in the top left (A) or bottom right (B),
+    // it is valid to maintain the inner rectangle (+) as part of the selection
+    //               A+++++++================
+    // ==============++++++++B
+    // + and = are valid highlights in this scenario.
+    // For selections with the anchor in in the top right (A) or bottom left (B),
+    // we must remove a portion of the first/last line that lies within the rectangle (+)
+    //               +++++++A=================
+    // ==============B+++++++
+    // Only = is valid for highlight in this scenario.
+    // This is only needed for line selection. Box selection doesn't need to account for this.
+    const auto selectionRect = Selection::Instance().GetSelectionRectangle();
+
+    // To extract the end anchor from this rect, we need to know which corner of the rect is the SelectionAnchor
+    // Then choose the opposite corner.
+    const auto anchor = Selection::Instance().GetSelectionAnchor();
+
+    const short x_pos = (selectionRect.Left == anchor.X) ? selectionRect.Right : selectionRect.Left;
+    const short y_pos = (selectionRect.Top == anchor.Y) ? selectionRect.Bottom : selectionRect.Top;
+
+    return { x_pos, y_pos };
+}
+
 // Routine Description:
 // - Given two points in the buffer space, color the selection between the two with the given attribute.
 // - This will create an internal selection rectangle covering the two points, assume a line selection,

diff --git a/src/host/renderData.hpp b/src/host/renderData.hpp
@@ -60,7 +60,8 @@ class RenderData final :
     const bool IsSelectionActive() const override;
     void ClearSelection() override;
     void SelectNewRegion(const COORD coordStart, const COORD coordEnd) override;
-    const COORD GetSelectionAnchor() const;
+    const COORD GetSelectionAnchor() const noexcept;
+    const COORD GetEndSelectionPosition() const noexcept;
     void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr);
 #pragma endregion
 };
diff --git a/src/interactivity/win32/screenInfoUiaProvider.cpp b/src/interactivity/win32/screenInfoUiaProvider.cpp
@@ -97,26 +97,22 @@ void ScreenInfoUiaProvider::ChangeViewport(const SMALL_RECT NewWindow)
     _pUiaParent->ChangeViewport(NewWindow);
 }
 
-HRESULT ScreenInfoUiaProvider::GetSelectionRanges(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _Out_ std::deque<ComPtr<UiaTextRangeBase>>& result)
+HRESULT ScreenInfoUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** ppUtr)
 {
-    try
-    {
-        result.clear();
-        typename std::remove_reference<decltype(result)>::type temporaryResult;
+    RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr);
+    *ppUtr = nullptr;
 
-        std::deque<ComPtr<UiaTextRange>> ranges;
-        RETURN_IF_FAILED(UiaTextRange::GetSelectionRanges(_pData, pProvider, wordDelimiters, ranges));
+    const auto start = _pData->GetSelectionAnchor();
 
-        while (!ranges.empty())
-        {
-            temporaryResult.emplace_back(std::move(ranges.back()));
-            ranges.pop_back();
-        }
+    // we need to make end exclusive
+    auto end = _pData->GetEndSelectionPosition();
+    _pData->GetTextBuffer().GetSize().IncrementInBounds(end, true);
 
-        std::swap(result, temporaryResult);
-        return S_OK;
-    }
-    CATCH_RETURN();
+    // TODO GH #4509: Box Selection is misrepresented here as a line selection.
+    UiaTextRange* result;
+    RETURN_IF_FAILED(MakeAndInitialize<UiaTextRange>(&result, _pData, pProvider, start, end, wordDelimiters));
+    *ppUtr = result;
+    return S_OK;
 }
 
 HRESULT ScreenInfoUiaProvider::CreateTextRange(_In_ IRawElementProviderSimple* const pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ UiaTextRangeBase** ppUtr)

diff --git a/src/interactivity/win32/screenInfoUiaProvider.hpp b/src/interactivity/win32/screenInfoUiaProvider.hpp
@@ -44,7 +44,7 @@ namespace Microsoft::Console::Interactivity::Win32
         void ChangeViewport(const SMALL_RECT NewWindow) override;
 
     protected:
-        HRESULT GetSelectionRanges(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _Out_ std::deque<WRL::ComPtr<Microsoft::Console::Types::UiaTextRangeBase>>& selectionRanges) override;
+        HRESULT GetSelectionRange(_In_ IRawElementProviderSimple* pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** ppUtr) override;
 
         // degenerate range
         HRESULT CreateTextRange(_In_ IRawElementProviderSimple* const pProvider, const std::wstring_view wordDelimiters, _COM_Outptr_result_maybenull_ Microsoft::Console::Types::UiaTextRangeBase** ppUtr) override;

diff --git a/src/interactivity/win32/uiaTextRange.cpp b/src/interactivity/win32/uiaTextRange.cpp
@@ -12,34 +12,6 @@ using namespace Microsoft::Console::Interactivity::Win32;
 using namespace Microsoft::WRL;
 using Microsoft::Console::Interactivity::ServiceLocator;
 
-HRESULT UiaTextRange::GetSelectionRanges(_In_ IUiaData* pData,
-                                         _In_ IRawElementProviderSimple* pProvider,
-                                         const std::wstring_view wordDelimiters,
-                                         _Out_ std::deque<ComPtr<UiaTextRange>>& ranges)
-{
-    try
-    {
-        typename std::remove_reference<decltype(ranges)>::type temporaryResult;
-
-        // get the selection rects
-        const auto rectangles = pData->GetSelectionRects();
-
-        // create a range for each row
-        for (const auto& rect : rectangles)
-        {
-            const auto start = rect.Origin();
-            const auto end = rect.EndExclusive();
-
-            ComPtr<UiaTextRange> range;
-            RETURN_IF_FAILED(MakeAndInitialize<UiaTextRange>(&range, pData, pProvider, start, end, wordDelimiters));
-            temporaryResult.emplace_back(std::move(range));
-        }
-        std::swap(temporaryResult, ranges);
-        return S_OK;
-    }
-    CATCH_RETURN();
-}
-
 // degenerate range constructor.
 HRESULT UiaTextRange::RuntimeClassInitialize(_In_ IUiaData* pData, _In_ IRawElementProviderSimple* const pProvider, _In_ const std::wstring_view wordDelimiters)
 {

diff --git a/src/interactivity/win32/uiaTextRange.hpp b/src/interactivity/win32/uiaTextRange.hpp
@@ -24,11 +24,6 @@ namespace Microsoft::Console::Interactivity::Win32
     class UiaTextRange final : public Microsoft::Console::Types::UiaTextRangeBase
     {
     public:
-        static HRESULT GetSelectionRanges(_In_ Microsoft::Console::Types::IUiaData* pData,
-                                          _In_ IRawElementProviderSimple* pProvider,
-                                          _In_ const std::wstring_view wordDelimiters,
-                                          _Out_ std::deque<WRL::ComPtr<UiaTextRange>>& ranges);
-
         UiaTextRange() = default;
 
         // degenerate range

diff --git a/src/types/IUiaData.h b/src/types/IUiaData.h
@@ -36,7 +36,8 @@ namespace Microsoft::Console::Types
         virtual const bool IsSelectionActive() const = 0;
         virtual void ClearSelection() = 0;
         virtual void SelectNewRegion(const COORD coordStart, const COORD coordEnd) = 0;
-        virtual const COORD GetSelectionAnchor() const = 0;
+        virtual const COORD GetSelectionAnchor() const noexcept = 0;
+        virtual const COORD GetEndSelectionPosition() const noexcept = 0;
         virtual void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr) = 0;
     };