1013412 - Properly handle wheel events with APZ enabled on OS X

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

My patches in bug 1009679 turn off main thread wheel event handling completely as soon as APZ is enabled. That's obviously not a solution.

What we want is:
 - When we receive a wheel event on the APZ thread, and there's no DOM wheel event listener that would fire for this event, start scrolling on the APZ thread immediately.
 - If there is a wheel listener, pass the event to the main thread and go through all the usual event handling. This will notify the listeners, allow them to preventDefault() the event, and do scrolling on the main thread in the default handling.

So the APZ thread needs to know which parts have potential DOM wheel event listeners. There must already be code that handles this problem on B2G with touch events, and we'll probably need to do something very similar here, I just haven't looked at that code yet.

We also need to figure out what to do when scrolling over a plugin - the plugin may want a first look at the wheel event, too.

And there's the question of how passing the event to the main thread should happen (and where) - for example, I don't know if we can freely pass cocoa NSEvent objects between threads.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Kartikaya Gupta (email:[e-mail])]

So the testcase mostly works, but because of the way wheel transactions work, if you are panning with the trackpad and your mouse pointer moves into the dark circle, it will continue panning while the ball goes around. If you wait a second for the transaction to end then the page doesn't move any more while the ball goes around. Without APZ the page stops panning immediately as soon as the cursor is on top of the gray circle. Leaving this open for now.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This fixes the problem; I'll see if I can write a test for it. dvander, do you think this will break anything?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8599331 [details] [diff] [review]
Patch

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

It's worth noting: we'll still behave incorrectly, if for whatever reason, the first event in the transaction has preventDefault=false but the second has preventDefault=true. I'm hoping we can get away with that though, I like this approach since it means the pessimistic case will still behave as it did before.

Markus and I had some other ideas on how to fix this, like lowering the transaction time and/or factoring in cursor move distance. But this seems a lot simpler.

Comment 5

[Markus Stange [:mstange]]

Now this is an interesting fix! I discussed this problem with dvander while he was in Toronto, but yours was not among the solutions we came up with. I like it.

One thing we discussed was that non-APZ scrolling has the property that a wheel event that's handled by content and preventDefault()ed is never also used for scrolling. This property is hard to guarantee completely because we'd have to wait for content to handle each individual wheel event, and we don't want to do that.
However, maybe we can guarantee a simpler property. If a webpage installs a wheel event handler on an element, that handler is usually not going to alternate randomly between preventDefault()ing and not preventDefault()ing the event - either it preventDefault()s all of them or none of them.
So instead we can only consider pages for which every DOM element falls into one of two cases:
 - either it has a wheel event listener that preventDefault()s all events
 - or it doesn't.
Then we can guarantee the property above by telling the EventStateManager to send all wheel events of a transaction to the same DOM element - the one that received the event that started the transaction. Then, if there was no preventDefault()ing handler on the first event, all events of the transaction will scroll the page and none will be preventDefaulte()ed. In the testcase attached here, it means that if you start scrolling outside the big dark gray circle, no event of the wheel transaction will be handled by content, so we don't have any double-handled events.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Anderson [:dvander] from comment #4)
> It's worth noting: we'll still behave incorrectly, if for whatever reason,
> the first event in the transaction has preventDefault=false but the second
> has preventDefault=true. I'm hoping we can get away with that though

Yeah, my patch isn't perfect in that whether or not an event gets double-handled is actually nondeterministic in the general case (although deterministic if the controller thread is the main thread).

Say you get the following series of wheel events:

event 1
event 2
event 3 (preventDefault = true)
event 4 (preventDefault = true)

Since 1 is not preventDefaulted, APZ will start handling this block of events, without waiting for the content response on events 2 onwards. If the controller thread is the main thread, then once event 3 has gone through content and sent the default-prevented notification via [1], APZ will terminate the block and start a new block from event 4 onwards. That block will have a preventDefaulted=true on the first event and so APZ won't handle it. In this scenario only event 3 gets double-handled.

However if the controller thread is not the same as the main thread, then the controller thread may handle events 3 and 4 (and more events) before the main-thread gets around to notifying APZ of the prevent-default on event 3. Therefore we might have a bunch of events that get double-handled.

(In reply to Markus Stange [:mstange] from comment #5)
> Then we can guarantee the property above by telling the EventStateManager to
> send all wheel events of a transaction to the same DOM element - the one
> that received the event that started the transaction.

This does help, but it still falls short of a "perfect" solution because of the assumption it makes (that a listener on a given element will either preventDefault all events or none of them). So even with this, you can't guarantee events won't be double-handled. And changing ESM to send all wheel events of a transaction to the same DOM element may break other things. So I'm not really sure how I feel about this - seems like the benefit may not be worth the breakage.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=dd54590ebfbd#1037

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test

Here's a test, it's basically markus' testcase turned into a mochitest. It fails when I run it without the patch and passes with the patch, using |mach mochitest --e10s --setpref layers.async-pan-zoom.enabled=true dom/events/test/test_bug1013412.html|. It also passes with and without the patch if I don't set APZ to true, as expected. Please review carefully as I haven't written this kind of a test before and I might have fall into all sorts of pitfalls, particularly if I'm supposed to be waiting for paint somewhere and I'm not.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8600063 [details] [diff] [review]
Test

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

::: dom/events/test/test_bug1013412.html
@@ +80,5 @@
> +function runTest() {
> +  var content = document.getElementById('content');
> +  if (iteration < 300) { // enough iterations that we would scroll to the bottom of 'content'
> +    iteration++;
> +    synthesizeWheel(content, 100, 10,

I can't tell whether this should be sendWheelAndPaint or not, but since the iteration count is so high it seems fine as-is.

@@ +92,5 @@
> +  is(rotationAdjusted, true, "The rotation should have been adjusted");
> +  SimpleTest.finish();
> +}
> +
> +window.onload = function() {

All of the wheel tests do:

  SimpleTest.waitForFocus(runTest, window);

Though I don't know if it's necessary.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Anderson [:dvander] from comment #8)
> 
> I can't tell whether this should be sendWheelAndPaint or not, but since the
> iteration count is so high it seems fine as-is.

If I try that I get a |TypeError: aWindow.waitForAllPaintsFlushed is not a function| error. I'll leave it as-is and do a try push to see how it fares.

> All of the wheel tests do:
> 
>   SimpleTest.waitForFocus(runTest, window);
> 
> Though I don't know if it's necessary.

I'll update to do this as well, can't hurt.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

A try push [1] showed that the test passes everywhere except B2G, so I disabled it on b2g and landed. (All the other wheel event tests are also disabled on b2g for the same reason).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbfa5f340d13

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b663a0bfe8b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0b57ab2eca

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b663a0bfe8b0
https://hg.mozilla.org/mozilla-central/rev/5c0b57ab2eca