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.

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

ScrollRegion function generates inappropriate EVENT_CONSOLE_UPDATE_SCROLL #12656

Open
j4james opened this issue Mar 10, 2022 · 1 comment
Open
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase
Milestone

Comments

@j4james
Copy link
Collaborator

j4james commented Mar 10, 2022

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 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, it makes a call to _ScrollScreen, which then makes a call to NotifyConsoleUpdateScrollEvent like this:

pNotifier->NotifyConsoleUpdateScrollEvent(target.Origin().X - source.Left(), target.Origin().Y - source.RightInclusive());

That code is a bit obfuscated by the use of Origin, but it essentially boils down to:

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.

@j4james j4james added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Mar 10, 2022
@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 Mar 10, 2022
@j4james
Copy link
Collaborator Author

j4james commented Mar 10, 2022

Btw, I thought at first that it might make sense to send an EVENT_CONSOLE_UPDATE_REGION here, indicating the area of the screen that had changed, but I suspect that's not right either. I tried that out with the screen reader running, and it forces it to start reading that whole area of the screen again. If you're doing something that scrolls in a window, I would think all you care about is the newly revealed lines - you don't need to be told about the parts that have just moved and which you've likely already seen.

@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Priority-3 A description (P3) labels Mar 29, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 29, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone Mar 29, 2022
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 29, 2022
ghost pushed a commit that referenced this issue Apr 14, 2022
This is an attempt to simplify the `ConGetSet` interface down to the
smallest set of methods necessary to support the `AdaptDispatch` class.
The idea is that it should then be easier to implement that interface in
Windows Terminal, so we can replace the `TerminalDispatch` class with
`AdaptDispatch`.

This is a continuation of the refactoring started in #12247, and a
significant step towards #3849.

## Detailed Description of the Pull Request / Additional comments

The general idea was to give the `AdaptDispatch` class direct access to
the high-level structures on which it needs to operate. Some of these
structures are now passed in when the class is constructed (the
`Renderer`, `RenderSettings`, and `TerminalInput`), and some are exposed
as new methods in `ConGetSet` (`GetStateMachine`, `GetTextBuffer`, and
`GetViewport`).

Many of the existing `ConhostInternalGetSet` methods could easily then
be reimplemented in `AdaptDispatch`, since they were often simply
forwarding to methods in one of the above structures. Some were a little
more complicated, though, and require further explanation.

* `GetConsoleScreenBufferInfoEx`: What we were typically using this for
  was to obtain the viewport, although what we really wanted was the
  virtual viewport, which is now accessible via the `GetViewport`
  method. This was also used to obtain the cursor position and buffer
  width, which we can now get via the `GetTextBuffer` method.

* `SetConsoleScreenBufferInfoEx`: This was only really used for the
  `AdaptDispatch::SetColumns` implementation (for `DECCOLM` mode), and
  that could be replaced with `ResizeWindow`. This is a slight change in
  behaviour (it sizes the window rather than the buffer), but neither is
  technically correct for `DECCOLM`, so I think it's good enough for
  now, and at least it's consistent with the other VT sizing operations.

* `SetCursorPosition`: This could mostly be replaced with direct
  manipulation of the `Cursor` object (accessible via the text buffer),
  although again this is a slight change in behavior. The original code
  would also have made a call to `ConsoleImeResizeCompStrView` (which I
  don't think is applicable to VT movement), and would potentially have
  moved the viewport (not essential for now, but could later be
  supported by `DECHCCM`). It also called `VtIo::SetCursorPosition` to
  handle cursor inheritance, but that should only apply to
  `InteractDispatch`, so I've moved that to the
  `InteractDispatch::MoveCursor` method.

* `ScrollRegion`: This has been replaced by two simple helper methods in
  `AdaptDispatch` which better meet the VT requirements -
  `_ScrollRectVertically` and `_ScrollRectHorizontally`. Unlike the
  original `ScrollRegion` implementation, these don't generate
  `EVENT_CONSOLE_UPDATE_SCROLL` events (see #12656 for more details).

* `FillRegion`: This has been replaced by the `_FillRect` helper method
  in `AdaptDispatch`. It differs from the original `FillRegion` in that
  it takes a rect rather than a start position and length, which gives
  us more flexibility for future operations.

* `ReverseLineFeed`: This has been replaced with a somewhat refactored
  reimplementation in `AdaptDispatch`, mostly using the
  `_ScrollRectVertically` helper described above.

* `EraseAll`: This was previously handled by
  `SCREEN_INFORMATION::VtEraseAll`, but has now been entirely
  reimplemented in the `AdaptDispatch::_EraseAll` method.

* `DeleteLines`/`InsertLines`/`_modifyLines`: These have been replaced
  by the `_InsertDeleteLineHelper` method in `AdaptDispatch`, which
  mostly relies on the `_ScrollRectVertically` helper described above.

Finally there were a few methods that weren't actually needed in the
`ConGetSet` interface:

* `MoveToBottom`: This was really just a hack to get the virtual
  viewport from `GetConsoleScreenBufferInfoEx`. We may still want
  something like in the future (e.g. to support `DECVCCM` or #8879), but
  I don't think it's essential for now.

* `SuppressResizeRepaint`: This was only needed in `InteractDispatch`
  and `PtySignalInputThread`, and they could easily access the `VtIo`
  object to implement it themselves.

* `ClearBuffer`: This was only used in `PtySignalInputThread`, and that
  could easily access the buffer directly via the global console
  information.

* `WriteControlInput`: This was only used in `InteractDispatch`, and
  that could easily be replaced with a direct call to
  `HandleGenericKeyEvent`.

As part of these changes, I've also refactored some of the existing
`AdaptDispatch` code:

* `_InsertDeleteHelper` (renamed `_InsertDeleteCharacterHelper`) is now
  just a straightforward call to the new `_ScrollRectHorizontally`
  helper.

* `EraseInDisplay` and `EraseInLine` have been implemented as a series
  of `_FillRect` calls, so `_EraseSingleLineHelper` is no longer
  required.

* `_EraseScrollback` is a essentially a special form of scrolling
  operation, which mostly depends on the `TextBuffer::ScrollRows`
  method, and with the filling now provided by the new `_FillRect`
  helper.

* There are quite a few operations now in `AdaptDispatch` that are
  affected by the scrolling margins, so I've pulled out the common
  margin setup into a new `_GetVerticalMargins` helper method. This also
  fixes some edge cases where margins could end up out of range.

## Validation Steps Performed
There were a number of unit tests that needed to be updated to work
around functions that have now been removed, but these substitutions
were fairly straightforward for the most part.

The adapter tests were a different story, though. In that case we were
explicitly testing how operations were passed through to the `ConGetSet`
interface, but with more than half those methods now gone, a significant
rewrite was required.

I've tried to retain the crux of the original tests, but we now have to
validate the state changes on the underlying data structures, where
before that state would have been tracked in the `TestGetSet` mock. And
in some cases we were testing what happened when a method failed, but
since that scenario is no longer possible, I've simply removed those
tests.

I've also tried to manually test all the affected operations to confirm
that they're still working as expected, both in vttest as well as my own
test scripts.

Closes #12662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

2 participants