Fix mouse coordinates when viewport is scrolled #10642

PankajBhojwani · 2021-07-12T23:28:19Z

Summary of the Pull Request

Adjust the y-coordinate of the mouse coordinates we send based on how much the viewport has been scrolled

Validation Steps Performed

Validated: cannot repro the issue in #10190

Closes #10190

src/cascadia/TerminalControl/ControlInteractivity.cpp

DHowett

blocking for discussion about what happens to out of bounds coords

src/cascadia/TerminalControl/ControlInteractivity.cpp

miniksa · 2021-07-15T22:23:04Z

blocking for discussion about what happens to out of bounds coords

I'm fine with out of bounds coordinates blackholing since it was said that it matches the known behavior of other terminals.

PankajBhojwani · 2021-07-15T23:07:06Z

I'm fine with out of bounds coordinates blackholing since it was said that it matches the known behavior of other terminals.

Yep, thats what this PR does

DHowett

Hate to be a pill... but since we can now write tests for ControlInteractivity, is there a way you can write a test for this? 😄

DHowett · 2021-07-16T02:10:32Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

@@ -182,7 +182,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        }
        else if (_canSendVTMouseInput(modifiers))
        {
-            _core->SendMouseEvent(terminalPosition, pointerUpdateKind, modifiers, 0, buttonState);
+            const auto adjustment = _core->ScrollOffset() > 0 ? _core->BufferHeight() - _core->ScrollOffset() - _core->ViewHeight() : 0;


I'm surprised we don't already have a thing that calculates "how far back from the top line are we scrolled"... hmm.

PankajBhojwani · 2021-07-16T20:03:40Z

Hate to be a pill... but since we can now write tests for ControlInteractivity, is there a way you can write a test for this? 😄

Test added!

…/pabhoj/mouse_viewport_fix

…crosoft/terminal into dev/pabhoj/mouse_viewport_fix

DHowett · 2021-07-20T21:08:14Z

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

+                                    true, // focused,
+                                    cursorPosition1);
+        Log::Comment(L"Verify that there's still no selection");
+        VERIFY_IS_FALSE(core->HasSelection());


we may not have a way to test that it actually doesn't generate mouse events. oh well 😄

@zadjii-msft any ideas? would we need the mock connection to tell us? that sounds tedious.

ew yea that would probably require a lot more plumbing than it's worth.

ghost · 2021-07-20T21:39:45Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

## Summary of the Pull Request Adjust the y-coordinate of the mouse coordinates we send based on how much the viewport has been scrolled ## Validation Steps Performed Validated: cannot repro the issue in microsoft#10190 Closes microsoft#10190

Adjust the y-coordinate of the mouse coordinates we send based on how much the viewport has been scrolled Validated: cannot repro the issue in #10190 Closes #10190

ghost · 2021-08-31T17:03:06Z

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-08-31T17:08:12Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

…ust pressed (#11290) Does the mouse coordinate adjustment added in #10642 for all the other mouse events as well (moved, released, wheel) Closes #10190

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

adjust sent coord

ecacf2b

ghost added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jul 12, 2021

PankajBhojwani commented Jul 12, 2021

View reviewed changes

src/cascadia/TerminalControl/ControlInteractivity.cpp Outdated Show resolved Hide resolved

carlos-zamora approved these changes Jul 13, 2021

View reviewed changes

src/cascadia/TerminalControl/ControlInteractivity.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalControl/ControlInteractivity.cpp Outdated Show resolved Hide resolved

DHowett requested changes Jul 13, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 13, 2021

ignore clicks outside active region

7f97dfd

DHowett reviewed Jul 14, 2021

View reviewed changes

src/cascadia/TerminalControl/ControlInteractivity.cpp Outdated Show resolved Hide resolved

PankajBhojwani added 2 commits July 14, 2021 13:36

readability

3e38fb2

fix

9867c88

DHowett requested changes Jul 16, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2021

DHowett reviewed Jul 16, 2021

View reviewed changes

PankajBhojwani added 2 commits July 16, 2021 09:51

has

b4e1aea

test

52a19da

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2021

PankajBhojwani added 5 commits July 16, 2021 13:15

dont need nomousedown

a7ce069

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

02f9aa3

…/pabhoj/mouse_viewport_fix

Merge branch 'dev/pabhoj/mouse_viewport_fix' of https://github.com/mi…

a2892f0

…crosoft/terminal into dev/pabhoj/mouse_viewport_fix

conflict

8976a22

fix tests

677b5d2

DHowett reviewed Jul 20, 2021

View reviewed changes

DHowett approved these changes Jul 20, 2021

View reviewed changes

zadjii-msft mentioned this pull request Jul 20, 2021

Tests we should maybe write one day #7001

Open

34 tasks

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 20, 2021

ghost merged commit 6ce2543 into main Jul 20, 2021

ghost deleted the dev/pabhoj/mouse_viewport_fix branch July 20, 2021 21:39

ghost mentioned this pull request Aug 31, 2021

Win32 mouse coordinates wrong when viewport scrolled #10190

Closed

PankajBhojwani mentioned this pull request Sep 20, 2021

Fix mouse coordinates when viewport is scrolled for all events, not just pressed #11290

Merged

DHowett mentioned this pull request Aug 22, 2024

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

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mouse coordinates when viewport is scrolled #10642

Fix mouse coordinates when viewport is scrolled #10642

PankajBhojwani commented Jul 12, 2021 •

edited by DHowett

Loading

DHowett left a comment

miniksa commented Jul 15, 2021

PankajBhojwani commented Jul 15, 2021

DHowett left a comment

DHowett Jul 16, 2021

PankajBhojwani commented Jul 16, 2021

DHowett Jul 20, 2021

zadjii-msft Jul 20, 2021

ghost commented Jul 20, 2021

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

Fix mouse coordinates when viewport is scrolled #10642

Fix mouse coordinates when viewport is scrolled #10642

Conversation

PankajBhojwani commented Jul 12, 2021 • edited by DHowett Loading

Summary of the Pull Request

Validation Steps Performed

DHowett left a comment

Choose a reason for hiding this comment

miniksa commented Jul 15, 2021

PankajBhojwani commented Jul 15, 2021

DHowett left a comment

Choose a reason for hiding this comment

DHowett Jul 16, 2021

Choose a reason for hiding this comment

PankajBhojwani commented Jul 16, 2021

DHowett Jul 20, 2021

Choose a reason for hiding this comment

zadjii-msft Jul 20, 2021

Choose a reason for hiding this comment

ghost commented Jul 20, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

PankajBhojwani commented Jul 12, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.