Skip to content

Commit

Permalink
Restore off-the-top behavior for VT mouse mode (#17779)
Browse files Browse the repository at this point in the history
PR #10642 and #11290 introduced an adjustment for the cursor position
used to generate VT mouse mode events.

One of the decisions made in those PRs was to only send coordinates
where Y was >= 0, so if you were off the top of the screen you wouldn't
get any events. However, terminal emulators are expected to send
_clamped_ events when the mouse is off the screen. This decision broke
clamping Y to 0 when the mouse was above the screen.

The other decision was to only adjust the Y coordinate if the core's
`ScrollOffset` was greater than 0. It turns out that `ScrollOffset` _is
0_ when you are scrolled all the way back in teh buffer. With this
check, we would clamp coordinates properly _until the top line of the
scrollback was visible_, at which point we would send those coordinates
over directly. This resulted in the same weird behavior as observed in
#10190.

I've fixed both of those things. Core is expected to receive negative
coordinates and clamp them to the viewport. ScrollOffset should never be
below 0, as it refers to the top visible buffer line.

In addition to that, #17744 uncovered that we were allowing
autoscrolling to happen even when VT mouse events were being generated.
I added a way for `ControlInteractivity` to halt further event
processing. It's crude.

Refs #10190
Closes #17744
  • Loading branch information
DHowett authored Aug 23, 2024
1 parent e006f75 commit d1a1f98
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 25 deletions.
17 changes: 9 additions & 8 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -329,19 +329,22 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_touchAnchor = contactPoint;
}

void ControlInteractivity::PointerMoved(Control::MouseButtonState buttonState,
bool ControlInteractivity::PointerMoved(Control::MouseButtonState buttonState,
const unsigned int pointerUpdateKind,
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers,
const bool focused,
const Core::Point pixelPosition,
const bool pointerPressedInBounds)
{
const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition });
// Returning true from this function indicates that the caller should do no further processing of this movement.
bool handledCompletely = false;

// Short-circuit isReadOnly check to avoid warning dialog
if (focused && !_core->IsInReadOnlyMode() && _canSendVTMouseInput(modifiers))
{
_sendMouseEventHelper(terminalPosition, pointerUpdateKind, modifiers, 0, buttonState);
handledCompletely = 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 @@ -378,6 +381,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

_core->SetHoveredCell(terminalPosition.to_core_point());
return handledCompletely;
}

void ControlInteractivity::TouchMoved(const Core::Point newTouchPoint,
Expand Down Expand Up @@ -682,13 +686,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
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlInteractivity.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const Core::Point pixelPosition);
void TouchPressed(const Core::Point contactPoint);

void PointerMoved(Control::MouseButtonState buttonState,
bool PointerMoved(Control::MouseButtonState buttonState,
const unsigned int pointerUpdateKind,
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers,
const bool focused,
Expand Down
12 changes: 6 additions & 6 deletions src/cascadia/TerminalControl/ControlInteractivity.idl
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ namespace Microsoft.Terminal.Control
Microsoft.Terminal.Core.Point pixelPosition);
void TouchPressed(Microsoft.Terminal.Core.Point contactPoint);

void PointerMoved(MouseButtonState buttonState,
UInt32 pointerUpdateKind,
Microsoft.Terminal.Core.ControlKeyStates modifiers,
Boolean focused,
Microsoft.Terminal.Core.Point pixelPosition,
Boolean pointerPressedInBounds);
Boolean PointerMoved(MouseButtonState buttonState,
UInt32 pointerUpdateKind,
Microsoft.Terminal.Core.ControlKeyStates modifiers,
Boolean focused,
Microsoft.Terminal.Core.Point pixelPosition,
Boolean pointerPressedInBounds);

void TouchMoved(Microsoft.Terminal.Core.Point newTouchPoint,
Boolean focused);
Expand Down
14 changes: 7 additions & 7 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1936,18 +1936,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (type == Windows::Devices::Input::PointerDeviceType::Mouse ||
type == Windows::Devices::Input::PointerDeviceType::Pen)
{
_interactivity.PointerMoved(TermControl::GetPressedMouseButtons(point),
TermControl::GetPointerUpdateKind(point),
ControlKeyStates(args.KeyModifiers()),
_focused,
pixelPosition.to_core_point(),
_pointerPressedInBounds);
auto suppressFurtherHandling = _interactivity.PointerMoved(TermControl::GetPressedMouseButtons(point),
TermControl::GetPointerUpdateKind(point),
ControlKeyStates(args.KeyModifiers()),
_focused,
pixelPosition.to_core_point(),
_pointerPressedInBounds);

// 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
5 changes: 2 additions & 3 deletions src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -998,16 +998,15 @@ namespace ControlUnitTests
VERIFY_IS_GREATER_THAN(core->ScrollOffset(), 0);

// Viewport is now above the mutable viewport, so the mouse event
// straight up won't be sent to the terminal.
// will be clamped to the top line.

expectedOutput.push_back(L"sentinel"); // Clearly, it won't be this string
expectedOutput.push_back(L"\x1b[M &!"); // 5, 1
interactivity->PointerPressed(leftMouseDown,
WM_LBUTTONDOWN, //pointerUpdateKind
0, // timestamp
modifiers,
cursorPosition0.to_core_point());
// Flush it out.
conn->WriteInput(winrt_wstring_to_array_view(L"sentinel"));
VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output");

// This is the part as mentioned in GH#12719
Expand Down

0 comments on commit d1a1f98

Please sign in to comment.