Closed Bug 1099491 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

36 Branch
x86_64
Windows 7
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
e10s m8+ ---
firefox38 --- affected
firefox42 --- fixed

People

(Reporter: raysatiro, Assigned: enndeakin, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
With e10s enabled? Same behaviour with e10s disabled?
Flags: needinfo?(raysatiro)
(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)
Blocks: e10s
Summary: middle-click scroll doesn't toggle properly → [e10s] middle-click scroll doesn't toggle properly
tracking-e10s: --- → ?
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.
Depends on: 987121
Blocks: 987121
No longer depends on: 987121
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.
Attached 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: wmccloskey → enndeakin
Attached patch mousescrollstopSplinter Review
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
> 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.
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Points: --- → 2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: