407876 - Scrolling stops when moving the mouse out of the window while dragging the scrollbar thumb

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Peter Van der Beken [:peterv]]

On a page that has a scrollbar, grab the scollbar thumb and drag it to scroll. Move the mouse out of the window while dragging, scrolling will stop. If you release the thumb while the mouse is outside of the window and the mouse is over a different window then focus will switch to that window. If you release the thumb while the mouse is outside of the window and the mouse is not over any other window then the window will keep scrolling once you move the mouse inside the window again, even though you're not dragging anymore.

Not sure if this is Mac-specific, please reassign if it's not.

Comment 1

[Josh Aas]

Attachment: fix v1.0

This seems like the right thing to do. At the point in the code in question, dragging events are basically equivalent to moving events (for the purpose of event routing).

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 292616 [details] [diff] [review]
fix v1.0

Josh, I've found two problems with your patch -- it causes a bad
regression, and doesn't fully fix the problem reported in this bug.
And there's a third problem, reported by you (channeling roc) on IRC
last week, which is fallout from your patch from bug 406362, and which
turns out still to happen in today's nightly.  (I'll describe all
three problems below.)

I haven't been able to find any simple fix that addresses all these
problems ... aside from always returning YES from
ensureCorrectMouseEventTarget if there's no rollup widget.

I'm beginning to think it's not a good idea for us to do our own mouse
event routing (in ensureCorrectMouseEventTarget) except in the special
case that a rollup widget exists.  There are too many complications,
and if there's no rollup widget the events seem to always already have
their correct target (to have been sent to the correct NSWindow
object).

First the two problems with your patch (attachment 292616 [details] [diff] [review]):

1) Open a window with a vertical scrollbar, and start using the
   scrollbar thumb to scroll it.
2) With the mouse button still down, drag the mouse pointer outside of
   the window (say over the desktop) and let go the mouse button.
3) Move the mouse cursor back over the window, without clicking in it
   -- you'll see that the window scrolls as you move the mouse.

1) Open two browser windows, at least one of which has a vertical
   scrollbar.
2) Start using the scrollbar thumb in one of the windows to scroll it.
3) With the mouse button still down, drag the mouse pointer outside of
   the first window but over the second window.
4) The first window won't scroll as you drag the mouse over the second
   window -- which is a variant of this bug as originally reported.

Now the third problem (fallout from bug 406362 that hasn't yet been
fixed in today's nightly 2007-12-11-04-trunk):

1) Open two browser windows.
2) Click and hold the mouse button somewhere in the first window, then
   drag it over the second window.
3) Release the mouse button over the second window -- the second
   window now comes forward and becomes active.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> I haven't been able to find any simple fix that addresses all these
> problems ... aside from always returning YES from
> ensureCorrectMouseEventTarget if there's no rollup widget.

I don't mean to imply that the correct solution is just to return YES
from ensureCorrectMouseEventTarget when there's no rollup widget,
without making other changes to that method.

But I do suspect that it won't be necessary to make changes outside of
ensureCorrectMouseEventTarget.

Comment 4

[Josh Aas]

Attachment: fix v2.0

This is a much more comprehensive fix. I spent a lot of time unwinding our behavior and fixing things based on what we've learned over the past few weeks. It also fixes bug 396952 and simplifies a lot of our code. I tested this with the following:

- repro steps in the summary of this bug
- steps for bugs that Steven previously outlined in this bug
- steps for every bug we've fixed in this code recently to make sure it doesn't regress anything (407310, 406362, 402926, 385034)
- steps for 396952
- lots of my own various tests

I think we have some more work to do in the case where we raise context menus while our app is inactive, but this doesn't regress that behavior afaict.

Some notes about bigger changes in this patch...

It turns out we don't need to synthesize mouse down events when we ensure correct routing any more. Either something changed or we mistook an event routing problem that has since been fixed for a problem in Gecko handling that doesn't really exist. I was thinking about bug 396952 and why we had to do the hack with synthesized mouse down events in the first place. That lead to me reading the nsMenuFrame code and realizing that we shouldn't need that hack at all.

Another significant change (bug fix) is that in our sendEvent: override in NSWindow, we need to handle the case where the hit test for the window doesn't return a view because the mouse isn't over the window. We can't just call the parent's sendEvent: there because that will just kill the event for the same reason we have the override in the first place, which is that the parent impl doesn't forward them. Instead we should target such events at the first responder. That fixed scrolling in popup windows when the mouse is dragged from the scrollbar off of the window (now scrolling continues to work).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Josh, almost there ... but still not quite :-(

Everything in your patch looks reasonable, but I found another
problem.  Since it doesn't occur in yesterday's nightly, it's
presumably caused by your patch (though at the moment I have no idea
how):

1) Open one of the Places menus (I used "Smart Bookmarks"), and stop
   moving the mouse long enough for a submenu to open.
2) From now on (until you click somewhere), you'll only be able to
   browse that submenu.  Moving the mouse over higher-level menu items
   won't close that submenu, or open other submenus.

Comment 6

[Josh Aas]

Attachment: fix v2.1

Fixes a couple more issues, including the 10.4 issue with v2.0 reported by Steven. This works for all my test cases on 10.4 and 10.5.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 292917 [details] [diff] [review]
fix v2.1

This patch looks fine, and I didn't find any problems testing it.

I tested in Minefield and Camino on Tiger and Leopard.  I played with
places and context menus, and looked for all the problems reported
here.  For good measure I also tested with the testcase for an obscure
bug (bug 392389) that involves a menu popped up from a popup window --
that bug stayed fixed.

Comment 9

[Josh Aas]

landed on trunk

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.