Christopher Thielen cthielen@gmail.com writes:
Fixes https://bugs.winehq.org/show_bug.cgi?id=13683
A window may be notified with WM_CAPTURECHANGED about itself gaining mouse capture if it calls SetCapture() twice.
You'd need to review all places where we handle that message, I don't think we've taken that behavior into account.
Because of this risk, I'm afraid it will have to wait until after code freeze.
Thanks Alexandre, that seems wise.
I'll get a review ready after 1.8.
On Monday, November 23, 2015, Alexandre Julliard julliard@winehq.org wrote:
Christopher Thielen <cthielen@gmail.com javascript:;> writes:
Fixes https://bugs.winehq.org/show_bug.cgi?id=13683
A window may be notified with WM_CAPTURECHANGED about itself gaining mouse capture if it calls SetCapture() twice.
You'd need to review all places where we handle that message, I don't think we've taken that behavior into account.
Because of this risk, I'm afraid it will have to wait until after code freeze.
-- Alexandre Julliard julliard@winehq.org javascript:;
Attached is my analysis of Wine's usage of WM_CAPTURECHANGED as requested by Alexandre.
Reading the code has revealed the same logic change I originally proposed -- allowing a WM_CAPTURECHANGED message to be sent to a window losing focus if it was also the window gaining focus -- is needed in the Mac driver as well. The Mac driver logic looks copied out of dlls/user32/input.c.
I would appreciate if someone could double-check my analysis in a few areas based on expertise:
dlls/comctl32/toolbar.c dlls/comctl32/trackbar.c dlls/user32/button.c programs/regedit/childwnd.c
If no one has any concerns after a few days, I'll resubmit my patch with the additional change mimicked for the Mac driver.
On 11/25/2015 01:51 PM, Chris Thielen wrote:
Thanks Alexandre, that seemswise.
I'll get a review ready after 1.8.
On Monday, November 23, 2015, Alexandre Julliard <julliard@winehq.org mailto:julliard@winehq.org> wrote:
Christopher Thielen <cthielen@gmail.com <javascript:;>> writes: > Fixes https://bugs.winehq.org/show_bug.cgi?id=13683 > > A window may be notified with WM_CAPTURECHANGED about itself > gaining mouse capture if it calls SetCapture() twice. You'd need to review all places where we handle that message, I don't think we've taken that behavior into account. Because of this risk, I'm afraid it will have to wait until after code freeze. -- Alexandre Julliard julliard@winehq.org <javascript:;>
On Jan 12, 2016, at 10:27 PM, Christopher Thielen cthielen@gmail.com wrote:
Attached is my analysis of Wine's usage of WM_CAPTURECHANGED as requested by Alexandre.
Reading the code has revealed the same logic change I originally proposed -- allowing a WM_CAPTURECHANGED message to be sent to a window losing focus if it was also the window gaining focus -- is needed in the Mac driver as well. The Mac driver logic looks copied out of dlls/user32/input.c.
dlls/winemac.drv/window.c: SendMessageW(previous, WM_CAPTURECHANGED, 0, (LPARAM)hwnd);
Appears to be in need of the same patch as I'm suggesting. Any idea why this code appears duplicated? Something about OS X having a different window server than *nix?
The duplication is to enforce Mac-appropriate window moving behaviors, like preventing the user from dragging a window's title bar behind the Mac menu bar.
I don't think your change is needed here, although I also don't think it would hurt anything. The code in the Mac driver is only called during a window move. It is not used when a program calls SetCapture(), which I understand is the case you're interested in.
-Ken
Thanks Ken!
You're right that I'm only concerned with missing WM_CAPTURECHANGED messages from SetCapture() calls, so I won't attempt to patch the Mac driver code.
On 01/13/2016 09:43 AM, Ken Thomases wrote:
On Jan 12, 2016, at 10:27 PM, Christopher Thielen cthielen@gmail.com wrote:
Attached is my analysis of Wine's usage of WM_CAPTURECHANGED as requested by Alexandre.
Reading the code has revealed the same logic change I originally proposed -- allowing a WM_CAPTURECHANGED message to be sent to a window losing focus if it was also the window gaining focus -- is needed in the Mac driver as well. The Mac driver logic looks copied out of dlls/user32/input.c.
dlls/winemac.drv/window.c: SendMessageW(previous, WM_CAPTURECHANGED, 0, (LPARAM)hwnd);
Appears to be in need of the same patch as I'm suggesting. Any idea why this code appears duplicated? Something about OS X having a different window server than *nix?
The duplication is to enforce Mac-appropriate window moving behaviors, like preventing the user from dragging a window's title bar behind the Mac menu bar.
I don't think your change is needed here, although I also don't think it would hurt anything. The code in the Mac driver is only called during a window move. It is not used when a program calls SetCapture(), which I understand is the case you're interested in.
-Ken
Christopher Thielen cthielen@gmail.com writes:
dlls/comctl32/toolbar.c: case WM_CAPTURECHANGED:
Probably fine? Appears to be a ToolbarProc translating window messages into toolbar-based equivalents.
That's too superficial, you need to look into what it's doing with the message. In this case it's clearly assuming that capture is lost, which wouldn't be the case with your change. The same thing probably applies to other message handlers.
Thank you Alexandre, you're absolutely right.
Below is a further analysis on toolbar, trackbar, and button:
Toolbar: Assumes a WM_CAPTURECHANGED messages implies a toolbar button is no longer pressed. The proposed patch would therefore break existing behavior if and only if the toolbar code also calls SetCapture() on itself while it already has capture. Toolbar currently only calls SetCapture() in the case of a LButtonDown on itself. Notably, another piece of code could in theory call SetCapture() on the toolbar's hWnd as well.
Trackbar: Assumes a WM_CAPTURECHANGED message implies a TB_ENDTRACK, again, only breaking current behavior if trackbar calls SetCapture() while it already has mouse capture. Like Toolbar above, Trackbar only calls SetCapture() if it receives a LButtonDown. Again, another piece of code could in theory call SetCapture() and pass the trackbar's hWnd even when it already has mouse focus.
dlls/user32/button.c: Causes a button with mouse focus and BUTTON_BTNPRESSED to lose its highlighted state as documented at https://msdn.microsoft.com/en-us/library/aa931528.aspx. Like the two cases above, button would need to call SetCapture() on itself for this patch's behavioral changes to be relevant. Button calls SetCapture() on KEYDOWN, LBUTTONDBCLK, and LBUTTONDOWN.
I would be grateful if a seasoned Wine developer could chime in as to whether or not the above scenarios are possible, or what further research they would recommend. Otherwise, one safe course of action could be to emulate the existing Wine behavior of requiring the newly mouse-focused hWnds to be different from the existing hWnd in order for these three code paths to run (thus maintaining the current behavior for these specific cases). That would correct the SetCapture() behavior, fix the documented bug, and prevent any side effects in this code, though I'd prefer to better understand the above three code paths than rely on that idea.
Thank you again to anyone who can offer any guidance.
On 01/14/2016 03:02 AM, Alexandre Julliard wrote:
Christopher Thielen cthielen@gmail.com writes:
dlls/comctl32/toolbar.c: case WM_CAPTURECHANGED:
Probably fine? Appears to be a ToolbarProc translating window messages into toolbar-based equivalents.
That's too superficial, you need to look into what it's doing with the message. In this case it's clearly assuming that capture is lost, which wouldn't be the case with your change. The same thing probably applies to other message handlers.
Christopher Thielen cthielen@gmail.com writes:
Thank you Alexandre, you're absolutely right.
Below is a further analysis on toolbar, trackbar, and button:
Toolbar: Assumes a WM_CAPTURECHANGED messages implies a toolbar button is no longer pressed. The proposed patch would therefore break existing behavior if and only if the toolbar code also calls SetCapture() on itself while it already has capture. Toolbar currently only calls SetCapture() in the case of a LButtonDown on itself. Notably, another piece of code could in theory call SetCapture() on the toolbar's hWnd as well.
Trackbar: Assumes a WM_CAPTURECHANGED message implies a TB_ENDTRACK, again, only breaking current behavior if trackbar calls SetCapture() while it already has mouse capture. Like Toolbar above, Trackbar only calls SetCapture() if it receives a LButtonDown. Again, another piece of code could in theory call SetCapture() and pass the trackbar's hWnd even when it already has mouse focus.
dlls/user32/button.c: Causes a button with mouse focus and BUTTON_BTNPRESSED to lose its highlighted state as documented at https://msdn.microsoft.com/en-us/library/aa931528.aspx. Like the two cases above, button would need to call SetCapture() on itself for this patch's behavioral changes to be relevant. Button calls SetCapture() on KEYDOWN, LBUTTONDBCLK, and LBUTTONDOWN.
I would be grateful if a seasoned Wine developer could chime in as to whether or not the above scenarios are possible, or what further research they would recommend. Otherwise, one safe course of action could be to emulate the existing Wine behavior of requiring the newly mouse-focused hWnds to be different from the existing hWnd in order for these three code paths to run (thus maintaining the current behavior for these specific cases). That would correct the SetCapture() behavior, fix the documented bug, and prevent any side effects in this code, though I'd prefer to better understand the above three code paths than rely on that idea.
As you said, other code may call SetCapture, or the controls themselves could be modified to call it at some later date. So the window procs should be updated to verify that the capture has really changed before proceeding.
-
Alexandre Julliard -
Chris Thielen -
Christopher Thielen
-
Ken Thomases