Closed Bug 632953 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: asa, Assigned: enndeakin)

References

Details

(Keywords: dogfood, regression)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 489303
OS: Windows 7 → Windows XP
cc'ing Jimm, overall this sounds like it should be a blocker, having to move focus to the window frame is a significant regression.
(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.
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.
Component: General → Widget: Win32
Product: Firefox → Core
QA Contact: general → win32
Summary: using the custom resizer on XP breaks titlebar drag and drop a little → After using the resizer on XP, the next mouse click on a chrome area is ignored
Duplicate of this bug: 633169
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
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.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Assignee: nobody → enndeakin
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #512786 - Flags: review?(jmathies)
Comment on attachment 512786 [details] [diff] [review]
fix

Olli, is this a suitable use of ClearGlobalActiveContent?
Attachment #512786 - Flags: review?(Olli.Pettay)
Comment on attachment 512786 [details] [diff] [review]
fix

Yes, if I understand WM_CAPTURECHANGED correctly.
Attachment #512786 - Flags: review?(Olli.Pettay) → review+
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
In which order do we get the WM_* events?

Could we use WM_EXITSIZEMOVE and not WM_CAPTURECHANGED?
Hmm, though, there is the sIsInMouseCapture check. I assume that is set when
mouse down starts capturing and is unset when mouse up happens
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.
Wouldn't WM_EXITSIZEMOVE be less regression-risky?
If so, better to use it.
Attachment #512786 - Attachment is obsolete: true
Attachment #512808 - Flags: review?(jmathies)
Attachment #512786 - Flags: review?(jmathies)
Attachment #512808 - Flags: review?(jmathies) → review+
Attachment #512808 - Flags: approval2.0?
Can we get an assessment of risk vs. reward? Touching events is scary.
<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 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.
Attachment #512808 - Flags: approval2.0? → approval2.0-
We'll get this into the 5.0 (summer) release. People won't have to wait too long.
http://hg.mozilla.org/mozilla-central/rev/f9c3197391f0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker]
You need to log in before you can comment on or make changes to this bug.