maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix a pair of TermControl dragging bugs (#10650)

Browse source 
## Summary of the Pull Request

This fixes two bugs related to dragging into the bounds of the `TermControl`. Although the fixes are fairly small, I'm batching them up, because I don't want to stack 2 more PRs on top of #10051.

* #9109 
  - This is fixed by only starting an autoscroll if the click&drag actually started within the bounds of the control. 
* #4603
  - Building on the above change, only modify the selection when the drag started in the control. 
 
## References
* srsly go read #10051.

## PR Checklist
* [x] Closes #9109
* [x] Closes #4603
* [x] I work here
* [x] Test added
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

This is kind of annoying that the auto-scrolling is handled by the TermControl, but it uses a timer that's still a WinUI construct.

We only want to start the auto-scrolling behavior when the drag started _inside_ the control. Otherwise, in the tab drag scenario, dragging into the bounds of the TermControl will trick it into thinking it should start a scroll.
This commit is contained in:
Mike Griese 2021-07-28 17:27:09 -05:00 committed by GitHub
parent f058b08fde
commit 4b45bb8df1
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 115 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
src/cascadia/TerminalControl/ControlInteractivity.cpp
View file

@ -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)
{

3
src/cascadia/TerminalControl/ControlInteractivity.h
View file

@ -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);

4
src/cascadia/TerminalControl/ControlInteractivity.idl
View file

@ -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);

19
src/cascadia/TerminalControl/TermControl.cpp
View file

@ -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();

4
src/cascadia/TerminalControl/TermControl.h
View file

@ -167,11 +167,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
std::shared_ptr<ThrottledFuncTrailing<ScrollBarUpdate>> _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<Windows::UI::Input::PointerPoint> _autoScrollingPointerPoint;
Windows::UI::Xaml::DispatcherTimer _autoScrollTimer;
std::optional<std::chrono::high_resolution_clock::time_point> _lastAutoScrollUpdateTime;
bool _pointerPressedInBounds{ false };
winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr };
Windows::UI::Xaml::DispatcherTimer _bellLightTimer{ nullptr };

90
src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp
View file

@ -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<int>() * 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<int>() * 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());
}