Closed
Bug 385034
Opened 18 years ago
Closed 17 years ago
Autoscroll doesn't cancel properly on second middle-click
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9beta2
People
(Reporter: benjamin, Assigned: jaas)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
smichaud
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
Steps to reproduce:
* Open a webpage that needs scrolling
* Middle-click to start autoscroll
* Middle-click again
Expected results:
The second middle-click should cancel autoscroll
Actual results:
The second middle-click moves the autoscroll marker but continues autoscroll mode.
I think this is a fairly recent regression because I tend to use autoscroll a lot and I've been using trunk nightlies.
Flags: blocking1.9?
Comment 1•18 years ago
|
||
The patch for bug 242621 regressed this. See also bug 384097 for another Cocoa-specific autoscroll problem.
Blocks: 242621
Keywords: regression
I don't use Macs, so I can't debug or fix this. I would assume the Mac is incorrectly targeting the events though (e.g. going to the popup when they should go to the page, or vice versa).
(In reply to comment #1)
> The patch for bug 242621 regressed this. See also bug 384097 for another
> Cocoa-specific autoscroll problem.
>
That bug is pre-new-autoscroll.
I don't see this bug any more. Can someone else confirm that it is gone?
Comment 4•17 years ago
|
||
I'm not seeing this bug either, using today's nightly.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
Updated•17 years ago
|
Flags: in-litmus?
My guess is that the autoscroll marker is its own widget or window and we're not routing the second middle click event properly.
The autoscroll marker is its own popup window and when we middle-click outside of the autoscroll marker's window the event is going to the window the click is over, not the popup window like it should. That is why this bug happens.
This is one way to fix it, but it might not be the right way. Here we stop handling otherMouseDown: if we roll up a popup, just like we do in mouseDown:. That rolls up the popup window for autoscroll and stops us from opening another one.
Either we have to fix the bug this way, or we have to target native events at popup windows even if they don't happen over a popup window. That seems more correct to me, but I have to sort out how that would work...
Comment 9•17 years ago
|
||
Perhaps we could override sendEvent on all windows, and if there's a gRollupWidget then we direct events to that instead?
Assignee | ||
Comment 10•17 years ago
|
||
In order to fix this correctly I read over the Windows code and our own code, wrote up pseudocode for correct behavior, and made sure all of our mouse down methods implement it correctly.
In addition to ensuring the correct logic in our mouse down methods, I reorganized and renamed some things for clarity.
Attachment #284440 -
Attachment is obsolete: true
Attachment #286855 -
Flags: review?(smichaud)
Comment 11•17 years ago
|
||
Josh, I've done a fair amount of testing without finding any problems.
But I still need to give it more thought ... I need to convince myself
that I understand _why_ I haven't seen any problems :-)
And now Damon's asked me to look for a patch for bug 400082 as quickly
as I can manage. So I'll put this off ... probably for the next 2-3
days.
Comment 12•17 years ago
|
||
Comment on attachment 286855 [details] [diff] [review]
fix v1.0
Josh, I have no problems with your patch except that you dropped one
of my comments above [ChildView ensureCorrectMouseEventTarget:] (what
used to be [ChildView maybeRerouteMouseEventToRollupWidget:). I think
what the comment says is useful and not obvious, so it should be
restored. So I'd rewrite your second paragraph above [ChildView
ensureCorrectMouseEventTarget:] as follows:
// This is needed when the user tries to navigate a context menu while keeping
// the mouse-button down (left or right mouse button) -- the OS thinks this is
// a dragging operation, so it sends events (mouseMoved and mouseUp) to the
// window where the dragging operation started (the parent of the context
// menu window). It also works around a bizarre Apple bug - if (while a context
// menu is open) you move the mouse over another app's window and then back over
// the context menu, mouseMoved events will be sent to the window underneath the
// context menu.
When you first told me about this patch (on IRC), I had trouble with
using [ChildView ensureCorrectMouseEventTarget:] to reroute mouseDown
events (left, right or other) to the current rollup widget -- I
thought this might result in the keyboard focus not being changed when
it should be changed (since, by default, the OS always gives the
keyboard focus to the object you've just clicked the mouse on).
Now I realize that other code in my popup patch (in [PopupWindow
sendEvent:]) was already doing exactly what I was worried about -- all
along it's ensured that all mouse events (including mouseDown events)
that happen over a rollup widget get passed to that rollup widget.
This doesn't (and wasn't) causing trouble. So it seems that Gecko was
(and is) doing the right thing with these mouseDown events, and that I
needn't have worried.
This also means that the parts of your patch that (optionally)
re-route mouseDown events (the calls to [ChildView
ensureCorrectMouseEventTarget:] in [ChildView mouseDown:], [ChildView
rightMouseDown:] and [ChildView otherMouseDown:]) are superfluous.
But I think you should leave them in, because they make it easier to
understand what the code actually does with mouseDown events.
Attachment #286855 -
Flags: review?(smichaud) → review+
Comment 13•17 years ago
|
||
Here's my revised comment again, without the silly line breaks:
// This is needed when the user tries to navigate a context menu while keeping
// the mouse-button down (left or right mouse button) -- the OS thinks this is
// a dragging operation, so it sends events (mouseMoved and mouseUp) to the
// window where the dragging operation started (the parent of the context
// menu window). It also works around a bizarre Apple bug - if (while a context
// menu is open) you move the mouse over another app's window and then back over
// the context menu, mouseMoved events will be sent to the window underneath the
// context menu.
Comment 14•17 years ago
|
||
Sigh :-(
Assignee | ||
Comment 15•17 years ago
|
||
Comment on attachment 286855 [details] [diff] [review]
fix v1.0
Thanks Steven. Good to have that clarified. I will update the comment on checkin.
Attachment #286855 -
Flags: superreview?(roc)
Attachment #286855 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 16•17 years ago
|
||
landed on trunk with updated comment
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 17•17 years ago
|
||
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007113004 Minefield/3.0b2pre following the STR in Comment 0.
Status: RESOLVED → VERIFIED
Comment 18•17 years ago
|
||
https://litmus.mozilla.org/show_test.cgi?id=5876 added to Litmus.
Flags: in-litmus? → in-litmus+
You need to log in
before you can comment on or make changes to this bug.
Description
•