Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore off-the-top behavior for VT mouse mode #17779

Merged
merged 4 commits into from
Aug 23, 2024
Merged

Restore off-the-top behavior for VT mouse mode #17779

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b55fee5
Allow Interactivity to suppress autoscroll
DHowett Aug 22, 2024
2b1310f
Fix mouse coordinate clamping
DHowett Aug 22, 2024
eb8030e
Revert b55fee5b5ef51cbac7cbb13944160939c2ec6501 and fix w/ bool return
DHowett Aug 23, 2024
9db0463
Fix the test
DHowett Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,14 +334,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers,
const bool focused,
const Core::Point pixelPosition,
const bool pointerPressedInBounds)
const bool pointerPressedInBounds,
bool& suppressFurtherHandling)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change this to a return value it'd probably be worth adding a comment that "return true" means "suppress further handlings" just in case this helps someone in the future.

{
const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition });
suppressFurtherHandling = false;

// Short-circuit isReadOnly check to avoid warning dialog
if (focused && !_core->IsInReadOnlyMode() && _canSendVTMouseInput(modifiers))
{
_sendMouseEventHelper(terminalPosition, pointerUpdateKind, modifiers, 0, buttonState);
suppressFurtherHandling = true;
}
// GH#4603 - don't modify the selection if the pointer press didn't
// actually start _in_ the control bounds. Case in point - someone drags
Expand Down Expand Up @@ -682,13 +685,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const SHORT wheelDelta,
Control::MouseButtonState buttonState)
{
const auto adjustment = _core->ScrollOffset() > 0 ? _core->BufferHeight() - _core->ScrollOffset() - _core->ViewHeight() : 0;
// If the click happened outside the active region, just don't send any mouse event
if (const auto adjustedY = terminalPosition.y - adjustment; adjustedY >= 0)
{
return _core->SendMouseEvent({ terminalPosition.x, adjustedY }, pointerUpdateKind, modifiers, wheelDelta, toInternalMouseState(buttonState));
}
return false;
const auto adjustment = _core->BufferHeight() - _core->ScrollOffset() - _core->ViewHeight();
// If the click happened outside the active region, core should get a chance to filter it out or clamp it.
const auto adjustedY = terminalPosition.y - adjustment;
return _core->SendMouseEvent({ terminalPosition.x, adjustedY }, pointerUpdateKind, modifiers, wheelDelta, toInternalMouseState(buttonState));
}

// Method Description:
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalControl/ControlInteractivity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers,
const bool focused,
const Core::Point pixelPosition,
const bool pointerPressedInBounds);
const bool pointerPressedInBounds,
bool& suppressFurtherHandling);
void TouchMoved(const Core::Point newTouchPoint,
const bool focused);

Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalControl/ControlInteractivity.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ namespace Microsoft.Terminal.Control
Microsoft.Terminal.Core.ControlKeyStates modifiers,
Boolean focused,
Microsoft.Terminal.Core.Point pixelPosition,
Boolean pointerPressedInBounds);
Boolean pointerPressedInBounds,
out Boolean suppressFurtherHandling);

void TouchMoved(Microsoft.Terminal.Core.Point newTouchPoint,
Boolean focused);
Expand Down
6 changes: 4 additions & 2 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1910,18 +1910,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (type == Windows::Devices::Input::PointerDeviceType::Mouse ||
type == Windows::Devices::Input::PointerDeviceType::Pen)
{
bool suppressFurtherHandling{ false };
_interactivity.PointerMoved(TermControl::GetPressedMouseButtons(point),
TermControl::GetPointerUpdateKind(point),
ControlKeyStates(args.KeyModifiers()),
_focused,
pixelPosition.to_core_point(),
_pointerPressedInBounds);
_pointerPressedInBounds,
/* out */ suppressFurtherHandling);

// 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())
if (!suppressFurtherHandling && _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
Expand Down
45 changes: 34 additions & 11 deletions src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@ namespace ControlUnitTests
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES()

bool unused{};

// This is a test for GH#9725
WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{};

Expand Down Expand Up @@ -333,7 +335,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's one selection");
VERIFY_IS_TRUE(core->HasSelection());

Expand All @@ -345,7 +348,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition2.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's now two selections (one on each row)");
VERIFY_IS_TRUE(core->HasSelection());

Expand Down Expand Up @@ -376,7 +380,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition4.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's now one selection");
VERIFY_IS_TRUE(core->HasSelection());
}
Expand All @@ -390,6 +395,8 @@ namespace ControlUnitTests
// For the sake of this test, scroll one line at a time
interactivity->_rowsToScroll = 1;

bool unused{};

Log::Comment(L"Add some test to the terminal so we can scroll");
for (auto i = 0; i < 40; ++i)
{
Expand Down Expand Up @@ -430,7 +437,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's one selection");
VERIFY_IS_TRUE(core->HasSelection());

Expand Down Expand Up @@ -550,6 +558,8 @@ namespace ControlUnitTests
auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn);
_standardInit(core, interactivity);

bool unused{};

// For this test, don't use any modifiers
const auto modifiers = ControlKeyStates();
const auto leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown };
Expand Down Expand Up @@ -577,7 +587,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's one selection");
VERIFY_IS_TRUE(core->HasSelection());

Expand All @@ -594,6 +605,8 @@ namespace ControlUnitTests
auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn);
_standardInit(core, interactivity);

bool unused {}

// For this test, don't use any modifiers
const auto modifiers = ControlKeyStates();
const auto leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown };
Expand Down Expand Up @@ -621,7 +634,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's one selection");
VERIFY_IS_TRUE(core->HasSelection());

Expand All @@ -646,7 +660,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition2.to_core_point(),
false);
false,
unused);

Log::Comment(L"The selection should be unchanged.");
VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor());
Expand All @@ -662,6 +677,8 @@ namespace ControlUnitTests
auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn);
_standardInit(core, interactivity);

bool unused{};

// For this test, don't use any modifiers
const auto modifiers = ControlKeyStates();
const auto leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown };
Expand Down Expand Up @@ -737,7 +754,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's still no selection");
VERIFY_IS_FALSE(core->HasSelection());
}
Expand All @@ -751,6 +769,8 @@ namespace ControlUnitTests
auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn);
_standardInit(core, interactivity);

bool unused{};

Log::Comment(L"Fill up the history buffer");
const auto scrollbackLength = settings->HistorySize();
// Output lines equal to history size + viewport height to make sure we're
Expand Down Expand Up @@ -790,7 +810,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
Log::Comment(L"Verify that there's one selection");
VERIFY_IS_TRUE(core->HasSelection());

Expand Down Expand Up @@ -849,7 +870,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition0.to_core_point(),
true);
true,
unused);
VERIFY_IS_TRUE(core->HasSelection());
{
const auto anchor{ core->_terminal->GetSelectionAnchor() };
Expand All @@ -873,7 +895,8 @@ namespace ControlUnitTests
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true);
true,
unused);
VERIFY_IS_TRUE(core->HasSelection());
{
const auto anchor{ core->_terminal->GetSelectionAnchor() };
Expand Down
Loading