632953 - After using the resizer on XP, the next mouse click on a chrome area is ignored

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Asa Dotzler [:asa]]

Steps to reproduce:
1. with a restored Firefox window, drag the corner re-sizer to re-size the window (make sure you are on the triangle re-sizer image and not on the very outside edge where you would be hitting the window frame and not the Firefox custom re-sizer image) 
2. click and drag the title bar to move the window. 

Results: the drag fails and requires a second click because the focus was on the content area rather than the window decorations.

This is pretty ugly and makes managing window size and position painful. It also has the effect of making the user feel a little bit stupid each time it happens. 

Tested with today's nightly build on Win XP.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

cc'ing Jimm, overall this sounds like it should be a blocker, having to move focus to the window frame is a significant regression.

Comment 2

[Jim Mathies [:jimm]]

(In reply to comment #1)
> cc'ing Jimm, overall this sounds like it should be a blocker, having to move
> focus to the window frame is a significant regression.

Hmm, this would be controlled by the drag window jsm code, or it's focus related maybe, not sure. You're clicking on xul content there, so I doubt it's a widget issue.

Comment 3

[Neil Deakin]

Window resizing is done on Windows by widget level code in nsWindow::BeginResizeDrag.

Messages that occur:

Mousedown on resizer:
 WM_LBUTTONDOWN
 WM_CAPTURECHANGED
 WM_MOUSEMOVE
 WM_SYSCOMMAND
 WM_GETMINMAXINFO
 WM_ENTERSIZEMOVE

Mouseup on resizer:
 WM_CAPTURECHANGED
 WM_WINDOWPOSCHANGING
 WM_GETMINMAXINFO
 WM_EXITSIZEMOVE

Note that no mouseup message is sent.

Adding a response to WM_EXITSIZEMOVE which does the same thing as the response to WM_LBUTTONUP 'fixes' this bug.

Note that this doesn't relate to the window titlebar. Anywhere in chrome is affected (as evidenced by theduplicate bug 633169)

I would assume that this bug is caused because the lack of a mouseup event doesn't reset this state in the presshell/eventstatemanager.

Something which resets these states should be happening as a response to the WM_EXITSIZEMOVE message, or probably more likely, the WM_CAPTURECHANGED message.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

How does this resizer work? (just want a quick idea on how it's positioned on top of content, and if it is a different window)
It seems it's not releasing the mouse capture correctly. The reason that the window drag itself doesn't work is because the titlebar isn't draggable during mouse capture, but this is more obvious now with the report in bug 633169.

Not quite the same but a similar condition we had to cover in the past was when the mouse moves between the client and non-client area. See http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5085

Comment 6

[Neil Deakin]

On Windows, the resizing is done entirely by the operating system. Comment 3 lists the messages that are sent to the window during this process.

Comment 7

[Neil Deakin]

Attachment: fix

This patch listens for the WM_CAPTURECHANGED message. To reduce the regression possibility, it only does anything when sInInMouseCapture is false, as is the case for this bug, since I worry that clearing it always may affect some behaviour that should occur on mouseup.

Comment 8

[Neil Deakin]

Comment on attachment 512786 [details] [diff] [review]
fix

Olli, is this a suitable use of ClearGlobalActiveContent?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 512786 [details] [diff] [review]
fix

Yes, if I understand WM_CAPTURECHANGED correctly.

Comment 10

[Jim Mathies [:jimm]]

We call SetCapture in widget on mouse down and mouse up in DispatchMouseEvent, so this is going to get called on every mouse click in content. Is that acceptable?

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#3970

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

In which order do we get the WM_* events?

Could we use WM_EXITSIZEMOVE and not WM_CAPTURECHANGED?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Hmm, though, there is the sIsInMouseCapture check. I assume that is set when
mouse down starts capturing and is unset when mouse up happens

Comment 13

[Neil Deakin]

WM_CAPTURECHANGED is only fired on mouseup. At the point, sIsInMouseCapture is true, so nothing different happens.

The only time I've found it sent on mousedown when sIsInMouseCapture is false is on the titlebar, although it occurs a few moments after the mousedown.

I could change the patch to use WM_EXITSIZEMOVE instead if desired.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Wouldn't WM_EXITSIZEMOVE be less regression-risky?
If so, better to use it.

Comment 15

[Neil Deakin]

Attachment: use WM_EXITSIZEMOVE instead

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get an assessment of risk vs. reward? Touching events is scary.

Comment 17

[Joe Drew (not getting mail)]

<jimm>	risk: seems low. it's isolated to when we have a mouse down on content and the window is being resized. reward: modest. it fixes a user interaction annoyance when you resize the window via the resizer on XP.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 512808 [details] [diff] [review]
use WM_EXITSIZEMOVE instead

Decided against the risk of touching events at this point in the game. Risk aversion won out, here.

Comment 19

[Jim Mathies [:jimm]]

We'll get this into the 5.0 (summer) release. People won't have to wait too long.

Comment 20

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/f9c3197391f0