1203140 - touchstart / touchmove handlers delay wheel scrolling (for example on imgur)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

At the moment, nsLayoutUtils::HasApzAwareListeners just lumps all kinds of event handlers into one, and makes APZ wait for a content response if there's a listener for any of those event types, regardless of what kind of event is being processed by APZ.

There are sites (e.g. imgur) which have touch event listeners but no wheel event listeners. Those sites shouldn't be allowed to slow down wheel event processing.

I think there are two options here:
 - Either create a separate dispatch-to-content region per event type, or
 - check which event types are supported by APZ on the current platform, and don't
   add listeners for unsupported events to the dispatch-to-content region.

The latter options sounds more attractive, until we start supporting both wheel and touch events on the same platform.

Comment 1

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

Doing this might also help reduce the performance impact of event regions.

Comment 2

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

Not important enough to uplift.

Comment 3

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

I have patches for this, but the test needs bug 1274284 fixed first.

Comment 4

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

Try push with bug 1273654 + bug 1274284 + this bug, for verification:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aca30e71385

Comment 5

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

I canceled the above try push because it had some bad patches from bug 1274284 included. Since then I rewrote the test to not require bug 1274284. New try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=211c68f276dc&group_state=expanded confirms it's good (the many starred windows failures are a pre-existing issue).

Comment 6

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

Attachment: Part 1 - Add a pref cache

Comment 7

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

Attachment: Part 2 - APZ shouldn't wait for touch listeners if touch is not enabled

Comment 8

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

Attachment: Part 3 - Test

Comment 9

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

Note that part 3 depends upon the patches in bug 1273654, although it can be rebased to not use those patches.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8755094 [details] [diff] [review]
Part 3 - Test

Don't you need some todo() or ok() in case isApzEnabled() returns false so that the test doesn't complain nothing was tested.

Comment 11

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

(In reply to Olli Pettay [:smaug] from comment #10)
> Don't you need some todo() or ok() in case isApzEnabled() returns false so
> that the test doesn't complain nothing was tested.

The isApzEnabled() function implementation does that internally. I figured it was nicer to do it that way so I didn't have to do it at every use-site.

Comment 12

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

I rebased the test to not depend on bug 1273654, so I can get it landed. I'll update it as part of the refactorings in bug 1273654 eventually.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ea3b7f6bac
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1effae7146
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d77ceeacfa6

Comment 14

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> I rebased the test to not depend on bug 1273654, so I can get it landed.
> I'll update it as part of the refactorings in bug 1273654 eventually.

Sorry for being away for so long and causing you to do things like this :/

Comment 15

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

No worries, it wasn't very hard to do. I do look forward to landing those refactoring patches though :)

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4ea3b7f6bac
https://hg.mozilla.org/mozilla-central/rev/1c1effae7146
https://hg.mozilla.org/mozilla-central/rev/7d77ceeacfa6

Comment 17

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

Comment on attachment 8755092 [details] [diff] [review]
Part 1 - Add a pref cache

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: Pages with touch listeners can slow down wheel scrolling even on devices where touch events aren't supported
[Describe test coverage new/current, TreeHerder]: patches include a test
[Risks and why]: pretty low risk, code has been on m-c for a bit and it's a fairly simple optimization
[String/UUID change made/needed]: none

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8755092 [details] [diff] [review]
Part 1 - Add a pref cache

Improve a feature, taking it.

Comment 19

[Carsten Book [:Tomcat]]

part 2 has problems to apply to beta :

grafting 346425:1c1effae7146 "Bug 1203140 - Don't add touch listener areas to dispatch-to-content regions unless touch events are enabled. r=smaug"
merging dom/events/EventListenerManager.cpp
warning: conflicts while merging dom/events/EventListenerManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

can you take a look ?

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7366f3c669b3

Comment 21

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

Parts 2 and 3:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4cec5ef7ea55
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2c5721ac42d7

Comment 22

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

https://hg.mozilla.org/releases/mozilla-beta/rev/8a2842b342b5

I disabled the test on beta because it was permafailing. It requires passive event listener support to pass, and passive event listener support is only in 49 and up. The actual patches in this bug don't need passive event listener support so they are find to stay in 48.