Fix negative delta scroll

[penguin-sophist]

Summary of the Pull Request

This fixes the negative delta scroll by correctly allowing for 16-bit to 32-bit sign extension by implicit type promotion.

References and Relevant Issues

#19484 #19391 (I might be wrong about the second one, but it looks similar since the thumb of the scrollbar is stuck on the top)

Detailed Description of the Pull Request / Additional comments

Hello!
After updating to the latest Windows Terminal Preview version, I've been experiencing an issue with the touchpad, when scrolling.
To be more precise, it started since v1.24.2682.0.
The issue happens only when scrolling down (that is to say when the thumb of the scrollbar should move down).
And, basically, it scrolls in reverse all the way up even with a minimum amount of scroll.
Scrolling up, instead, works as expected.

Given the premise, I decided to give it a chance and see if I could fix it.
I don't have the needed VS environment to compile the code, so I used a debugger (x64dbg) to adventure myself in this "quest".
After literally 3 hours of misplaced breakpoints I started to notice a pattern.
For starters the interesting code path is the AppHost::_WindowMouseWheeled method.

I'll add a visual screenshot diff between the Preview and Stable version and I'll try my best to explain in words what I discovered.
By the way, I made sure to scroll the same amount in both versions so they are easily comparable.

Screenshots

Windows Terminal Preview

Windows Terminal Stable

What's really interesting is the R8 register. It contains the delta scroll and it corresponds to the second parameter of the _WindowMouseWheeled method.

• In the Stable version it's a int32_t stored as 32-bit in the second half of the register.
• In the Preview version it became a winrt::Microsoft::Terminal::Core::Point which seems to be nicely packed into the R8 register as Y + X (32-bit + 32-bit, which is cool!).

However, despite this, there's another important difference between the two versions: it's the sign part of the delta scroll.
In the Stable version it's correctly stored as a 32-bit signed integer, but in the Preview version it's stored as a 16-bit signed integer which is wrong, because it's going to be read as a 32-bit signed integer.

Basically:

• Stable 0xFFFFFF58 -> -168
• Preview 0x0000FF58 -> 65368

In the Preview version it becomes a huge positive number and that's why the thumb of the scrollbar goes all the way up.
Just to be sure I tried to manually patch the R8 register to its correct value FFFFFF5800000000 and it works as expected.

When looking at the source code and, specifically, the b53d7df commit, there's this code change to the IslandWindow.cpp file which seems to be the source of the issue:

- const auto wheelDelta = static_cast<short>(HIWORD(wparam));
+ winrt::Microsoft::Terminal::Core::Point wheelDelta{ 0, static_cast<int32_t>(HIWORD(wparam)) };

I literally went by exclusion since that's where the param is set and nothing else happens in-between (there's also a std::swap, but it's not really relevant).
So I had an idea and I tried to write a really small C++ program to test the integer conversion. And, indeed, I could replicate the same issue.
I also mocked the HIWORD macro just to be sure I had a similar approach to that of the original code.

Here's the snippet:

Code Snippet

#include <cstdint>
#include <cstdio>

typedef uint16_t WORD;
typedef uintptr_t DWORD_PTR;

#define HIWORD(l) ((WORD)((((DWORD_PTR)(l)) >> 16) & 0xffff))

int main(int argc, char** argv)
{
    // Maybe the wparam is 64-bits, but it's the same either way.
    uint32_t wparam = 0xFF58AAAA;

    const auto delta_before = static_cast<short>(HIWORD(wparam)); // The code before the b53d7df commit
    const int32_t delta_after = static_cast<int32_t>(HIWORD(wparam)); // The code after the b53d7df commit
    const int32_t fixed_delta = static_cast<int16_t>(HIWORD(wparam)); // My fix

    printf("%.8X\n", delta_before); // Outputs: FFFFFF58 (correct)
    printf("%.8X\n", delta_after); // Outputs: 0000FF58 (wrong)
    printf("%.8X\n", fixed_delta); // Outputs: FFFFFF58 (correct)

    return 0;
}

I compiled the snippet with gcc --pedantic --std=c++17 -Wall -O0 -o test main.cpp, but I suppose the same output should apply for Visual Studio.

That being said, I think there are two potential approaches to fix the issue:

// Fix 1
winrt::Microsoft::Terminal::Core::Point wheelDelta{ 0, static_cast<int32_t>(static_cast<int16_t>(HIWORD(wparam))) };

// Fix 2
winrt::Microsoft::Terminal::Core::Point wheelDelta{ 0, static_cast<int16_t>(HIWORD(wparam)) };

The first fix is much more verbose and it was the one I wanted to commit, just to be safe (since I can't really test the whole Terminal compilation).
The second one, which I ended up choosing, relies on the implicit type promotion, but it also should work (or at least I hope so!).

As a bonus, since I was already in the debugger rabbit hole, I wanted to see where the proper sign conversion happened in the Stable version and here it is:

Screenshot

Windows Terminal Stable

There's an intermediate function call where the movsx eax, word ptr ds:[r8] instruction is used.
Which is literally Move with Sign-Extension. It takes the delta scroll from the stack to "fix" it into eax.

One last fun thing: if I enable the precision touchpad, in Windows settings, the issue doesn't happen at all.
That's because a completely different code path is executed, namely TermControl::_MouseWheelHandler.

I apologize for the long message considering the fix is really tiny!

Validation Steps Performed

Tested with a debugger and wrote a code snippet

PR Checklist

• Closes wt.exe scrolling locks up #19484 Mouse and touchpad malfunctions and crashes #19391
• Tests added/passed
• Documentation updated
  • If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated (if necessary)