microsoft / microsoft/terminal
ScrollRegion function generates inappropriate EVENT_CONSOLE_UPDATE_SCROLL
- Dominant language
- C++
- Stars
- 105k
- Forks
- 9.6k
- Avg merge
- 3d 17h
- Merged PRs (30d)
- 29
Description
### Windows Terminal version
n/a
### Windows build number
10.0.19041.1415
### Other Software
_No response_
### Steps to reproduce
The `ScrollRegion` function is used in conhost to "scroll" an area of the screen buffer in multiple directions. It's used in a variety of places, but the closest equivalent in the public API is the [ScrollConsoleScreenBuffer](https://docs.microsoft.com/en-us/windows/console/scrollconsolescreenbuffer) function. It doesn't pan the viewport, as typically occurs when the console scrolls - it's more akin to copying a chunk of the buffer from one position to another.
But if you look at the code for [`ScrollRegion`](https://github.com/microsoft/terminal/blob/3bd3a4f712006d752b59d66e104aeb6bc5f7f9d6/src/host/output.cpp#L436-L437), it makes a call to `_ScrollScreen`, which then makes a call to `NotifyConsoleUpdateScrollEvent` like this:
https://github.com/microsoft/terminal/blob/3bd3a4f712006d752b59d66e104aeb6bc5f7f9d6/src/host/output.cpp#L279
That code is a bit obfuscated by the use of `Origin`, but it essentially boils down to:
```cpp
pNotifier->NotifyConsoleUpdateScrollEvent(target.Left() - source.Left(), target.Top() - source.RightInclusive());
```
Quite clearly the second parameter is nonsense.
That said, I'm not sure a scroll event should be used here at all. As mentioned above, it's not the equivalent of the viewport scrolling, so I don't think it makes sense to trigger an `EVENT_CONSOLE_UPDATE_SCROLL`.
Hopefully someone that is more knowledgable about accessibility can chime in here, but it seemed to me that `EVENT_CONSOLE_UPDATE_SCROLL` was how you would track the movement of the viewport. Other update events use absolute coordinates, so you need to know the viewport position in order to obtain their relative location. And if something like `ScrollRegion` is sending unrelated scroll events, that makes that data useless.
### Expected Behavior
I don't think `ScrollRegion` should be triggering `EVENT_CONSOLE_UPDATE_SCROLL` at all, but if it does, it should at least give meaningful deltas.
### Actual Behavior
`ScrollRegion` is triggering meaningless `EVENT_CONSOLE_UPDATE_SCROLL` notifications.
Contributor guide
Research direction
Start in src/host/output.cpp at ScrollRegion and follow its call to _ScrollScreen and NotifyConsoleUpdateScrollEvent. Compare the notification's arguments with the viewport-scrolling behavior and determine whether this operation should emit EVENT_CONSOLE_UPDATE_SCROLL. Done means the notification behavior is correct for ScrollRegion and any resulting deltas are meaningful.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- accessibility, operating-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100