938359 - [e10s] Support middle-click scroll

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

In desktop Firefox, I frequently middle-click on a page to enter a "scrolling mode" where I can scroll by simply moving the mouse up or down. The cursor also changes to reflect the fact that I have entered this mode.

With e10s enabled, that feature appears not to work.

Comment 1

[(no longer active)]

This is where the autoscroll action is initiated: <http://hg.mozilla.org/mozilla-central/annotate/b53589696cf8/toolkit/content/widgets/browser.xml#l1254>.  I guess the reason this doesn't work is that the code tries to access event.originalTarget which lives in the content process.

Can anyone provide pointers to how we usually change code like this to make it e10s compatible?

Comment 2

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

I seem to be making progress on this, so I'll take it.

Comment 3

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

Attachment: autoscroll

I just split this into a content part and a chrome part and used the message manager. I was a little wary of using mozRequestAnimationFrame in a content script, but I talked to smaug and it sounds like it's fine.

Also, the reason I created browser-content.js (rather than reusing browser-child.js) is that we're using this code regardless of whether it's a remote browser or not.

Comment 4

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

Attachment: autoscroll-test-fixes

I had to update the tests a little to account for the time it takes for messages to arrive.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8381742 [details] [diff] [review]
autoscroll

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

::: toolkit/content/widgets/browser.xml
@@ +773,5 @@
>            this.addEventListener("pagehide", this.onPageHide, true);
>            this.addEventListener("DOMPopupBlocked", this.onPopupBlocked, true);
> +
> +          // We can't use receiveMessage here because it's defined by remote-browser.xml
> +          // and we don't want them to conflict.

I was about to say "let's use receiveMessage" when I saw this comment..

hmm I think it would still be better to work around this in some way instead of having these functions spread around the code. I've seen it done somewhere where the parent defined a function with a different name (like _receiveMessage) so that the child binding could call it.  It's a bit ugly, but what do think?

Something like:

browser.xml:

  receiveMessage: function(...args) { this._receiveMessage(...args); }


remote-browser.xml:

  receiveMessage: function(..) {
    switch (message.name) {
      case 'a':
        ...

      default:
        // see if the parent binding whats to handle it
        this._receiveMessage(..);
    }
  }

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8384062 [details] [diff] [review]
autoscroll-test-fixes

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

::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js
@@ +77,5 @@
>           test.elem+' should'+(scrollHori ? '' : ' not')+' have scrolled horizontally');
> +
> +      // Before continuing the test, we need to ensure that the IPC
> +      // message that stops autoscrolling has had time to arrive.
> +      executeSoon(nextTest);

do you think one tick is enough to guarantee it?

Comment 8

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

(In reply to :Felipe Gomes from comment #7)
> do you think one tick is enough to guarantee it?

I was thinking about this myself. I think I'll add a function called executeAfterIPCRoundtrip that sends a message to the child, waits for a response, and then runs a callback. I suspect that will be useful in a lot of tests. I remember having to do similar stuff in the about:home tests, and it felt kind of gross.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/77fdb4380b1a

I filed bug 983430 for executeAfterIPCRoundtrip.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/32b52e414a28 for metro-chrome bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=36102341&tree=Mozilla-Inbound

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc298e4b0f47

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

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

I backed this out in aurora since it's had two regressions so far.
https://hg.mozilla.org/releases/mozilla-aurora/rev/83f1fa9a7ca9