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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: raysatiro, Assigned: enndeakin, Mentored)

Tracking

36 Branch
Firefox 42
x86_64
Windows 7
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(e10sm8+, firefox38 affected, firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

Comment 1

5 years ago
With e10s enabled? Same behaviour with e10s disabled?
Flags: needinfo?(raysatiro)
Reporter

Comment 2

5 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.
Flags: needinfo?(raysatiro)

Updated

5 years ago
Blocks: e10s
Summary: middle-click scroll doesn't toggle properly → [e10s] middle-click scroll doesn't toggle properly

Updated

5 years ago
tracking-e10s: --- → ?

Comment 3

5 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.
Flags: firefox-backlog+
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
Assignee: nobody → mail
Happy to help you investigate this Michael - let me know if you have any questions!
Mentor: mconley
Michael, any update here?
Flags: needinfo?(mail)
Assignee: mail → wmccloskey
Note: to reproduce, move the mouse pointer so it remains inside of the scroll icon.
Assignee

Updated

4 years ago
Duplicate of this bug: 1180057

Updated

4 years ago
Depends on: 987121

Updated

4 years ago
Blocks: 987121
No longer depends on: 987121
Assignee

Comment 9

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

Comment 10

4 years ago
Posted patch Use a timeout (obsolete) — Splinter Review
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.
Flags: needinfo?(mail)
Attachment #8638061 - Flags: feedback?(wmccloskey)
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?
Attachment #8638061 - Flags: feedback?(wmccloskey) → feedback+
Assignee

Updated

4 years ago
Assignee: wmccloskey → enndeakin
Assignee

Comment 12

4 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.
Attachment #8638061 - Attachment is obsolete: true
Attachment #8640007 - Flags: review?(felipc)
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
Attachment #8640007 - Flags: review?(felipc) → review+
> 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
Assignee

Comment 16

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

Comment 17

4 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
https://hg.mozilla.org/mozilla-central/rev/c911a3b09f6f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee

Updated

4 years ago
Points: --- → 2
Assignee

Updated

4 years ago
Duplicate of this bug: 996829
You need to log in before you can comment on or make changes to this bug.