1661108 - MLS map continues to zoom after touchpad scroll gesture stops with allow_zooming on Windows

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Chris Peterson [:cpeterson]]

Steps to reproduce

1. In Firefox Nightly on Windows, open MLS map page: https://location.services.mozilla.com/map
2. Use the laptop touchpad scroll gesture to zoom in and out of the MLS map.
3. Lift your fingers from the laptop touchpad to stop zooming.

Expected results

The map should stop zooming.

Actual results

The map will zoom in and out, but when you lift your fingers from the laptop touchpad, the map will continue zooming in or out more more zoom level. I can reproduce this on my Windows 10 laptop, but not on my MacBook Pro.

This is a regression from allow_zooming bug 1620055, which is restricted to the Nightly channel. I bisected this bug to the following pushlog:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a518405abe4c93e6d8df622f59d5f01c4766e1d5&tochange=a09e4ce9ebba8fa4a29a4833560c8ee807018c03

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: wev2.html

Comment 2

[Timothy Nikkel (:tnikkel)]

apz.allow_zooming also enables direct manipulation. This means we can get pixel level scrolls out of those two finger scroll gestures on the touchpad instead of just line level events. direct manipulation automatically also gives us inertia for these gestures, in gecko we call it momentum. Either way it is events that happen after the user lifts their fingers. Without dmanip, the line level events we got either didn't give us momentum or the momentum was smaller.

On mac we also get momentum events, and I feel like I can reproduce the bug on mac as well, but I do not claim to be a very sensitive observer of this effect in general.

Using the testcase wev2.html that I uploaded it's a little easier to see what is going on instead of having to look at discrete jumps of zoom in a map that is also likely animated by the page (so the zooms are going to lag behind input almost always). Open it and do pan gestures over the yellow box while you have the console open. It will show the events it gets, the delta for each event, the accumulated deltaY for all events since the last time the clear button was pressed, and the delta mode for each event (0 == pixel, 1 == line). Playing around with this you can get a feel for how much momentum there is.

Using the testcase my subjective observation was that on Windows without dmanip we had very little to no momemtum, with dmanip we had quite a bit of momentum, and on mac we have some momentum but less than with dmanip on Windows.

I'm not exactly sure what, if anything we want to do here. The only thing that is initially coming to mind is to not generate WidgetWheelEvent's for pan gestures that are momentum, but I'm not even sure if that is a good idea in general.

Comment 3

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

How does Chrome behave in this scenario? If they have the same issue then I would say this is non-blocking.

Comment 4

[Timothy Nikkel (:tnikkel)]

Good call.

Chrome seems the same as Firefox (with zooming = true) to me. I tested on both mac and Windows, although the amount of momentum differs by platform, on each platform Firefox (with zooming = true) and Chrome feel like they match to me.

Comment 5

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

Ok. I'll mark it as a post-release bug. I think it would be nice to fix if we can. One approach might be to see if the non-momentum wheel events are getting canceled by content, and if they are, then don't send the momentum-based wheel events at all.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)

How does Chrome behave in this scenario? If they have the same issue then I would say this is non-blocking.

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

Chrome seems the same as Firefox (with zooming = true) to me. I tested on both mac and Windows, although the amount of momentum differs by platform, on each platform Firefox (with zooming = true) and Chrome feel like they match to me.

FWIW, I can't reproduce this zooming behavior on Chrome on my Windows laptop. I only see the problem in Firefox (with allow_zooming = true).

Comment 7

[Timothy Nikkel (:tnikkel)]

Could you try comparing scrolling over the yellow div in wev2.html attachment here with the console visible in both Chrome and Firefox to see if you get about the same amount of momentum wheel events?

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Could you try comparing scrolling over the yellow div in wev2.html attachment here with the console visible in both Chrome and Firefox to see if you get about the same amount of momentum wheel events?

It's really hard to compare because I can't really tell if I'm scrolling a similar distance on the touchpad in both browsers, but it looks like Firefox is generating about the same or maybe slightly more wheel events.

One interesting difference is that Chrome always rounds event.deltaX/deltaY/deltaZ to a multiple of 0.5:

wheel handler called 0 32.5 0 tot 2332.5
deltamode 0
wheel handler called 0 40 0 tot 2405
deltamode 0
wheel handler called 0 2.5 0 tot 1222.5
deltamode 0
wheel handler called 0 2.5 0 tot 1225
deltamode 0
wheel handler called 0 0 0 tot 1225

whereas Firefox 82 Nightly returns delta values with many decimal digits:

wheel handler called 0 2.01607666015625 0 tot 1497.3048095703123
deltamode 0
wheel handler called 0 0.516064453125 0 tot 1497.8208740234372
deltamode 0
wheel handler called 0 1.30439453125 0 tot 1499.1252685546872
deltamode 0
wheel handler called 0 -0.0096435546875 0 tot 1499.1156249999997

But if I set apz.allow_zooming = 0, then Firefox rounds the delta values to a multiple of 0.025:

wheel handler called 0 -0.475 0 tot 20.349999999999998
deltamode 1
wheel handler called 0 -0.625 0 tot 54.550000000000004
deltamode 1
wheel handler called 0 -28.05 0 tot 26.500000000000004
deltamode 1
wheel handler called 0 -1.6 0 tot 24.900000000000002
deltamode 1
wheel handler called 0 -0.775 0 tot 24.125000000000004

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Peterson [:cpeterson] from comment #8)

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Could you try comparing scrolling over the yellow div in wev2.html attachment here with the console visible in both Chrome and Firefox to see if you get about the same amount of momentum wheel events?

It's really hard to compare because I can't really tell if I'm scrolling a similar distance on the touchpad in both browsers, but it looks like Firefox is generating about the same or maybe slightly more wheel events.

I mainly just look at how long after my touch do events keep coming in.

The total numbers seem about the same to me.

I don't claim to be the best observer of this. Maybe a third person needs to try and give their observation.

Comment 10

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

I tried it on my Windows laptop. On the MLS map I see similar behaviour in both FF and Chrome - the map continues zooming after I lift my fingers, depending on "aggressive" my fling is. The behaviour seems comparable in both browser.

I also tried the test page and get similar cumulative numbers for similar gestures.

(In reply to Chris Peterson [:cpeterson] from comment #6)

FWIW, I can't reproduce this zooming behavior on Chrome on my Windows laptop. I only see the problem in Firefox (with allow_zooming = true).

This part is surprising to me, I can't think of differences that would account for the problem only happening in Firefox for you but not for me. Specially if the behaviour on the test page is comparable with the browsers and you're getting momentum/dmanip in Chrome.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #6)

FWIW, I can't reproduce this zooming behavior on Chrome on my Windows laptop. I only see the problem in Firefox (with allow_zooming = true).

This part is surprising to me, I can't think of differences that would account for the problem only happening in Firefox for you but not for me. Specially if the behaviour on the test page is comparable with the browsers and you're getting momentum/dmanip in Chrome.

Correction: I retested Chrome and I can reproduce the "map continues zooming after I lift my fingers" problem in Chrome, but only occasionally. In Firefox, the extra zoom happens every time I lift my fingers off the touchpad, no matter how long or motionless I leave my fingers resting on the touchpad after scrolling before lifting them.

Comment 12

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

Ah, I can reproduce something like that. On the test page, if I do a two-finger scroll and then leave my fingers on the trackpad as still as I can, Chrome stops sending events. However in FF we still send a trickle of events with small values. Maybe we should do some debouncing and throw away events with very small deltas.

Comment 13

[Timothy Nikkel (:tnikkel)]

Thanks for the investigation!

Yeah, seems like Chrome does something like that. If I do a simple quick pan (a brief touch, no lingering) then in the momentum phase Chrome never sends a delta less than 1.5.

So seems like we should try something about very small values. Two ideas come to mind: 1) ignore small deltas like kats suggested or 2) accumulate deltas and only send events when the accumulation exceeds 1.5. The latter is what we do to accumulate a value that we put in mLineOrPageDeltaX on the wheel event, which we then later use to dispatch legacy scroll events like MozMousePixelScroll (which we had to implement for dmanip to fix other issues). I'll give these a try and see what seems best.

Comment 14

[Chris Peterson [:cpeterson]]

That Chrome is ignoring small deltas is probably related to Chrome's event.deltaX/deltaY/deltaZ always being rounded to a multiple of 0.5.

Comment 15

[Timothy Nikkel (:tnikkel)]

Have you seen delta values less than 1.5 (other than 0)? I can't get any.

I did notice an interesting thing, if I start the gesture as a pinch and then do two finger panning Chrome suddenly starts giving me delta's that are not rounded to 0.5 and can be quite small. So they must have a pan mode that massages the delta's.

Comment 16

[Timothy Nikkel (:tnikkel)]

Looks like when Chromium converts pen gestures into wheel events it rounds to the nearest integer

https://source.chromium.org/chromium/chromium/src/+/master:ui/events/event.cc;l=642?q=MouseWheelEvent::MouseWheelEvent

I could easily see the 0.5 coming into play by taking into account the screen scale factor which they do in a number of places.

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1661108. Only send pan events from DManip if the delta is at least 1. r=kats

For reasons.

Comment 18

[Timothy Nikkel (:tnikkel)]

I implemented the rounding but I'm not sure if you'll find it is a significant improvement or not.

Here is a try build. Could you test it please?

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/aHojdI9ESQmx1iR2TsALIg/runs/0/artifacts/public/build/target.zip

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #15)

Have you seen delta values less than 1.5 (other than 0)? I can't get any.

Correction: earlier I said Chrome rounds delta values to multiple of 0.5, but on closer inspection, it is actually rounding to multiples of 2.5! I never see a delta value between 0 and 2.5. I see Chrome delta values like:

wheel handler called 0 -2.5 0
wheel handler called 0 -5 0
wheel handler called 0 -7.5 0
wheel handler called 0 -10 0
wheel handler called 0 -12.5 0
wheel handler called 0 15 0
wheel handler called 0 -22.5 0
wheel handler called 0 -25 0
wheel handler called 5 -35 0
wheel handler called 5 -57.5

(In reply to Timothy Nikkel (:tnikkel) from comment #18)

I implemented the rounding but I'm not sure if you'll find it is a significant improvement or not.

Here is a try build. Could you test it please?

With a regular build, I see an extra zoom jump every time I zoom the map in or out. With your try build, I still see the extra zoom every time I zoom out, but I see the extra zoom much less frequently when I zoom in. So this build is better, but not equivalent behavior to Chrome or Firefox without allow_zooming.

Comment 20

[Timothy Nikkel (:tnikkel)]

Thanks for testing.

I'm not quite sure what to do next. We don't want to round the delta's of the PanGestureInput too much because then we lose precision (for scrolls that aren't intercepted by a web content wheel listener), but the delta's we put in PanGestureInput are directly linked to the delta's on the wheel event that web content gets. Open to ideas.

Comment 21

[Timothy Nikkel (:tnikkel)]

I made a new build that increases the threshold slightly. Instead of checking that the rounded pixel delta is at least 1 this new build checks that the pixel delta (before rounding) is at least 1. Thus effectively moving the threshold from 0.5 to 1 (the actually delta that shows up in the dom event will be different because of screen scaling and maybe full (reflow) zoom).

Can you try this and report how it feels in comparison to the last build and a regular nightly build? Thanks
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Na_DhK8-Rq-l1tttkvvGvA/runs/0/artifacts/public/build/target.zip

Comment 22

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #21)

I made a new build that increases the threshold slightly. Instead of checking that the rounded pixel delta is at least 1 this new build checks that the pixel delta (before rounding) is at least 1. Thus effectively moving the threshold from 0.5 to 1 (the actually delta that shows up in the dom event will be different because of screen scaling and maybe full (reflow) zoom).

FWIW, in the wev2.html test, the events' delta values now appear to always be multiples of 0.2.

Can you try this and report how it feels in comparison to the last build and a regular nightly build? Thanks
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Na_DhK8-Rq-l1tttkvvGvA/runs/0/artifacts/public/build/target.zip

This try build feels better than the last try build. I still see an extra zoom less than half the time when I zoom out, whereas the previous build always had an extra zoom when zooming out. Like the last try build, I rarely see an extra zoom when zooming in.

Comment 23

[Timothy Nikkel (:tnikkel)]

Testing on mac it seems as though the OS only sends us touchpad sourced scrolls if the delta is 1 or greater, which we then multiple by the backing scale factor (2 on retina screens). Only sending events that have a delta of at least 1 before multiplying by the backing scale factor might also explain Chrome's behaviour, so let's give that a try.

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Sb-M6IBkTpGxOmmsVinWQw/runs/0/artifacts/public/build/target.zip

Comment 24

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #23)

Testing on mac it seems as though the OS only sends us touchpad sourced scrolls if the delta is 1 or greater, which we then multiple by the backing scale factor (2 on retina screens). Only sending events that have a delta of at least 1 before multiplying by the backing scale factor might also explain Chrome's behaviour, so let's give that a try.

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Sb-M6IBkTpGxOmmsVinWQw/runs/0/artifacts/public/build/target.zip

LGTM on my Windows laptop. I see very few extra jumps when zooming in or out.

However, zooming on all these try builds feels laggier than in Chrome or Firefox Nightly. I don't know if that's because these aren't PGO opt builds or due to the scroll event changes or just my imagination. It's pretty subjective.

Comment 25

[Timothy Nikkel (:tnikkel)]

Thanks for testing. We can look at the other issue after we land this fix.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a95c23aa4e10
Only send pan events from DManip if the delta is at least 1. r=kats

Comment 27

[Atila Butkovits]

Backed out for build bustage at DirectManipulationOwner.

Backout link: https://hg.mozilla.org/integration/autoland/rev/ff9fbfbe506c186bb915ebfbdcd6e7cc130b3384

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=a95c23aa4e100b62e71ceda1014629d77e2e4760

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314805224&repo=autoland&lineNumber=30763

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d871d71f5196
Only send pan events from DManip if the delta is at least 1. r=kats

Comment 29

[Cristina Coroiu [:ccoroiu]]

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