Repository: microsoft/terminal (language: cpp)

Issue:
Scroll wheel no longer works correctly in OpenConsole
### Windows Terminal version

Commit ed27737233714dea77877624d1beeb49e2ccd36e

### Windows build number

10.0.19044.1706

### Other Software

_No response_

### Steps to reproduce

1. Checkout commit ed27737233714dea77877624d1beeb49e2ccd36e or later.
2. Build and run OpenConsole.
3. Perform a few directory listings or something to get a couple of pages into the buffer.
4. Use the scroll wheel to scroll back in the buffer.
5. Use the scroll wheel to scroll forward again.


### Expected Behavior

Scrolling should work correctly in both directions.

### Actual Behavior

When I attempt to scroll forward, the viewport immediately jumps back to the start of the buffer.

I did a git bisect, and it looks like this problem may have been introduced by commit ed27737233714dea77877624d1beeb49e2ccd36e.

/cc @lhecker 


Discussion / hints:
I'll check this out as soon as I can. Quite curious that this happens because I did notice this exact bug while I was developing the PR and fixed it.
It can occur when you aren't careful when extracting the wheel delta, as it's stored as a signed 16-bit integer in the high word of the `wParam`: https://github.com/microsoft/terminal/blob/4e20a8631cbb5b20defe549a0e75bc7719969bee/src/interactivity/win32/windowproc.cpp#L653

Initially I had replaced all `(short)` with `(til::CoordType)` which caused large jumps during scrolling. This appears to be the exact same bug? Wheel deltas (e.g. scrolling down once: `int16_t(-3)`) gets passed around as unsigned integers (here: `uint16_t(65532)`). But `(til::CoordType)uint16_t(65532)` isn't the same as `(short)uint16_t(65532)`...
FYI, I think the issue might be here:
https://github.com/microsoft/terminal/blob/ed27737233714dea77877624d1beeb49e2ccd36e/src/host/scrolling.cpp#L145

We're dividing a short by an unsigned long, which I think is treated as an unsigned division, and that results in a massive positive delta. Previously the `ulActualDelta` was cast to `short` here, so it was a signed division.

Fix patch:
diff --git a/src/host/scrolling.cpp b/src/host/scrolling.cpp
index 6d211a0fd3d..8e7b122733b 100644
--- a/src/host/scrolling.cpp
+++ b/src/host/scrolling.cpp
@@ -13,13 +13,13 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
 using namespace Microsoft::Console::Interactivity;
 using namespace Microsoft::Console::Types;
 
-ULONG Scrolling::s_ucWheelScrollLines = 0;
-ULONG Scrolling::s_ucWheelScrollChars = 0;
+til::CoordType Scrolling::s_ucWheelScrollLines = 0;
+til::CoordType Scrolling::s_ucWheelScrollChars = 0;
 
 void Scrolling::s_UpdateSystemMetrics()
 {
-    s_ucWheelScrollLines = ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollLines();
-    s_ucWheelScrollChars = ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollCharacters();
+    s_ucWheelScrollLines = ::base::saturated_cast<decltype(s_ucWheelScrollLines)>(ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollLines());
+    s_ucWheelScrollChars = ::base::saturated_cast<decltype(s_ucWheelScrollChars)>(ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollCharacters());
 }
 
 bool Scrolling::s_IsInScrollMode()
@@ -112,7 +112,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
     if (isMouseWheel && s_ucWheelScrollLines > 0)
     {
         // Rounding could cause this to be zero if gucWSL is bigger than 240 or so.
-        const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollLines, 1ul);
+        const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollLines, 1);
 
         // If we change direction we need to throw away any remainder we may have in the other direction.
         if ((ScreenInfo.WheelDelta > 0) == (wheelDelta > 0))
@@ -124,7 +124,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
             ScreenInfo.WheelDelta = wheelDelta;
         }
 
-        if ((ULONG)abs(ScreenInfo.WheelDelta) >= ulActualDelta)
+        if (abs(ScreenInfo.WheelDelta) >= ulActualDelta)
         {
             /*
             * By default, SHIFT + WM_MOUSEWHEEL will scroll 1/2 the
@@ -134,7 +134,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
             til::CoordType delta = 1;
             if (hasShift)
             {
-                delta = std::max((ScreenInfo.GetViewport().Height() * ScreenInfo.ScrollScale) / 2, 1u);
+                delta = std::max((ScreenInfo.GetViewport().Height() * ::base::saturated_cast<til::CoordType>(ScreenInfo.ScrollScale)) / 2, 1);
 
                 // Account for scroll direction changes by adjusting delta if there was a direction change.
                 delta *= (ScreenInfo.WheelDelta < 0 ? -1 : 1);
@@ -161,7 +161,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
     }
     else if (isMouseHWheel && s_ucWheelScrollChars > 0)
     {
-        const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollChars, 1ul);
+        const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollChars, 1);
 
         if ((ScreenInfo.HWheelDelta > 0) == (wheelDelta > 0))
         {
@@ -172,7 +172,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel,
             ScreenInfo.HWheelDelta = wheelDelta;
         }
 
-        if ((ULONG)abs(ScreenInfo.HWheelDelta) >= ulActualDelta)
+        if (abs(ScreenInfo.HWheelDelta) >= ulActualDelta)
         {
             til::CoordType delta = 1;
 
diff --git a/src/host/scrolling.hpp b/src/host/scrolling.hpp
index a082fa21a15..63956660fa1 100644
--- a/src/host/scrolling.hpp
+++ b/src/host/scrolling.hpp
@@ -41,6 +41,6 @@ class Scrolling
 private:
     static BOOL s_IsPointInRectangle(const til::rect* prc, const til::point pt);
 
-    static ULONG s_ucWheelScrollLines;
-    static ULONG s_ucWheelScrollChars;
+    static til::CoordType s_ucWheelScrollLines;
+    static til::CoordType s_ucWheelScrollChars;
 };


Test patch: