Ray Satiro Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141114030206

Steps to reproduce:

If I middle-click I can scroll up and down the page. However it happens sometimes I do that and didn't want to or I wanted to middle click a link or something. In that case I will middle-click again to shut it back off. In trunk this is not always possible. Try it. Middle-click somewhere, hardly move the mouse and then middle-click again. It doesn't shut it off. In 35.0a2 it still works properly.


Actual results:

Toggle is broken, if I middle click and the scroll was already on it doesn't always shut off.


Expected results:

If I middle-click when the scroll is on it should always shut it off, regardless of how close it is to the scroll circle widget.

	Loic
	Comment 1 • 10 years ago

With e10s enabled? Same behaviour with e10s disabled?

	Ray Satiro Reporter
	Comment 2 • 10 years ago

(In reply to Loic from comment #1)
> With e10s enabled? Same behaviour with e10s disabled?

It looks like it's only with e10s enabled. I just tested in a new non-e10s window and it's fine.

	John Pavlicek
	Comment 3 • 10 years ago

This appears to happen if the cursor is hovering over the scroll indicator when the middle mouse is clicked.  To replicate, try middle click to enable the scroll and, without moving the mouse, middle click again.  With e10s enabled, this does not dis-able scroll.  With e10s disabled, the scroll does get disabled.

	Camelia Badau [:cbadau], Desktop QA
	Comment 4 • 9 years ago

I can confirm this issue on the latest Nightly 38.0a1 (buildID: 20150119030222) on Windows 7 64bit, 32bit: "This appears to happen if the cursor is hovering over the scroll indicator when the middle mouse is clicked" - with e10s enabled.

	Mike Conley (:mconley) (:⚙️)
	Comment 5 • 9 years ago

Happy to help you investigate this Michael - let me know if you have any questions!

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 9 years ago

Michael, any update here?

	Tracy Walker [:tracy]
	Comment 7 • 9 years ago

Note: to reproduce, move the mouse pointer so it remains inside of the scroll icon.

	Neil Deakin Assignee
	Comment 9 • 9 years ago

I took a look at this briefly.

On non-e10s:

1. Middle-click happens.
2. The popup hides and calls stopScroll, sending the Autoscroll:Stop message to the child process.
3. The child process handles the middle-click event, sees that scrolling is enabled, and skips opening the popup.
4. The Autoscroll:Stop message is received to stop scrolling.

In e10s, steps 3 and 4 happen is reverse, that is, the child process receives the Autoscroll:Stop message before it receives the middle-click event.

	Neil Deakin Assignee
	Comment 10 • 9 years ago

It does work if I use a timeout to hide the popup, which, I think, will cause the mousedown event to be queued up to be sent to the child before the popuphidden event, and thus the Autoscroll:Stop message does. I'm not sure if that's guaranteed, but this fixes the bug on my machine.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 11 • 9 years ago

Comment on attachment 8638061 [details] [diff] [review]
Use a timeout

Review of attachment 8638061 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking this!

::: toolkit/content/widgets/browser.xml
@@ +1096,5 @@
>                case "mouseup":
>                case "mousedown":
>                case "contextmenu": {
> +                if (!this._ignoreMouseEvents) {
> +                  setTimeout(() => this._autoScrollPopup.hidePopup(), 0);

This looks okay to me. Is there any way we could use preventDefault or something to stop the middle click from going to the child process entirely?

	Neil Deakin Assignee
	Comment 12 • 9 years ago

I tried this with a preventDefault but that causes the test test_bug647770-2.html to fail. This is because when the middlemouse.paste preference is on, closing the popup currently causes the cursor to close and the paste to occur.

It probably shouldn't paste but contenteditable and middlemouse paste don't seem to integrate well anyway I filed bug 1188536 so that can be sorted out. For now, this patch gets us back to the old behaviour.

	:Felipe Gomes (needinfo for replies!)
	Comment 13 • 9 years ago

Comment on attachment 8640007 [details] [diff] [review]
mousescrollstop

Review of attachment 8640007 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe a better solution is to use stopCrossProcessForwarding() if _autoScrollPopup is open. But that function is noscript (I forgot the reason), so let's leave that for bug 1188536

	Jared Wein [:jaws] (please needinfo? me)
	Comment 14 • 9 years ago

(In reply to :Felipe Gomes from comment #13)
> that function is noscript (I forgot the
> reason)

http://logs.glob.uno/?c=mozilla%23e10s&s=25+May+2015&e=25+May+2015&h=stopCrossProcessForwarding#c40064

	Jared Wein [:jaws] (please needinfo? me)
	Comment 15 • 9 years ago

> 20:37 gw280 felipe: thanks for the review. would you prefer setting mFlags directly, or making the method chromeOnly?
> 20:37 gw280 I'd prefer the latter because I don't like the idea of mucking with mFlags directly in nsXULPopupManager
> 20:42 felipe  gw280: personally I'd just set mFlags directly to avoid adding a new function, but I guess either way is fine. Perhaps see what smaug prefers? He will need to do a final review on that anyway
> 20:43 smaug "would you prefer setting mFlags directly, or making the method chromeOnly" sounds odd
> 20:44 gw280 smaug: https://bugzilla.mozilla.org/show_bug.cgi?id=1119074#c13
> 20:45 smaug hmm, that is .idl
> 20:45 smaug we use .webidl to expose Event to the js
> 20:45 smaug gw280: and you can always use [noscript] in .idl
> 20:46 gw280 yeah I was just looking that up
> 20:46 gw280 it seems like noscript would be what I want?
> 20:46 smaug yeah, that sounds ok to me

	Neil Deakin Assignee
	Comment 16 • 9 years ago

> Maybe a better solution is to use stopCrossProcessForwarding() if
> _autoScrollPopup is open. But that function is noscript (I forgot the
> reason), so let's leave that for bug 1188536

I think that would have the same problem, since the test relies on the content process receiving a mousedown event.

	Neil Deakin Assignee
	Comment 17 • 9 years ago

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c911a3b09f6fc44cdf0690cc851d2638db2bb848
changeset:  c911a3b09f6fc44cdf0690cc851d2638db2bb848
user:       Neil Deakin <neil@mozilla.com>
date:       Wed Jul 29 06:30:06 2015 -0400
description:
Bug 1099491, e10s, middle click over autoscroll cursor doesn't close it, r=felipe

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c911a3b09f6f