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

Jump to bottom

Win32 mouse coordinates wrong when viewport scrolled #10190

Closed
malxau opened this issue May 25, 2021 · 12 comments · Fixed by #10642 or #11290
Closed

Win32 mouse coordinates wrong when viewport scrolled #10190

malxau opened this issue May 25, 2021 · 12 comments · Fixed by #10642 or #11290
Assignees
@PankajBhojwani
Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.12

Comments

@malxau
Copy link
Contributor

malxau commented May 25, 2021

Windows Terminal version (or Windows build number)

1.9.1445.0

Other Software

Using Yori's mouseover support, but anything else should work. It's just unnatural to observe this with a TUI application like FAR manager (why scroll the viewport for a full screen TUI application?) However, scrolling the viewport should show the same behavior for anything.

Steps to reproduce

  1. Fill the console with output until it scrolls.
  2. Manually scroll the viewport up.
  3. Generate mouse events and observe the coordinates that the event contains.

Expected Behavior

Since I know Dustin doesn't like the idea of applications being able to address regions that have scrolled off the visible portion of the display at the bottom, I was expecting the application to observe coordinates corresponding to mouse location if that location is visible at the bottom of the viewport. If the cell is not visible at the bottom of the viewport, I was expecting the reported location to be capped.

Actual Behavior

The reported mouse coordinates assume that the window has not scrolled and report coordinates on the assumption that the visible viewport is the same as the bottom of the scroll region. This causes the application to highlight/select the wrong text, several lines below where the mouse is positioned.

terminal_mouseover
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 25, 2021
@DHowett
Copy link
Member

DHowett commented May 26, 2021

If I had to guess, this is a regression from the recent Interactivity work in TermControl. @zadjii-msft mind having a look?

@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels May 26, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 26, 2021
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label May 26, 2021
@DHowett
Copy link
Member

DHowett commented May 27, 2021

Confirming that all mouse mode input happens relative to the control and not to the viewport after #9820.

@DHowett DHowett added this to the Terminal v1.10 milestone May 27, 2021
@DHowett DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 27, 2021
@DHowett DHowett added Priority-1 A description (P1) and removed Priority-2 A description (P2) labels Jul 12, 2021
@DHowett
Copy link
Member

DHowett commented Jul 12, 2021

It looks like this isn't a regression from 1.8 (Stable) due to the Control/Interactivity refactoring. I'm going to demote it back to P2 and schedule it for 1.11!

@DHowett DHowett added Priority-2 A description (P2) and removed Priority-1 A description (P1) zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Jul 12, 2021
@PankajBhojwani PankajBhojwani mentioned this issue Jul 12, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Jul 12, 2021
@ghost ghost closed this as completed in #10642 Jul 20, 2021
ghost pushed a commit that referenced this issue Jul 20, 2021
@PankajBhojwani
Fix mouse coordinates when viewport is scrolled (#10642)
6ce2543 
## 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
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 20, 2021
Rosefield pushed a commit to Rosefield/terminal that referenced this issue Jul 22, 2021
@PankajBhojwani @Rosefield
Fix mouse coordinates when viewport is scrolled (microsoft#10642)
be4db0a 
## 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
DHowett pushed a commit that referenced this issue Aug 25, 2021
@PankajBhojwani @DHowett
Fix mouse coordinates when viewport is scrolled (#10642)
2f6530c 
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
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10642, which has now been successfully released as Windows Terminal Preview v1.10.2383.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10642, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

@malxau
Copy link
Contributor Author

malxau commented Sep 20, 2021

I still see the buggy behavior on Terminal 1.11.2421.0, which is supposed to have this fix.

@DHowett
Copy link
Member

DHowett commented Sep 20, 2021

Huh. I am as well.

@PankajBhojwani, did this regress? If so, why didn't the test catch it?

@DHowett DHowett reopened this Sep 20, 2021
@DHowett DHowett removed the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Sep 20, 2021
@DHowett
Copy link
Member

DHowett commented Sep 20, 2021

Since I can't send you messages images on Teams (thanks Teams):

wt-1 12-bug-mouse-offset

@PankajBhojwani PankajBhojwani mentioned this issue Sep 20, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Sep 20, 2021
@PankajBhojwani
Copy link
Contributor

PankajBhojwani commented Sep 20, 2021 •
edited
Loading

This never regressed, turns out we only did the mouse coordinate adjustment for pointer pressed, not for release/move/wheel 🤦

Pushed a PR for it, and the test didn't catch it because we don't actually have a way right now to test if mouse events were generated

@ghost ghost closed this as completed in #11290 Sep 22, 2021
ghost pushed a commit that referenced this issue Sep 22, 2021
@PankajBhojwani
Fix mouse coordinates when viewport is scrolled for all events, not j…
f04fd08 
…ust pressed (#11290)

Does the mouse coordinate adjustment added in #10642 for all the other mouse events as well (moved, released, wheel)

Closes #10190
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 22, 2021
miniksa pushed a commit that referenced this issue Sep 27, 2021
@PankajBhojwani @miniksa
Fix mouse coordinates when viewport is scrolled for all events, not j…
f86d02e 
…ust pressed (#11290)

Does the mouse coordinate adjustment added in #10642 for all the other mouse events as well (moved, released, wheel)

Closes #10190
miniksa pushed a commit that referenced this issue Sep 27, 2021
@PankajBhojwani @miniksa
Fix mouse coordinates when viewport is scrolled for all events, not j…
7965b39 
…ust pressed (#11290)

Does the mouse coordinate adjustment added in #10642 for all the other mouse events as well (moved, released, wheel)

Closes #10190
@ghost
Copy link

ghost commented Oct 4, 2021

🎉This issue was addressed in #11290, which has now been successfully released as Windows Terminal v1.10.2714.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Oct 4, 2021

🎉This issue was addressed in #11290, which has now been successfully released as Windows Terminal Preview v1.11.2731.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Oct 20, 2021

🎉This issue was addressed in #11290, which has now been successfully released as Windows Terminal Preview v1.12.2922.0.:tada:

Handy links:

@DHowett DHowett mentioned this issue Aug 22, 2024
Merged
DHowett added a commit that referenced this issue Aug 23, 2024
@DHowett
Restore off-the-top behavior for VT mouse mode (#17779)
d1a1f98 
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
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PankajBhojwani PankajBhojwani

Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.12
4 participants
@DHowett @malxau @zadjii-msft @PankajBhojwani