diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 56c6686982..dc3f61490f 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -279,7 +279,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation const unsigned int pointerUpdateKind, const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const bool focused, - const til::point pixelPosition) + const til::point pixelPosition, + const bool pointerPressedInBounds) { const til::point terminalPosition = _getTerminalPosition(pixelPosition); @@ -288,7 +289,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _core->SendMouseEvent(terminalPosition, pointerUpdateKind, modifiers, 0, toInternalMouseState(buttonState)); } - else if (focused && WI_IsFlagSet(buttonState, MouseButtonState::IsLeftButtonDown)) + // GH#4603 - don't modify the selection if the pointer press didn't + // actually start _in_ the control bounds. Case in point - someone drags + // a file into the bounds of the control. That shouldn't send the + // selection into space. + else if (focused && pointerPressedInBounds && WI_IsFlagSet(buttonState, MouseButtonState::IsLeftButtonDown)) { if (_singleClickTouchdownPos) { diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index 1c86a52d69..9375348388 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -58,7 +58,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation const unsigned int pointerUpdateKind, const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const bool focused, - const til::point pixelPosition); + const til::point pixelPosition, + const bool pointerPressedInBounds); void TouchMoved(const til::point newTouchPoint, const bool focused); diff --git a/src/cascadia/TerminalControl/ControlInteractivity.idl b/src/cascadia/TerminalControl/ControlInteractivity.idl index 385eefdbd5..6261b773d8 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.idl +++ b/src/cascadia/TerminalControl/ControlInteractivity.idl @@ -39,7 +39,9 @@ namespace Microsoft.Terminal.Control UInt32 pointerUpdateKind, Microsoft.Terminal.Core.ControlKeyStates modifiers, Boolean focused, - Microsoft.Terminal.Core.Point pixelPosition); + Microsoft.Terminal.Core.Point pixelPosition, + Boolean pointerPressedInBounds); + void TouchMoved(Microsoft.Terminal.Core.Point newTouchPoint, Boolean focused); diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 435dbc178e..e1ea1d7a03 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1052,6 +1052,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation Focus(FocusState::Pointer); } + // Mark that this pointer event actually started within our bounds. + // We'll need this later, for PointerMoved events. + _pointerPressedInBounds = true; + if (type == Windows::Devices::Input::PointerDeviceType::Touch) { const auto contactRect = point.Properties().ContactRect(); @@ -1104,10 +1108,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation TermControl::GetPointerUpdateKind(point), ControlKeyStates(args.KeyModifiers()), _focused, - pixelPosition); + pixelPosition, + _pointerPressedInBounds); - if (_focused && point.Properties().IsLeftButtonPressed()) + // GH#9109 - Only start an auto-scroll when the drag actually + // started within our bounds. Otherwise, someone could start a drag + // outside the terminal control, drag into the padding, and trick us + // into starting to scroll. + if (_focused && _pointerPressedInBounds && point.Properties().IsLeftButtonPressed()) { + // We want to find the distance relative to the bounds of the + // SwapChainPanel, not the entire control. If they drag out of + // the bounds of the text, into the padding, we still what that + // to auto-scroll const double cursorBelowBottomDist = cursorPosition.Y - SwapChainPanel().Margin().Top - SwapChainPanel().ActualHeight(); const double cursorAboveTopDist = -1 * cursorPosition.Y + SwapChainPanel().Margin().Top; @@ -1157,6 +1170,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation return; } + _pointerPressedInBounds = false; + const auto ptr = args.Pointer(); const auto point = args.GetCurrentPoint(*this); const auto cursorPosition = point.Position(); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index a074e020ef..6c9aa67573 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -167,11 +167,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation std::shared_ptr> _updateScrollBar; bool _isInternalScrollBarUpdate; - // Auto scroll occurs when user, while selecting, drags cursor outside viewport. View is then scrolled to 'follow' the cursor. + // Auto scroll occurs when user, while selecting, drags cursor outside + // viewport. View is then scrolled to 'follow' the cursor. double _autoScrollVelocity; std::optional _autoScrollingPointerPoint; Windows::UI::Xaml::DispatcherTimer _autoScrollTimer; std::optional _lastAutoScrollUpdateTime; + bool _pointerPressedInBounds{ false }; winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr }; Windows::UI::Xaml::DispatcherTimer _bellLightTimer{ nullptr }; diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index bf6eaede1d..14801722e0 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -32,6 +32,9 @@ namespace ControlUnitTests TEST_METHOD(ScrollWithSelection); TEST_METHOD(TestScrollWithTrackpad); TEST_METHOD(TestQuickDragOnSelect); + + TEST_METHOD(TestDragSelectOutsideBounds); + TEST_METHOD(PointerClickOutsideActiveRegion); TEST_CLASS_SETUP(ClassSetup) @@ -288,7 +291,8 @@ namespace ControlUnitTests WM_LBUTTONDOWN, //pointerUpdateKind modifiers, true, // focused, - cursorPosition1); + cursorPosition1, + true); Log::Comment(L"Verify that there's one selection"); VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); @@ -300,7 +304,8 @@ namespace ControlUnitTests WM_LBUTTONDOWN, //pointerUpdateKind modifiers, true, // focused, - cursorPosition2); + cursorPosition2, + true); Log::Comment(L"Verify that there's now two selections (one on each row)"); VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(2u, core->_terminal->GetSelectionRects().size()); @@ -333,7 +338,8 @@ namespace ControlUnitTests WM_LBUTTONDOWN, //pointerUpdateKind modifiers, true, // focused, - cursorPosition4); + cursorPosition4, + true); Log::Comment(L"Verify that there's now one selection"); VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); @@ -388,7 +394,8 @@ namespace ControlUnitTests WM_LBUTTONDOWN, //pointerUpdateKind modifiers, true, // focused, - cursorPosition1); + cursorPosition1, + true); Log::Comment(L"Verify that there's one selection"); VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); @@ -536,7 +543,8 @@ namespace ControlUnitTests WM_LBUTTONDOWN, //pointerUpdateKind modifiers, true, // focused, - cursorPosition1); + cursorPosition1, + true); Log::Comment(L"Verify that there's one selection"); VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); @@ -546,6 +554,74 @@ namespace ControlUnitTests VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); } + void ControlInteractivityTests::TestDragSelectOutsideBounds() + { + // This is a test for GH#4603 + + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const Control::MouseButtonState leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown }; + const Control::MouseButtonState noMouseDown{}; + + const til::size fontSize{ 9, 21 }; + Log::Comment(L"Click on the terminal"); + const til::point cursorPosition0{ 6, 0 }; + interactivity->PointerPressed(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + 0, // timestamp + modifiers, + cursorPosition0); + + Log::Comment(L"Verify that there's not yet a selection"); + VERIFY_IS_FALSE(core->HasSelection()); + + VERIFY_IS_TRUE(interactivity->_singleClickTouchdownPos.has_value()); + VERIFY_ARE_EQUAL(cursorPosition0, interactivity->_singleClickTouchdownPos.value()); + + Log::Comment(L"Drag the mouse a lot. This simulates dragging the mouse real fast."); + const til::point cursorPosition1{ 6 + fontSize.width() * 2, 0 }; + interactivity->PointerMoved(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + modifiers, + true, // focused, + cursorPosition1, + true); + Log::Comment(L"Verify that there's one selection"); + VERIFY_IS_TRUE(core->HasSelection()); + VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); + + Log::Comment(L"Verify that it started on the first cell we clicked on, not the one we dragged to"); + COORD expectedAnchor{ 0, 0 }; + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); + COORD expectedEnd{ 2, 0 }; + VERIFY_ARE_EQUAL(expectedEnd, core->_terminal->GetSelectionEnd()); + + interactivity->PointerReleased(noMouseDown, + WM_LBUTTONUP, + modifiers, + cursorPosition1); + + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); + VERIFY_ARE_EQUAL(expectedEnd, core->_terminal->GetSelectionEnd()); + + Log::Comment(L"Simulate dragging the mouse into the control, without first clicking into the control"); + const til::point cursorPosition2{ fontSize.width() * 10, 0 }; + interactivity->PointerMoved(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + modifiers, + true, // focused, + cursorPosition2, + false); + + Log::Comment(L"The selection should be unchanged."); + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); + VERIFY_ARE_EQUAL(expectedEnd, core->_terminal->GetSelectionEnd()); + } + void ControlInteractivityTests::PointerClickOutsideActiveRegion() { // This is a test for GH#10642 @@ -561,7 +637,6 @@ namespace ControlUnitTests const Control::MouseButtonState noMouseDown{}; const til::size fontSize{ 9, 21 }; - interactivity->_rowsToScroll = 1; int expectedTop = 0; int expectedViewHeight = 20; @@ -630,7 +705,8 @@ namespace ControlUnitTests WM_LBUTTONDOWN, //pointerUpdateKind modifiers, true, // focused, - cursorPosition1); + cursorPosition1, + true); Log::Comment(L"Verify that there's still no selection"); VERIFY_IS_FALSE(core->HasSelection()); }