1144324 - Do a better job of registering for touch events in the windows widget code

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

So right now we have the funky code at [1] which causes the widget to register as a "touch window" when there are touch listeners attached and unregister when there are no touch listeners. This is was supposedly a half-decent solution for the state of things back in the day, but it was a very "magical" behavior which has led to much angst where scrolling was broken on pages with touch listeners (see bug 736048). This behaviour was eventually disabled by setting the dom.w3c_touch_events.enabled pref to 0 on all desktop firefoxen.

I think this code has outlived its usefulness now that we have APZ, and should just be killed. APZ isn't yet enabled by default on desktop, so we still need to keep the existing touch-based scrolling code in in widget/windows. However, we can do a better job of picking between that and APZ on startup and then just sticking with one scrolling implementation. I have patches that do this and they seem to work as expected.

Comment 1

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

Attachment: Part 1 - Try to register as a touch window if APZ is enabled

This makes it so that if the window gets an APZCTreeManager (i.e. we have layers.async-pan-zoom.enabled set to true and the window is a top-level or child window), then we automatically try to register as a touch window. The registration itself further checks for the dom.w3c_touch_events.enabled and dom.w3c_pointer_events.enabled prefs, and only succeeds if one or both of those is true. Therefore, setting both layers.async-pan-zoom.enabled and dom.w3c_touch_events.enabled will switch desktop windows to scroll using APZ with touch events.

Comment 2

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

Attachment: Part 2 - Remove all unneeded machinery

And this removes all the machinery that we don't need any more to track if have touch listeners or not.

Comment 3

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

In my local testing I was working on top of the patches in these two bugs. There shouldn't be an actual conceptual dependency but I didn't test these patches in isolation.

Comment 4

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

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

Comment 5

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

Comment on attachment 8578866 [details] [diff] [review]
Part 2 - Remove all unneeded machinery

Update IID of nsPIDOMWindow.

I thought the original reason for this code was touch vs. gesture events case on
Windows 7. But if we can remove this, fine.

Comment 6

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

That try push got screwed up by infra melting. Did another push:

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

Comment 7

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d3dadcb10226
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/33c30e283fa8

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/d3dadcb10226
https://hg.mozilla.org/mozilla-central/rev/33c30e283fa8

Comment 9

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

Note that the second patch here broke some stuff on Fennec, so I backed out part of it to fix bug 1146843. We still need to track the touch listeners (although we don't need the UpdateTouchWindow goop), until Fennec switches over to the C++ APZ code. Then we can rip it out again.