Closed Bug 1203140 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(3 files)

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.
Doing this might also help reduce the performance impact of event regions.
Blocks: apz-talos
I have patches for this, but the test needs bug 1274284 fixed first.
Assignee: nobody → bugmail.mozilla
Depends on: 1274284
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).
Note that part 3 depends upon the patches in bug 1273654, although it can be rebased to not use those patches.
Attachment #8755092 - Flags: review?(bugs) → review+
Attachment #8755093 - Flags: review?(bugs) → review+
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.
Attachment #8755094 - Flags: review?(bugs) → review+
(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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :/
No worries, it wasn't very hard to do. I do look forward to landing those refactoring patches though :)
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
Attachment #8755092 - Flags: approval-mozilla-beta?
Comment on attachment 8755092 [details] [diff] [review]
Part 1 - Add a pref cache

Improve a feature, taking it.
Attachment #8755092 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8755093 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8755094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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 ?
Flags: needinfo?(bugmail.mozilla)
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.
Depends on: 1311505
Depends on: 1328065
You need to log in before you can comment on or make changes to this bug.