diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index ecee7cc0e56..433150cabb5 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -197,7 +197,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (multiClickMapper == 1) { _singleClickTouchdownPos = pixelPosition; - _singleClickTouchdownTerminalPos = terminalPosition; if (!_core->HasSelection()) { @@ -266,10 +265,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto fontSizeInDips{ _core->FontSizeInDips() }; if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f)) { - _core->SetSelectionAnchor(terminalPosition); + // GH#9955.c: Make sure to use the terminal location of the + // _touchdown_ point here. We want to start the selection + // from where the user initially clicked, not where they are + // now. + _core->SetSelectionAnchor(_getTerminalPosition(touchdownPoint)); + // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; - _singleClickTouchdownTerminalPos = std::nullopt; } } @@ -303,12 +306,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation // panning down) const float numRows = -1.0f * (dy / fontSizeInDips.height()); - const auto currentOffset = ::base::ClampedNumeric(_core->ScrollOffset()); - const auto newValue = numRows + currentOffset; + const double currentOffset = ::base::ClampedNumeric(_core->ScrollOffset()); + const double newValue = numRows + currentOffset; // Update the Core's viewport position, and raise a // ScrollPositionChanged event to update the scrollbar - _updateScrollbar(newValue); + UpdateScrollbar(newValue); // Use this point as our new scroll anchor. _touchAnchor = newTouchPoint; @@ -341,7 +344,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation } _singleClickTouchdownPos = std::nullopt; - _singleClickTouchdownTerminalPos = std::nullopt; } void ControlInteractivity::TouchReleased() @@ -396,7 +398,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } else { - _mouseScrollHandler(delta, terminalPosition, state.isLeftButtonDown); + _mouseScrollHandler(delta, pixelPosition, state.isLeftButtonDown); } return false; } @@ -428,35 +430,50 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - Scroll the visible viewport in response to a mouse wheel event. // Arguments: // - mouseDelta: the mouse wheel delta that triggered this event. - // - point: the location of the mouse during this event + // - pixelPosition: the location of the mouse during this event // - isLeftButtonPressed: true iff the left mouse button was pressed during this event. void ControlInteractivity::_mouseScrollHandler(const double mouseDelta, - const til::point terminalPosition, + const til::point pixelPosition, const bool isLeftButtonPressed) { - const auto currentOffset = _core->ScrollOffset(); + // GH#9955.b: Start scrolling from our internal scrollbar position. This + // lets us accumulate fractional numbers of rows to scroll with each + // event. Especially for precision trackpads, we might be getting scroll + // deltas smaller than a single row, but we still want lots of those to + // accumulate. + // + // At the start, let's compare what we _think_ the scrollbar is, with + // what it should be. It's possible the core scrolled out from + // underneath us. We wouldn't know - we don't want the overhead of + // another ScrollPositionChanged handler. If the scrollbar should be + // somewhere other than where it is currently, then start from that row. + const int currentInternalRow = ::base::saturated_cast(::std::round(_internalScrollbarPosition)); + const int currentCoreRow = _core->ScrollOffset(); + const double currentOffset = currentInternalRow == currentCoreRow ? + _internalScrollbarPosition : + currentCoreRow; // negative = down, positive = up // However, for us, the signs are flipped. // With one of the precision mice, one click is always a multiple of 120 (WHEEL_DELTA), // but the "smooth scrolling" mode results in non-int values - const auto rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA); + const double rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA); // WHEEL_PAGESCROLL is a Win32 constant that represents the "scroll one page // at a time" setting. If we ignore it, we will scroll a truly absurd number // of rows. - const auto rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? _core->ViewHeight() : _rowsToScroll }; + const double rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? ::base::saturated_cast(_core->ViewHeight()) : _rowsToScroll }; double newValue = (rowsToScroll * rowDelta) + (currentOffset); // Update the Core's viewport position, and raise a // ScrollPositionChanged event to update the scrollbar - _updateScrollbar(::base::saturated_cast(newValue)); + UpdateScrollbar(newValue); if (isLeftButtonPressed) { // If user is mouse selecting and scrolls, they then point at new // character. Make sure selection reflects that immediately. - SetEndSelectionPoint(terminalPosition); + SetEndSelectionPoint(pixelPosition); } } @@ -476,15 +493,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - newValue: The new top of the viewport // Return Value: // - - void ControlInteractivity::_updateScrollbar(const int newValue) + void ControlInteractivity::UpdateScrollbar(const double newValue) { - _core->UserScrollViewport(newValue); + // Set this as the new value of our internal scrollbar representation. + // We're doing this so we can accumulate fractional amounts of a row to + // scroll each time the mouse scrolls. + _internalScrollbarPosition = std::clamp(newValue, 0.0, _core->BufferHeight()); + + // If the new scrollbar position, rounded to an int, is at a different + // row, then actually update the scroll position in the core, and raise + // a ScrollPositionChanged to inform the control. + int viewTop = ::base::saturated_cast(::std::round(_internalScrollbarPosition)); + if (viewTop != _core->ScrollOffset()) + { + _core->UserScrollViewport(viewTop); - // _core->ScrollOffset() is now set to newValue - _ScrollPositionChangedHandlers(*this, - winrt::make(_core->ScrollOffset(), - _core->ViewHeight(), - _core->BufferHeight())); + // _core->ScrollOffset() is now set to newValue + _ScrollPositionChangedHandlers(*this, + winrt::make(_core->ScrollOffset(), + _core->ViewHeight(), + _core->BufferHeight())); + } } void ControlInteractivity::_hyperlinkHandler(const std::wstring_view uri) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index cd099dff3a4..f684f2dfff3 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -73,6 +73,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation const int32_t delta, const til::point pixelPosition, const ::Microsoft::Console::VirtualTerminal::TerminalInput::MouseButtonState state); + + void UpdateScrollbar(const double newValue); + #pragma endregion bool CopySelectionToClipboard(bool singleLine, @@ -83,6 +86,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation private: winrt::com_ptr _core{ nullptr }; unsigned int _rowsToScroll; + double _internalScrollbarPosition{ 0.0 }; // If this is set, then we assume we are in the middle of panning the // viewport via touch input. @@ -97,7 +101,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation Timestamp _lastMouseClickTimestamp; std::optional _lastMouseClickPos; std::optional _singleClickTouchdownPos; - std::optional _singleClickTouchdownTerminalPos; std::optional _lastMouseClickPosNoSelection; // This field tracks whether the selection has changed meaningfully // since it was last copied. It's generally used to prevent copyOnSelect @@ -124,7 +127,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _canSendVTMouseInput(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers); void _sendPastedTextToConnection(std::wstring_view wstr); - void _updateScrollbar(const int newValue); til::point _getTerminalPosition(const til::point& pixelPosition); TYPED_EVENT(OpenHyperlink, IInspectable, Control::OpenHyperlinkEventArgs); diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 5346d0507c9..3c9108da3b9 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1302,8 +1302,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation return; } - const auto newValue = static_cast(args.NewValue()); - _core->UserScrollViewport(newValue); + const auto newValue = args.NewValue(); + _interactivity->UpdateScrollbar(newValue); // User input takes priority over terminal events so cancel // any pending scroll bar update if the user scrolls. diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index eeb9825475f..3839d86e8bc 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -29,6 +29,9 @@ namespace ControlUnitTests TEST_METHOD(TestScrollWithMouse); TEST_METHOD(CreateSubsequentSelectionWithDragging); + TEST_METHOD(ScrollWithSelection); + TEST_METHOD(TestScrollWithTrackpad); + TEST_METHOD(TestQuickDragOnSelect); TEST_CLASS_SETUP(ClassSetup) { @@ -226,11 +229,13 @@ namespace ControlUnitTests Log::Comment(L"Scroll down 21 more times, to the bottom"); for (int i = 0; i < 21; ++i) { + Log::Comment(NoThrowString().Format(L"---scroll down #%d---", i)); expectedTop++; interactivity->MouseWheel(modifiers, -WHEEL_DELTA, til::point{ 0, 0 }, { false, false, false }); + Log::Comment(NoThrowString().Format(L"internal scrollbar pos:%f", interactivity->_internalScrollbarPosition)); } Log::Comment(L"Scrolling up more should do nothing"); expectedTop = 21; @@ -330,4 +335,211 @@ namespace ControlUnitTests VERIFY_IS_TRUE(core->HasSelection()); VERIFY_ARE_EQUAL(1u, core->_terminal->GetSelectionRects().size()); } + + void ControlInteractivityTests::ScrollWithSelection() + { + // This is a test for GH#9955.a + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + // For the sake of this test, scroll one line at a time + interactivity->_rowsToScroll = 1; + + Log::Comment(L"Add some test to the terminal so we can scroll"); + for (int i = 0; i < 40; ++i) + { + conn->WriteInput(L"Foo\r\n"); + } + // We printed that 40 times, but the final \r\n bumped the view down one MORE row. + VERIFY_ARE_EQUAL(20, core->_terminal->GetViewport().Height()); + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + VERIFY_ARE_EQUAL(20, core->ViewHeight()); + VERIFY_ARE_EQUAL(41, core->BufferHeight()); + + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const TerminalInput::MouseButtonState leftMouseDown{ true, false, false }; + const TerminalInput::MouseButtonState noMouseDown{ false, false, false }; + + const til::size fontSize{ 9, 21 }; + + Log::Comment(L"Click on the terminal"); + const til::point terminalPosition0{ 5, 5 }; + const til::point cursorPosition0{ terminalPosition0 * fontSize }; + 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 just a little"); + // move not quite a whole cell, but enough to start a selection + const til::point cursorPosition1{ cursorPosition0 + til::point{ 6, 0 } }; + interactivity->PointerMoved(leftMouseDown, + WM_LBUTTONDOWN, //pointerUpdateKind + modifiers, + true, // focused, + cursorPosition1); + 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 the location of the selection"); + // The viewport is on row 21, so the selection will be on: + // {(5, 5)+(0, 21)} to {(5, 5)+(0, 21)} + COORD expectedAnchor{ 5, 26 }; + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); + + Log::Comment(L"Scroll up a line, with the left mouse button selected"); + interactivity->MouseWheel(modifiers, + WHEEL_DELTA, + cursorPosition1, + { true, false, false }); + + Log::Comment(L"Verify the location of the selection"); + // The viewport is now on row 20, so the selection will be on: + // {(5, 5)+(0, 20)} to {(5, 5)+(0, 21)} + COORD newExpectedAnchor{ 5, 25 }; + // Remember, the anchor is always before the end in the buffer. So yes, + // se started the selection on 5,26, but now that's the end. + VERIFY_ARE_EQUAL(newExpectedAnchor, core->_terminal->GetSelectionAnchor()); + VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); + } + + void ControlInteractivityTests::TestScrollWithTrackpad() + { + WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{}; + + auto [settings, conn] = _createSettingsAndConnection(); + auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); + _standardInit(core, interactivity); + // For the sake of this test, scroll one line at a time + interactivity->_rowsToScroll = 1; + + for (int i = 0; i < 40; ++i) + { + conn->WriteInput(L"Foo\r\n"); + } + // We printed that 40 times, but the final \r\n bumped the view down one MORE row. + VERIFY_ARE_EQUAL(20, core->_terminal->GetViewport().Height()); + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + VERIFY_ARE_EQUAL(20, core->ViewHeight()); + VERIFY_ARE_EQUAL(41, core->BufferHeight()); + + Log::Comment(L"Scroll up a line"); + const auto modifiers = ControlKeyStates(); + + // Deltas that I saw while scrolling with the surface laptop trackpad + // were on the range [-22, 7], though I'm sure they could be greater in + // magnitude. + // + // WHEEL_DELTA is 120, so we'll use 24 for now as the delta, just so the tests don't take forever. + + const int delta = WHEEL_DELTA / 5; + const til::point mousePos{ 0, 0 }; + TerminalInput::MouseButtonState state{ false, false, false }; + + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + + Log::Comment(L"Scroll up 4 more times. Once we're at 3/5 scrolls, " + L"we'll round the internal scrollbar position to scrolling to the next row."); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 3/5 + VERIFY_ARE_EQUAL(20, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 4/5 + VERIFY_ARE_EQUAL(20, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 5/5 + VERIFY_ARE_EQUAL(20, core->ScrollOffset()); + + Log::Comment(L"Jump to line 5, so we can scroll down from there."); + interactivity->UpdateScrollbar(5); + VERIFY_ARE_EQUAL(5, core->ScrollOffset()); + Log::Comment(L"Scroll down 5 times, at which point we should accumulate a whole row of delta."); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(5, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(5, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 3/5 + VERIFY_ARE_EQUAL(6, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 4/5 + VERIFY_ARE_EQUAL(6, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, -delta, mousePos, state); // 5/5 + VERIFY_ARE_EQUAL(6, core->ScrollOffset()); + + Log::Comment(L"Jump to the bottom."); + interactivity->UpdateScrollbar(21); + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + Log::Comment(L"Scroll a bit, then emit a line of text. We should reset our internal scroll position."); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + + conn->WriteInput(L"Foo\r\n"); + VERIFY_ARE_EQUAL(22, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 1/5 + VERIFY_ARE_EQUAL(22, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 2/5 + VERIFY_ARE_EQUAL(22, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 3/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 4/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + interactivity->MouseWheel(modifiers, delta, mousePos, state); // 5/5 + VERIFY_ARE_EQUAL(21, core->ScrollOffset()); + } + + void ControlInteractivityTests::TestQuickDragOnSelect() + { + // This is a test for GH#9955.c + + 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 TerminalInput::MouseButtonState leftMouseDown{ true, false, false }; + const TerminalInput::MouseButtonState noMouseDown{ false, false, false }; + + 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); + 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()); + } }