1099491 - [e10s] middle-click scroll doesn't toggle properly

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ray Satiro]

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.

Comment 1

[Loic]

With e10s enabled? Same behaviour with e10s disabled?

Comment 2

[Ray Satiro]

(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.

Comment 3

[John Pavlicek]

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.

Comment 4

[Camelia Badau [:cbadau], Desktop Test Engineering]

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.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Michael, any update here?

Comment 7

[Tracy Walker [:tracy]]

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

Comment 9

[Neil Deakin]

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.

Comment 10

[Neil Deakin]

Attachment: Use a timeout

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.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 12

[Neil Deakin]

Attachment: mousescrollstop

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.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(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

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

> 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

Comment 16

[Neil Deakin]

> 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.

Comment 17

[Neil Deakin]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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