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)

x86
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: benjamin, Assigned: jaas)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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?
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.
Flags: blocking1.9? → blocking1.9+
I don't see this bug any more. Can someone else confirm that it is gone?
I'm not seeing this bug either, using today's nightly.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
It's back!
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Attached patch possible fix v1.0 (obsolete) (deleted) — Splinter Review
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...
Perhaps we could override sendEvent on all windows, and if there's a gRollupWidget then we direct events to that instead?
Target Milestone: --- → mozilla1.9 M10
Attached patch fix v1.0 (deleted) — Splinter Review
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)
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 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+
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 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+
landed on trunk with updated comment
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
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
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: